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]> > > Bernrado > -- Thomas Thrainer | 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, Katherine Stephens
