I accidentally squashed this together in the wrong way, FYI here the
interdiff to all previous changes in this patch:
diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index 2c00ba9..c256130 100755
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -269,7 +269,8 @@ def UpgradeInstances(config_data):
missing_spindles = False
for instance, iobj in config_data["instances"].items():
- if _ConvertNicNameToUuid(iobj, network2uuid):
+ _ConvertNicNameToUuid(iobj, network2uuid)
+ if _ConvertDiskAndCheckMissingSpindles(iobj, instance):
missing_spindles = True
if GetExclusiveStorageValue(config_data) and missing_spindles:
Cheers,
Helga
On Thu, May 22, 2014 at 4:56 PM, Jose A. Lopes <[email protected]> wrote:
> LGTM.
>
> Thank you for taking the time do this sort of boring stuff :)
> Jose
>
> On May 22 16:45, Helga Velroyen wrote:
> > On Thu, May 22, 2014 at 3:59 PM, Jose A. Lopes <[email protected]>
> wrote:
> >
> > > On May 22 14:56, 'Helga Velroyen' via ganeti-devel wrote:
> > > > A 'too-many-branches' error.
> > > >
> > > > Signed-off-by: Helga Velroyen <[email protected]>
> > > > ---
> > > > tools/cfgupgrade | 77
> > > ++++++++++++++++++++++++++++++++------------------------
> > > > 1 file changed, 44 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/tools/cfgupgrade b/tools/cfgupgrade
> > > > index bcfb84f..5025448 100755
> > > > --- a/tools/cfgupgrade
> > > > +++ b/tools/cfgupgrade
> > > > @@ -219,6 +219,46 @@ def UpgradeDiskDevType(disk):
> > > > ChangeDiskDevType(disk, DEV_TYPE_OLD_NEW)
> > > >
> > > >
> > > > +def _ConvertNicNameToUuid(iobj, network2uuid):
> > > > + for nic in iobj["nics"]:
> > > > + name = nic.get("network", None)
> > > > + if name:
> > > > + uuid = network2uuid.get(name, None)
> > > > + if uuid:
> > > > + print("NIC with network name %s found."
> > > > + " Substituting with uuid %s." % (name, uuid))
> > > > + nic["network"] = uuid
> > > > +
> > > > +
> > > > +def _ConvertDiskAndCheckMissingSpindles(iobj, instance):
> > > > + missing_spindles = None
> > >
> > > A function that returns either 'True' or 'None' is really weird.
> > > We can instead change this into
> > >
> > > missing_spindles = False
> > >
> > > and change the call (see below)
> > >
> > > > + if "disks" not in iobj:
> > > > + raise Error("Instance '%s' doesn't have a disks entry?!" %
> instance)
> > > > + disks = iobj["disks"]
> > > > + if not all(isinstance(d, str) for d in disks):
> > > > + # Disks are not top level citizens
> > > > + for idx, dobj in enumerate(disks):
> > > > + RemovePhysicalId(dobj)
> > > > +
> > > > + expected = "disk/%s" % idx
> > > > + current = dobj.get("iv_name", "")
> > > > + if current != expected:
> > > > + logging.warning("Updating iv_name for instance %s/disk %s"
> > > > + " from '%s' to '%s'",
> > > > + instance, idx, current, expected)
> > > > + dobj["iv_name"] = expected
> > > > +
> > > > + if "dev_type" in dobj:
> > > > + UpgradeDiskDevType(dobj)
> > > > +
> > > > + if not "spindles" in dobj:
> > > > + missing_spindles = True
> > > > +
> > > > + if not "uuid" in dobj:
> > > > + dobj["uuid"] = utils.io.NewUUID()
> > > > + return missing_spindles
> > > > +
> > > > +
> > > > def UpgradeInstances(config_data):
> > > > """Upgrades the instances' configuration."""
> > > >
> > > > @@ -229,39 +269,10 @@ def UpgradeInstances(config_data):
> > > >
> > > > missing_spindles = False
> > > > for instance, iobj in config_data["instances"].items():
> > > > - for nic in iobj["nics"]:
> > > > - name = nic.get("network", None)
> > > > - if name:
> > > > - uuid = network2uuid.get(name, None)
> > > > - if uuid:
> > > > - print("NIC with network name %s found."
> > > > - " Substituting with uuid %s." % (name, uuid))
> > > > - nic["network"] = uuid
> > > > -
> > > > - if "disks" not in iobj:
> > > > - raise Error("Instance '%s' doesn't have a disks entry?!" %
> > > instance)
> > > > - disks = iobj["disks"]
> > > > - if not all(isinstance(d, str) for d in disks):
> > > > - # Disks are not top level citizens
> > > > - for idx, dobj in enumerate(disks):
> > > > - RemovePhysicalId(dobj)
> > > > -
> > > > - expected = "disk/%s" % idx
> > > > - current = dobj.get("iv_name", "")
> > > > - if current != expected:
> > > > - logging.warning("Updating iv_name for instance %s/disk %s"
> > > > - " from '%s' to '%s'",
> > > > - instance, idx, current, expected)
> > > > - dobj["iv_name"] = expected
> > > > -
> > > > - if "dev_type" in dobj:
> > > > - UpgradeDiskDevType(dobj)
> > > > -
> > > > - if not "spindles" in dobj:
> > > > - missing_spindles = True
> > > > -
> > > > - if not "uuid" in dobj:
> > > > - dobj["uuid"] = utils.io.NewUUID()
> > > > + _ConvertNicNameToUuid(iobj, network2uuid)
> > > > + missing_spindles_inst =
> _ConvertDiskAndCheckMissingSpindles(iobj,
> > > instance)
> > > > + if missing_spindles_inst is not None:
> > > > + missing_spindles = missing_spindles_inst
> > >
> > > if _ConvertDiskAndCheckMissingSpindles(iobj, instance):
> > > missing_spindles = True
> > >
> > > this also removes a local variable, making it easier to read the code.
> > >
> >
> > True, it was kind of a half-hearted refactoring. Here is the interdiff,
> FYI:
> >
> > diff --git a/tools/cfgupgrade b/tools/cfgupgrade
> > index 7d64cd6..2c00ba9 100755
> > --- a/tools/cfgupgrade
> > +++ b/tools/cfgupgrade
> > @@ -231,7 +231,7 @@ def _ConvertNicNameToUuid(iobj, network2uuid):
> >
> >
> > def _ConvertDiskAndCheckMissingSpindles(iobj, instance):
> > - missing_spindles = None
> > + missing_spindles = False
> > if "disks" not in iobj:
> > raise Error("Instance '%s' doesn't have a disks entry?!" % instance)
> > disks = iobj["disks"]
> > @@ -269,10 +269,8 @@ def UpgradeInstances(config_data):
> >
> > missing_spindles = False
> > for instance, iobj in config_data["instances"].items():
> > - _ConvertNicNameToUuid(iobj, network2uuid)
> > - missing_spindles_inst = _ConvertDiskAndCheckMissingSpindles(iobj,
> > instance)
> > - if missing_spindles_inst is not None:
> > - missing_spindles = missing_spindles_inst
> > + if _ConvertNicNameToUuid(iobj, network2uuid):
> > + missing_spindles = True
> >
> > if GetExclusiveStorageValue(config_data) and missing_spindles:
> > # We cannot be sure that the instances that are missing spindles
> have
> >
> >
> >
> > >
> > > Rest LGTM.
> > >
> > > Thanks,
> > > Jose
> > >
> > > >
> > > > if GetExclusiveStorageValue(config_data) and missing_spindles:
> > > > # We cannot be sure that the instances that are missing spindles
> > > have
> > > > --
> > > > 1.9.1.423.g4596e3a
> > > >
> > >
> > > --
> > > Jose Antonio Lopes
> > > Ganeti Engineering
> > > Google Germany GmbH
> > > Dienerstr. 12, 80331, München
> > >
> > > Registergericht und -nummer: Hamburg, HRB 86891
> > > Sitz der Gesellschaft: Hamburg
> > > Geschäftsführer: Graham Law, Christine Elizabeth Flores
> > > Steuernummer: 48/725/00206
> > > Umsatzsteueridentifikationsnummer: DE813741370
> > >
> >
> >
> >
> > --
> > --
> > Helga Velroyen | Software Engineer | [email protected] |
> >
> > Google Germany GmbH
> > Dienerstr. 12
> > 80331 München
> >
> > Registergericht und -nummer: Hamburg, HRB 86891
> > Sitz der Gesellschaft: Hamburg
> > Geschäftsführer: Graham Law, Christine Elizabeth Flores
>
> --
> Jose Antonio Lopes
> Ganeti Engineering
> Google Germany GmbH
> Dienerstr. 12, 80331, München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Steuernummer: 48/725/00206
> Umsatzsteueridentifikationsnummer: DE813741370
>
--
--
Helga Velroyen | Software Engineer | [email protected] |
Google Germany GmbH
Dienerstr. 12
80331 München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
