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.
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
