On Tue, Oct 1, 2013 at 4:33 PM, Santi Raffa <[email protected]> wrote:
> Interdiff on top of previous interdiff:
>
> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
> index 367b405..0e18f4c 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, CheckDiskAccessModeConsistency
> + CheckIpolicyVsDiskTemplates, CheckDiskAccessModeValidity,
> + CheckDiskAccessModeConsistency
>
> import ganeti.masterd.instance
>
> @@ -704,10 +705,13 @@ 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)
>
> +
> +
>
I guess those additional newlines won't lint...
> def ExpandNames(self):
> # FIXME: in the future maybe other cluster params won't require
> checking on
> # all nodes to be modified.
> diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py
> index f527e94..d8e185f 100644
> --- a/lib/cmdlib/common.py
> +++ b/lib/cmdlib/common.py
> @@ -1135,9 +1135,29 @@ def CheckIpolicyVsDiskTemplates(ipolicy,
> enabled_disk_templates):
> " 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.
> + """
> +
> + 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.
> @@ -1149,6 +1169,9 @@ def CheckDiskAccessModeConsistency(parameters,
> cfg, group=None):
> @return: None if all checks are passed.
> """
>
> + # For good measure
> + CheckDiskAccessModeValidity(parameters)
> +
> if constants.DT_RBD in parameters:
> access = parameters[constants.DT_RBD].get(constants.LDP_ACCESS,
> constants.DISK_KERNELSPACE)
> @@ -1169,20 +1192,19 @@ def CheckDiskAccessModeConsistency(parameters,
> cfg, group=None):
> #hyp, disk, access
> inst = cfg.GetInstanceInfo(entry)
> hv = inst.hypervisor
> - for disk in inst.disks:
> - template = disk.dev_type
> + dt = inst.disk_template
>
> - #do not check for disk types that don't have this setting.
> - if disk.dev_type not in (constants.DT_RBD):
> - continue
> + #do not check for disk types that don't have this setting.
> + if dt not in (constants.DT_RBD):
> + continue
>
> - 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=inst.name,
> - a=access,
> - h=hv,
> - d=template))
> + 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):
> @@ -1191,8 +1213,8 @@ def IsValidDiskAccessModeCombination(hv,
> disk_template, 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.
> + @return: True if the hypervisor can read a given read disk_template
> + in the specified mode.
>
> """
>
> diff --git a/lib/cmdlib/group.py b/lib/cmdlib/group.py
> index 10b0eeb..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, CheckDiskAccessModeConsistency
> + 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)
>
> --
> 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
>
LGTM, but I got a bit lost with all the interdiffs, I will take another
look at the whole patch once it is resent as a whole.
Cheers,
Thomas
--
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
