LGTM. Thanks, Jose
On May 22 17:06, Helga Velroyen wrote: > 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 -- 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
