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
