On 17 May 2013 11:13, Thomas Thrainer <[email protected]> wrote: > > > > On Fri, May 17, 2013 at 10:52 AM, Bernardo Dal Seno <[email protected]> > wrote: >> >> On 17 May 2013 09:08, Thomas Thrainer <[email protected]> wrote: >> > Interdiff: >> > >> > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py >> > index 5f94775..b5dc099 100644 >> > --- a/lib/cmdlib/instance.py >> > +++ b/lib/cmdlib/instance.py >> > @@ -56,7 +56,8 @@ from ganeti.cmdlib.common import INSTANCE_ONLINE, >> > INSTANCE_DOWN, \ >> > _IsExclusiveStorageEnabledNode, _CheckHVParams, _CheckOSParams, \ >> > _GetWantedInstances, _CheckInstancesNodeGroups, _AnnotateDiskParams, >> > \ >> > _GetUpdatedParams, _ExpandInstanceName, _FindFaultyInstanceDisks, \ >> > - _ComputeIPolicySpecViolation, _CheckInstanceState, _ExpandNodeName >> > + _ComputeIPolicySpecViolation, _ComputeIPolicyInstanceViolation, \ >> > + _CheckInstanceState, _ExpandNodeName >> > from ganeti.cmdlib.instance_utils import _AssembleInstanceDisks, \ >> > _BuildInstanceHookEnvByObject, _GetClusterDomainSecret, \ >> > _BuildInstanceHookEnv, _NICListToTuple, _NICToTuple, >> > _CheckNodeNotDrained, \ >> > @@ -1190,7 +1191,7 @@ class LUInstanceCreate(LogicalUnit): >> > if self.op.no_install and self.op.start: >> > self.LogInfo("No-installation mode selected, disabling startup") >> > self.op.start = False >> > - # validate/normalize the instance name >> > + # validate/normalize the instance name >> > self.op.instance_name = \ >> > netutils.Hostname.GetNormalizedName(self.op.instance_name) >> > >> > @@ -1202,7 +1203,7 @@ class LUInstanceCreate(LogicalUnit): >> > # check nics' parameter names >> > for nic in self.op.nics: >> > utils.ForceDictType(nic, constants.INIC_PARAMS_TYPES) >> > - # check that NIC's parameters names are unique and valid >> > + # check that NIC's parameters names are unique and valid >> > utils.ValidateDeviceNames("NIC", self.op.nics) >> > >> > # check that disk's names are unique and valid >> > @@ -1260,7 +1261,7 @@ class LUInstanceCreate(LogicalUnit): >> > >> > # file storage checks >> > if (self.op.file_driver and >> > - not self.op.file_driver in constants.FILE_DRIVER): >> > + not self.op.file_driver in constants.FILE_DRIVER): >> > raise errors.OpPrereqError("Invalid file driver name '%s'" % >> > self.op.file_driver, >> > errors.ECODE_INVAL) >> > >> > @@ -1599,7 +1600,7 @@ class LUInstanceCreate(LogicalUnit): >> > self.op.tags = einfo.get(constants.INISECT_INS, "tags").split() >> > >> > if (self.op.hypervisor is None and >> > - einfo.has_option(constants.INISECT_INS, "hypervisor")): >> > + einfo.has_option(constants.INISECT_INS, "hypervisor")): >> > self.op.hypervisor = einfo.get(constants.INISECT_INS, >> > "hypervisor") >> > >> > if einfo.has_section(constants.INISECT_HYP): >> > @@ -1614,7 +1615,7 @@ class LUInstanceCreate(LogicalUnit): >> > for name, value in einfo.items(constants.INISECT_BEP): >> > if name not in self.op.beparams: >> > self.op.beparams[name] = value >> > - # Compatibility for the old "memory" be param >> > + # Compatibility for the old "memory" be param >> > if name == constants.BE_MEMORY: >> > if constants.BE_MAXMEM not in self.op.beparams: >> > self.op.beparams[constants.BE_MAXMEM] = value >> > @@ -1624,7 +1625,7 @@ class LUInstanceCreate(LogicalUnit): >> > # try to read the parameters old style, from the main section >> > for name in constants.BES_PARAMETERS: >> > if (name not in self.op.beparams and >> > - einfo.has_option(constants.INISECT_INS, name)): >> > + einfo.has_option(constants.INISECT_INS, name)): >> > self.op.beparams[name] = einfo.get(constants.INISECT_INS, >> > name) >> > >> > if einfo.has_section(constants.INISECT_OSP): >> > @@ -1642,18 +1643,18 @@ class LUInstanceCreate(LogicalUnit): >> > for name in self.op.hvparams.keys(): >> > if name in hv_defs and hv_defs[name] == self.op.hvparams[name]: >> > del self.op.hvparams[name] >> > - # beparams >> > + # beparams >> > be_defs = cluster.SimpleFillBE({}) >> > for name in self.op.beparams.keys(): >> > if name in be_defs and be_defs[name] == self.op.beparams[name]: >> > del self.op.beparams[name] >> > - # nic params >> > + # nic params >> > nic_defs = cluster.SimpleFillNIC({}) >> > for nic in self.op.nics: >> > for name in constants.NICS_PARAMETERS: >> > if name in nic and name in nic_defs and nic[name] == >> > nic_defs[name]: >> > del nic[name] >> > - # osparams >> > + # osparams >> > os_defs = cluster.SimpleFillOS(self.op.os_type, {}) >> > for name in self.op.osparams.keys(): >> > if name in os_defs and os_defs[name] == self.op.osparams[name]: >> > @@ -1702,12 +1703,12 @@ class LUInstanceCreate(LogicalUnit): >> > self._old_instance_name = None >> > >> > if (not self.cfg.GetVGName() and >> > - self.op.disk_template not in constants.DTS_NOT_LVM): >> > + self.op.disk_template not in constants.DTS_NOT_LVM): >> > raise errors.OpPrereqError("Cluster does not support lvm-based" >> > " instances", errors.ECODE_STATE) >> > >> > if (self.op.hypervisor is None or >> > - self.op.hypervisor == constants.VALUE_AUTO): >> > + self.op.hypervisor == constants.VALUE_AUTO): >> > self.op.hypervisor = self.cfg.GetHypervisorType() >> > >> > cluster = self.cfg.GetClusterInfo() >> > @@ -1941,7 +1942,7 @@ class LUInstanceCreate(LogicalUnit): >> > raise errors.OpPrereqError("Online logical volumes found, >> > cannot" >> > " adopt: %s" % >> > utils.CommaJoin(online_lvs), >> > errors.ECODE_STATE) >> > - # update the size of disk based on what is found >> > + # update the size of disk based on what is found >> > for dsk in self.disks: >> > dsk[constants.IDISK_SIZE] = \ >> > int(float(node_lvs["%s/%s" % (dsk[constants.IDISK_VG], >> > @@ -2318,7 +2319,7 @@ class LUInstanceRename(LogicalUnit): >> > hostname = _CheckHostnameSane(self, new_name) >> > new_name = self.op.new_name = hostname.name >> > if (self.op.ip_check and >> > - netutils.TcpPing(hostname.ip, >> > constants.DEFAULT_NODED_PORT)): >> > + netutils.TcpPing(hostname.ip, constants.DEFAULT_NODED_PORT)): >> > raise errors.OpPrereqError("IP %s of instance %s already in >> > use" % >> > (hostname.ip, new_name), >> > errors.ECODE_NOTUNIQUE) >> > @@ -2337,7 +2338,7 @@ class LUInstanceRename(LogicalUnit): >> > >> > rename_file_storage = False >> > if (inst.disk_template in constants.DTS_FILEBASED and >> > - self.op.new_name != inst.name): >> > + self.op.new_name != inst.name): >> > old_file_storage_dir = >> > os.path.dirname(inst.disks[0].logical_id[1]) >> > rename_file_storage = True >> > >> > @@ -2475,6 +2476,27 @@ def _CheckInstanceBridgesExist(lu, instance, >> > node=None): >> > _CheckNicsBridgesExist(lu, instance.nics, node) >> > >> > >> > +def _ComputeIPolicyNodeViolation(ipolicy, instance, current_group, >> > + target_group, cfg, >> > + >> > _compute_fn=_ComputeIPolicyInstanceViolation): >> > + """Compute if instance meets the specs of the new target group. >> > + >> > + @param ipolicy: The ipolicy to verify >> > + @param instance: The instance object to verify >> > + @param current_group: The current group of the instance >> > + @param target_group: The new group of the instance >> > + @type cfg: L{config.ConfigWriter} >> > + @param cfg: Cluster configuration >> > + @param _compute_fn: The function to verify ipolicy (unittest only) >> > + @see: L{_ComputeIPolicySpecViolation} >> > + >> > + """ >> > + if current_group == target_group: >> > + return [] >> > + else: >> > + return _compute_fn(ipolicy, instance, cfg) >> > + >> > + >> > def _CheckTargetNodeIPolicy(lu, ipolicy, instance, node, cfg, >> > ignore=False, >> > _compute_fn=_ComputeIPolicyNodeViolation): >> > """Checks that the target node is correct in terms of instance >> > policy. >> > @@ -3344,7 +3366,7 @@ class LUInstanceRecreateDisks(LogicalUnit): >> > errors.ECODE_INVAL) >> > >> > if ((self.op.nodes or self.op.iallocator) and >> > - sorted(self.disks.keys()) != range(len(instance.disks))): >> > + sorted(self.disks.keys()) != range(len(instance.disks))): >> > raise errors.OpPrereqError("Can't recreate disks partially and" >> > " change the nodes at the same time", >> > errors.ECODE_INVAL) >> > @@ -3385,7 +3407,7 @@ class LUInstanceRecreateDisks(LogicalUnit): >> > # need to update the nodes and minors >> > assert len(self.op.nodes) == 2 >> > assert len(disk.logical_id) == 6 # otherwise disk internals >> > - # have changed >> > + # have changed >> > (_, _, old_port, _, _, old_secret) = disk.logical_id >> > new_minors = self.cfg.AllocateDRBDMinor(self.op.nodes, >> > instance.name) >> > new_id = (self.op.nodes[0], self.op.nodes[1], old_port, >> > @@ -4104,7 +4126,7 @@ class LUInstanceReinstall(LogicalUnit): >> > assert instance is not None, \ >> > "Cannot retrieve locked instance %s" % self.op.instance_name >> > _CheckNodeOnline(self, instance.primary_node, "Instance primary >> > node" >> > - " offline, cannot >> > reinstall") >> > + " offline, cannot reinstall") >> > >> > if instance.disk_template == constants.DT_DISKLESS: >> > raise errors.OpPrereqError("Instance '%s' has no disks" % >> > diff --git a/lib/cmdlib/instance_utils.py b/lib/cmdlib/instance_utils.py >> > index adae1cb..8718af3 100644 >> > --- a/lib/cmdlib/instance_utils.py >> > +++ b/lib/cmdlib/instance_utils.py >> > @@ -238,7 +238,7 @@ def _ShutdownInstanceDisks(lu, instance, disks=None, >> > ignore_primary=False): >> > lu.LogWarning("Could not shutdown block device %s on node %s: >> > %s", >> > disk.iv_name, node, msg) >> > if ((node == instance.primary_node and not ignore_primary) or >> > - (node != instance.primary_node and not result.offline)): >> > + (node != instance.primary_node and not result.offline)): >> > all_result = False >> > return all_result >> > >> > >> >> LGTM, but don't forget to update the patch comment. > > > Sorry, forgot to mention, but the comment is reworded to: > > Extract instance related logical units from cmdlib > > All LUInstance* classes are extracted to instance.py. Common functions > are moved to common.py if used by non-instance logical units as well. > Additionally, helper functions which are only used by LUBackup* and > LUInstance* are moved to instance_utils.py. > > Signed-off-by: Thomas Thrainer <[email protected]>
LGTM (still)! Bernardo
