On Thu, Dec 5, 2013 at 1:07 PM, Santi Raffa <[email protected]> wrote:
> On Thu, Dec 5, 2013 at 11:11 AM, Thomas Thrainer <[email protected]> > wrote: > >> diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > >> index 5a1a017..d31d011 100644 > >> --- a/lib/cmdlib/common.py > >> +++ b/lib/cmdlib/common.py > >> @@ -1142,14 +1142,14 @@ def CheckDiskAccessModeValidity(parameters): > >> @raise errors.OpPrereqError: if the check fails. > >> > >> """ > >> - if constants.DT_RBD in parameters: > >> - access = parameters[constants.DT_RBD].get(constants.RBD_ACCESS, > >> - > constants.DISK_KERNELSPACE) > >> + for disk_template in parameters: > >> + access = parameters[disk_template].get(constants.LDP_ACCESS, > >> + constants.DISK_KERNELSPACE) > > > > The check for the disk template is dropped altogether, but the access > > parameter only makes sense for RBD and Gluster. > > > > You might want to figure out if it's OK to substitute those with > LDP_ACCESS, > > but please double-check with Helga if that's OK. > > AFAIK. LDP_* constants are only used internally when sending stuff via > RPC, > > and RBD_ACCESS (and presumably GLUSTER_ACCESS) should be used for > > user-facing stuff like OpCodes. Maybe you could simply rename RBD_ACCESS > to > > DIST_STORAGE_ACCESS or the like. > > Well, the idea is that instead of changing these files every single > time userspace access is added for a given disk template as exactly > the same parameter, we might as well just look if the parameter is > available for a generic disk template and, if so, honor that choice, > assuming kernelspace access as usual otherwise. > It would be possible to make the code generic without dropping the checks by introducing a constant which holds the disk templates which support the access parameter. This way, we wouldn't have to change the file any more neither. > > Given the current state of the codebase and the design plans around OS > installs (+mtartara), which will still require some sort of > kernelspace implementation for all disk templates afaik, assuming that > all disk templates will be kernelspace first seems reasonable to me. > Once things do change, this code can be changed to reference some > (DiskTemplate, AccessMode) map from the constants. > > Here's the current interdiff: > > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index d31d011..8669669 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -1148,7 +1148,7 @@ def CheckDiskAccessModeValidity(parameters): > 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, > + " one of {o})".format(d=disk_template, > > a=constants.LDP_ACCESS, > v=access, > o=valid_vals_str)) > diff --git a/lib/config.py b/lib/config.py > index 1d03c4b..4a9e7cb 100644 > --- a/lib/config.py > +++ b/lib/config.py > @@ -679,12 +679,12 @@ class ConfigWriter(object): > _helper_ipolicy("cluster", cluster.ipolicy, True) > > for disk_template in cluster.diskparams: > - access = cluster.diskparams[disk_template].get(constants.RBD_ACCESS, > + access = cluster.diskparams[disk_template].get(constants.LDP_ACCESS, > > constants.DISK_KERNELSPACE) > if access not in constants.DISK_VALID_ACCESS_MODES: > result.append( > "Invalid value of '%s:%s': '%s' (expected one of %s)" % ( > - disk_template, constants.RBD_ACCESS, access, > + disk_template, constants.LDP_ACCESS, access, > utils.CommaJoin(constants.DISK_VALID_ACCESS_MODES) > ) > ) > > This doesn't solve my concern about using the LDP_* constants instead of the RBD_* (or other user-facing once) here. > > -- > 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
