On Mon, Sep 30, 2013 at 5:32 PM, Santi Raffa <[email protected]> wrote:
> Interdiff:
>
> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
> index 78161f8..367b405 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, CheckAccessTypeConsistency
> + CheckIpolicyVsDiskTemplates, CheckDiskAccessModeConsistency
>
> import ganeti.masterd.instance
>
> @@ -704,7 +704,6 @@ 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)
>
The check for the 'access' parameter being one of ['userspace',
'kernelspace'] should actually be done in CheckArguments. Only the instance
check has to be done in CheckPrereq.
> except errors.OpPrereqError, err:
> raise errors.OpPrereqError("While verify diskparams options: %s"
> % err,
> errors.ECODE_INVAL)
> @@ -1025,6 +1024,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 27170ac..f527e94 100644
> --- a/lib/cmdlib/common.py
> +++ b/lib/cmdlib/common.py
> @@ -1135,7 +1135,7 @@ def CheckIpolicyVsDiskTemplates(ipolicy,
> enabled_disk_templates):
> " cluster: %s" %
> utils.CommaJoin(not_enabled))
>
>
> -def CheckAccessTypeConsistency(parameters, cfg, group=None):
> +def CheckDiskAccessModeConsistency(parameters, cfg, group=None):
> """Checks if the access param is consistent with the cluster
> configuration.
>
> @param parameters: the parameters to validate
> @@ -1152,8 +1152,8 @@ def CheckAccessTypeConsistency(parameters, cfg,
> group=None):
> 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)
> + 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,
>
That's actually the part which should be extracted and called in
CheckArguments.
> @@ -1162,28 +1162,47 @@ def CheckAccessTypeConsistency(parameters,
> cfg, group=None):
>
> #Check the combination of instance hypervisor, disk template and
> access
> #protocol is sane.
> - instances = cfg.GetNodeGroupInstances(group) if group else \
> - cfg.GetInstanceList()
> + inst_uuids = cfg.GetNodeGroupInstances(group) if group else \
> + cfg.GetInstanceList()
>
> - for instance in instances:
> + for entry in inst_uuids:
> #hyp, disk, access
> - information = cfg.GetInstanceInfo(instance)
> - hv = information.hypervisor
> - for disk in information.disks:
> + inst = cfg.GetInstanceInfo(entry)
> + hv = inst.hypervisor
> + for disk in inst.disks:
>
Sorry for not spotting earlier: You don't have to iterate, all disks of an
instance must have the same disk template. And there is inst.disk_template
which holds this value.
> 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:
> + if not IsValidDiskAccessModeCombination(hv, template, access):
> raise errors.OpPrereqError("Instance {i}: cannot use '{a}'
> access"
> " setting with {h} hypervisor
> and {d} disk"
> - " type.".format(i=information.name,
> + " type.".format(i=inst.name,
> a=access,
> h=hv,
> d=template))
>
> - return
> +
> +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 a given read disk_template with
> + the specified mode.
>
a? Should that read access?
> +
> + """
> +
> + 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 0700fb4..10b0eeb 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, CheckAccessTypeConsistency
> + CheckIpolicyVsDiskTemplates, CheckDiskAccessModeConsistency
>
> import ganeti.masterd.instance
>
> @@ -503,8 +503,8 @@ class LUGroupSetParams(LogicalUnit):
>
> try:
> utils.VerifyDictOptions(self.new_diskparams,
> constants.DISK_DT_DEFAULTS)
> - CheckAccessTypeConsistency(self.new_diskparams, self.cfg,
> - group=self.group)
> + 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 9a64ccc..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, \
> @@ -1175,8 +1175,10 @@ class LUInstanceCreate(LogicalUnit):
> 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:
> +
> + 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),
> diff --git a/lib/config.py b/lib/config.py
> index f2afd4b..21789f4 100644
> --- a/lib/config.py
> +++ b/lib/config.py
> @@ -669,12 +669,13 @@ class ConfigWriter(object):
>
> 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"
> - " %s)" % (constants.DT_RBD,
> - constants.LDP_ACCESS, access,
> - ", ".join(
> -
> constants.DISK_VALID_ACCESS_PROTOCOLS)))
> + 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:
>
>
>
> --
> Raffa Santi
> 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
>
--
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
