On Wed, Oct 2, 2013 at 11:02 AM, Raffa Santi <[email protected]> wrote:
> * Prevent incompatible hypervisor/disk template/access type > combinations from being reached through the command line. > * Prevent the access config from taking illegal values such as "pink > bunny" through the command line. > > Signed-off-by: Raffa Santi <[email protected]> > --- > lib/cmdlib/cluster.py | 5 ++- > lib/cmdlib/common.py | 95 > ++++++++++++++++++++++++++++++++++++++++++++++++ > lib/cmdlib/group.py | 9 ++++- > lib/cmdlib/instance.py | 18 ++++++++- > lib/config.py | 10 +++++ > 5 files changed, 134 insertions(+), 3 deletions(-) > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index 5ed5aa7..998de8e 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -57,7 +57,8 @@ from ganeti.cmdlib.common import ShareAll, RunPostHook, \ > GetUpdatedIPolicy, ComputeNewInstanceViolations, GetUpdatedParams, \ > CheckOSParams, CheckHVParams, AdjustCandidatePool, CheckNodePVs, \ > ComputeIPolicyInstanceViolation, AnnotateDiskParams, SupportsOob, \ > - CheckIpolicyVsDiskTemplates > + CheckIpolicyVsDiskTemplates, CheckDiskAccessModeValidity, > + CheckDiskAccessModeConsistency > > import ganeti.masterd.instance > > @@ -704,6 +705,7 @@ class LUClusterSetParams(LogicalUnit): > utils.ForceDictType(dt_params, constants.DISK_DT_TYPES) > try: > utils.VerifyDictOptions(self.op.diskparams, > constants.DISK_DT_DEFAULTS) > + CheckDiskAccessModeValidity(self.op.diskparams) > except errors.OpPrereqError, err: > raise errors.OpPrereqError("While verify diskparams options: %s" > % err, > errors.ECODE_INVAL) > @@ -1024,6 +1026,7 @@ class LUClusterSetParams(LogicalUnit): > self.new_diskparams[dt_name] = dt_params > else: > self.new_diskparams[dt_name].update(dt_params) > + CheckDiskAccessModeConsistency(self.op.diskparams, self.cfg) > > # os hypervisor parameters > self.new_os_hvp = objects.FillDict(cluster.os_hvp, {}) > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 80334e9..d8e185f 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -1133,3 +1133,98 @@ def CheckIpolicyVsDiskTemplates(ipolicy, > enabled_disk_templates): > raise errors.OpPrereqError("The following disk template are allowed" > " by the ipolicy, but not enabled on the" > " cluster: %s" % > utils.CommaJoin(not_enabled)) > + > + > +def CheckDiskAccessModeValidity(parameters): > + """Checks if the access parameter is legal. > + > + @see: L{CheckDiskAccessModeConsistency} for cluster consistency checks. > + @raise errors.OpPrereqError: if the check fails. > + """ > + > The newline is in the comment, not afterwards. > + if constants.DT_RBD in parameters: > + access = parameters[constants.DT_RBD].get(constants.LDP_ACCESS, > + constants.DISK_KERNELSPACE) > + if access not in constants.DISK_VALID_ACCESS_MODES: > + valid_vals_str = utils.CommaJoin(constants.DISK_VALID_ACCESS_MODES) > + raise errors.OpPrereqError("Invalid value of '{d}:{a}': '{v}' > (expected" > + " one of {o})".format(d=constants.DT_RBD, > + > a=constants.LDP_ACCESS, > + v=access, > + o=valid_vals_str)) > + > + > +def CheckDiskAccessModeConsistency(parameters, cfg, group=None): > + """Checks if the access param is consistent with the cluster > configuration. > + > + @note: requires a configuration lock to run. > + @param parameters: the parameters to validate > + @param cfg: the cfg object of the cluster > + @param group: if set, only check for consistency within this group. > + @raise errors.OpPrereqError: if the LU attempts to change the access > parameter > + to an invalid value, such as "pink bunny". > + @raise errors.OpPrereqError: if the LU attempts to change the access > parameter > + to an inconsistent value, such as asking > for RBD > + userspace access to the chroot hypervisor. > + @return: None if all checks are passed. > + """ > + > Same here. > + # For good measure > + CheckDiskAccessModeValidity(parameters) > + > + if constants.DT_RBD in parameters: > + access = parameters[constants.DT_RBD].get(constants.LDP_ACCESS, > + constants.DISK_KERNELSPACE) > + if access not in constants.DISK_VALID_ACCESS_MODES: > + valid_vals_str = utils.CommaJoin(constants.DISK_VALID_ACCESS_MODES) > + raise errors.OpPrereqError("Invalid value of '{d}:{a}': '{v}' > (expected" > + " one of {o})".format(d=constants.DT_RBD, > + > a=constants.LDP_ACCESS, > + v=access, > + o=valid_vals_str)) > This whole check is copied from the code above. > + > + #Check the combination of instance hypervisor, disk template and > access > + #protocol is sane. > + inst_uuids = cfg.GetNodeGroupInstances(group) if group else \ > + cfg.GetInstanceList() > + > + for entry in inst_uuids: > + #hyp, disk, access > + inst = cfg.GetInstanceInfo(entry) > + hv = inst.hypervisor > + dt = inst.disk_template > + > + #do not check for disk types that don't have this setting. > + if dt not in (constants.DT_RBD): > + continue > + > + if not IsValidDiskAccessModeCombination(hv, dt, access): > + raise errors.OpPrereqError("Instance {i}: cannot use '{a}' access" > + " setting with {h} hypervisor and {d} > disk" > + " type.".format(i=inst.name, > + a=access, > + h=hv, > + d=dt)) > + > + > +def IsValidDiskAccessModeCombination(hv, disk_template, mode): > + """Checks if an hypervisor can read a disk template with given mode. > + > + @param hv: the hypervisor that will access the data > + @param disk_template: the disk template the data is stored as > + @param mode: how the hypervisor should access the data > + @return: True if the hypervisor can read a given read disk_template > + in the specified mode. > + > + """ > + > + if mode == constants.DISK_KERNELSPACE: > + return True > + > + if (hv == constants.HT_KVM and > + disk_template == constants.DT_RBD and > + mode == constants.DISK_USERSPACE): > + return True > + > + # Everything else: > + return False > diff --git a/lib/cmdlib/group.py b/lib/cmdlib/group.py > index 4c0ea03..e14e600 100644 > --- a/lib/cmdlib/group.py > +++ b/lib/cmdlib/group.py > @@ -39,7 +39,8 @@ from ganeti.cmdlib.common import MergeAndVerifyHvState, \ > CheckNodeGroupInstances, GetUpdatedIPolicy, \ > ComputeNewInstanceViolations, GetDefaultIAllocator, ShareAll, \ > CheckInstancesNodeGroups, LoadNodeEvacResult, MapInstanceLvsToNodes, \ > - CheckIpolicyVsDiskTemplates > + CheckIpolicyVsDiskTemplates, CheckDiskAccessModeValidity, > + CheckDiskAccessModeConsistency > > import ganeti.masterd.instance > > @@ -406,6 +407,9 @@ class LUGroupSetParams(LogicalUnit): > raise errors.OpPrereqError("Please pass at least one modification", > errors.ECODE_INVAL) > > + if self.op.diskparams: > + CheckDiskAccessModeValidity(self.op.diskparams) > + > def ExpandNames(self): > # This raises errors.OpPrereqError on its own: > self.group_uuid = self.cfg.LookupNodeGroup(self.op.group_name) > @@ -500,8 +504,11 @@ class LUGroupSetParams(LogicalUnit): > # As we've all subdicts of diskparams ready, lets merge the actual > # dict with all updated subdicts > self.new_diskparams = objects.FillDict(diskparams, new_diskparams) > + > try: > utils.VerifyDictOptions(self.new_diskparams, > constants.DISK_DT_DEFAULTS) > + CheckDiskAccessModeConsistency(self.new_diskparams, self.cfg, > + group=self.group) > except errors.OpPrereqError, err: > raise errors.OpPrereqError("While verify diskparams options: %s" > % err, > errors.ECODE_INVAL) > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > index 83f5128..1c5f522 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -49,7 +49,7 @@ from ganeti.cmdlib.common import INSTANCE_DOWN, \ > IsExclusiveStorageEnabledNode, CheckHVParams, CheckOSParams, \ > AnnotateDiskParams, GetUpdatedParams, ExpandInstanceUuidAndName, \ > ComputeIPolicySpecViolation, CheckInstanceState, ExpandNodeUuidAndName, > \ > - CheckDiskTemplateEnabled > + CheckDiskTemplateEnabled, IsValidDiskAccessModeCombination > from ganeti.cmdlib.instance_storage import CreateDisks, \ > CheckNodesFreeDiskPerVG, WipeDisks, WipeOrCleanupDisks, WaitForSync, \ > IsExclusiveStorageEnabledNodeUuid, CreateSingleBlockDev, ComputeDisks, \ > @@ -1168,6 +1168,22 @@ class LUInstanceCreate(LogicalUnit): > dsk[constants.IDISK_SIZE] = \ > int(float(node_disks[dsk[constants.IDISK_ADOPT]])) > > + # Check disk access param to be compatible with specified hypervisor > + node_info = self.cfg.GetNodeInfo(self.op.pnode_uuid) > + node_group = self.cfg.GetNodeGroup(node_info.group) > + disk_params = self.cfg.GetGroupDiskParams(node_group) > + access_type = disk_params[self.op.disk_template].get( > + constants.LDP_ACCESS, constants.DISK_KERNELSPACE > + ) > + > + if not IsValidDiskAccessModeCombination(self.op.hypervisor, > + self.op.disk_template, > + access_type): > + raise errors.OpPrereqError("Selected hypervisor (%s) cannot be" > + " used with %s disk access param" % > + (self.op.hypervisor, access_type), > + errors.ECODE_STATE) > + > # Verify instance specs > spindle_use = self.be_full.get(constants.BE_SPINDLE_USE, None) > ispec = { > diff --git a/lib/config.py b/lib/config.py > index 6894a38..21789f4 100644 > --- a/lib/config.py > +++ b/lib/config.py > @@ -667,6 +667,16 @@ class ConfigWriter(object): > constants.NDS_PARAMETER_TYPES) > _helper_ipolicy("cluster", cluster.ipolicy, True) > > + if constants.DT_RBD in cluster.diskparams: > + access = cluster.diskparams[constants.DT_RBD][constants.LDP_ACCESS] > + if access not in constants.DISK_VALID_ACCESS_MODES: > + result.append( > + "Invalid value of '%s:%s': '%s' (expected one of %s)" % ( > + constants.DT_RBD, constants.LDP_ACCESS, access, > + utils.CommaJoin(constants.DISK_VALID_ACCESS_MODES) > + ) > + ) > + > # per-instance checks > for instance_uuid in data.instances: > instance = data.instances[instance_uuid] > -- > 1.7.10.4 > > Rest LGTM, no need to resend, I'll fix it before pushing. -- 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, Christine Elizabeth Flores
