On Fri, Sep 27, 2013 at 3:35 PM, 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 | 3 ++- > lib/cmdlib/common.py | 54 > ++++++++++++++++++++++++++++++++++++++++++++++++ > lib/cmdlib/group.py | 5 ++++- > lib/cmdlib/instance.py | 14 +++++++++++++ > lib/config.py | 9 ++++++++ > 5 files changed, 83 insertions(+), 2 deletions(-) > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index 5ed5aa7..78161f8 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -57,7 +57,7 @@ from ganeti.cmdlib.common import ShareAll, RunPostHook, \ > GetUpdatedIPolicy, ComputeNewInstanceViolations, GetUpdatedParams, \ > CheckOSParams, CheckHVParams, AdjustCandidatePool, CheckNodePVs, \ > ComputeIPolicyInstanceViolation, AnnotateDiskParams, SupportsOob, \ > - CheckIpolicyVsDiskTemplates > + CheckIpolicyVsDiskTemplates, CheckAccessTypeConsistency > Same as on the Haskell side: CheckAccessTypeConsistency is not really verbose enough, given that the import comes from common. Additionally, here you call it AccessType. Please settle on DiskAccessMode throughout your code. > > import ganeti.masterd.instance > > @@ -704,6 +704,7 @@ class LUClusterSetParams(LogicalUnit): > utils.ForceDictType(dt_params, constants.DISK_DT_TYPES) > try: > utils.VerifyDictOptions(self.op.diskparams, > constants.DISK_DT_DEFAULTS) > + CheckAccessTypeConsistency(self.op.diskparams, self.cfg) > In CheckArguments, no locks are held yet. So you are not allowed to access the configuration, as it could change again until the LU is actually executed. CheckPrereq is the right place for checks which depend on the configuration. Having said that, you still can perform validity checks (if the values are correct) in CheckArguments. > except errors.OpPrereqError, err: > raise errors.OpPrereqError("While verify diskparams options: %s" > % err, > errors.ECODE_INVAL) > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 80334e9..27170ac 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -1133,3 +1133,57 @@ 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 CheckAccessTypeConsistency(parameters, cfg, group=None): > + """Checks if the access param is consistent with the cluster > configuration. > + > + @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. > You could just omit the @return part if there is no return value. > + """ > + > + 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_PROTOCOLS: > + valid_vals_str = > utils.CommaJoin(constants.DISK_VALID_ACCESS_PROTOCOLS) > + 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)) > + > + #Check the combination of instance hypervisor, disk template and > access > + #protocol is sane. > + instances = cfg.GetNodeGroupInstances(group) if group else \ > + cfg.GetInstanceList() > Per convention, this variable should be named inst_uuids... > + > + for instance in instances: > + #hyp, disk, access > + information = cfg.GetInstanceInfo(instance) > ... and this one inst. > + hv = information.hypervisor > + for disk in information.disks: > + template = disk.dev_type > + > + #do not check for disk types that don't have this setting. > + if disk.dev_type not in (constants.DT_RBD): > + continue > + > + access_combination = (hv, template, access) > + > + if access_combination not in > constants.DISK_VALID_ACCESS_COMBINATIONS: > Somehow it's weird that the logic of this check is hidden in a constant. If a potential contributor tries to figure out where to add a new valid combination, it would be really hard. I would prefer to remove the logic from the constants but use a proper method here. > + raise errors.OpPrereqError("Instance {i}: cannot use '{a}' > access" > + " setting with {h} hypervisor and > {d} disk" > + " type.".format(i=information.name, > + a=access, > + h=hv, > + d=template)) > + > + return > No need for an explicit return. > diff --git a/lib/cmdlib/group.py b/lib/cmdlib/group.py > index 4c0ea03..0700fb4 100644 > --- a/lib/cmdlib/group.py > +++ b/lib/cmdlib/group.py > @@ -39,7 +39,7 @@ from ganeti.cmdlib.common import MergeAndVerifyHvState, \ > CheckNodeGroupInstances, GetUpdatedIPolicy, \ > ComputeNewInstanceViolations, GetDefaultIAllocator, ShareAll, \ > CheckInstancesNodeGroups, LoadNodeEvacResult, MapInstanceLvsToNodes, \ > - CheckIpolicyVsDiskTemplates > + CheckIpolicyVsDiskTemplates, CheckAccessTypeConsistency > > import ganeti.masterd.instance > > @@ -500,8 +500,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) > + CheckAccessTypeConsistency(self.new_diskparams, self.cfg, > + group=self.group) > Here it's actually already in CheckPrereq ;-). But again, the parameter validity could already be checked in CheckArguments (the "is it a valid string" part). > 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..9a64ccc 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -1168,6 +1168,20 @@ 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 > + ) > + access_combo = (self.op.hypervisor, self.op.disk_template, > access_type) > + if access_combo not in constants.DISK_VALID_ACCESS_COMBINATIONS: > + raise errors.OpPrereqError("Selected hypervisor (%s) cannot be" > + " used with %s disk access param" % > + (self.op.hypervisor, access_type), > + errors.ECODE_STATE) > This check should be in common, with the other ones. And the logic shouldn't be encoded in constants. > + > # 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..f2afd4b 100644 > --- a/lib/config.py > +++ b/lib/config.py > @@ -667,6 +667,15 @@ 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_PROTOCOLS: > + result.append("Invalid value of '%s:%s': '%s' (expected one of" > Why split the string? It would fit on one line. > + " %s)" % (constants.DT_RBD, > + constants.LDP_ACCESS, access, > Those lines should line up with the first one, AFAIK. > + ", ".join( > + > constants.DISK_VALID_ACCESS_PROTOCOLS))) > + > # per-instance checks > for instance_uuid in data.instances: > instance = data.instances[instance_uuid] > -- > 1.7.10.4 > > -- 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
