On Wed, Apr 3, 2013 at 2:57 PM, Helga Velroyen <[email protected]> wrote:
> This patch deprecates the option '--no-lvm-storage' of 'gnt-cluster > modify'. > Technically, it is not fully removed, but kept in order to warn the user > that it is no longer supported and that she should use > --enabled-disk-templates > instead. The consistency check between '--no-lvm-storage' and '--vg-name' > is replaced by one between '--enabled-disk-templates' and '--vg-name'. > > Signed-off-by: Helga Velroyen <[email protected]> > --- > lib/client/gnt_cluster.py | 62 > ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 43 insertions(+), 19 deletions(-) > > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py > index 01ef3ba..8c2fb6e 100644 > --- a/lib/client/gnt_cluster.py > +++ b/lib/client/gnt_cluster.py > @@ -62,6 +62,17 @@ _EPO_PING_TIMEOUT = 1 # 1 second > _EPO_REACHABLE_TIMEOUT = 15 * 60 # 15 minutes > > > +def _CheckNoLvmStorageOptDeprecated(opts): > + """Checks if the legacy option '--no-lvm-storage' is used.""" > + if not opts.lvm_storage: > + ToStderr("The option --no-lvm-storage is no longer supported. If you > want" > + " to disable lvm-based storage cluster-wide, use the option" > + " --enabled-disk-templates to disable all of these lvm-base > disk " > + " templates: %s" % > + utils.CommaJoin(utils.GetLvmDiskTemplates())) > + return 1 > + > + > @UsesRPC > def InitCluster(opts, args): > """Initialize the cluster. > @@ -74,12 +85,22 @@ def InitCluster(opts, args): > @return: the desired exit code > > """ > - if not opts.lvm_storage and opts.vg_name: > - ToStderr("Options --no-lvm-storage and --vg-name conflict.") > + if _CheckNoLvmStorageOptDeprecated(opts): > + return 1 > + enabled_disk_templates = opts.enabled_disk_templates > + if enabled_disk_templates: > + enabled_disk_templates = enabled_disk_templates.split(",") > + else: > + enabled_disk_templates = > list(constants.DEFAULT_ENABLED_DISK_TEMPLATES) > Given that you are moving this chunk of code around, could you also improve it a little bit? having enabled_disk_templates initially set to a string, and then substituted by a list, is quite ugly from the point of view of type safety. What about: - enabled_disk_templates = opts.enabled_disk_templates > + if opts.enabled_disk_templates: > + enabled_disk_templates = opts.enabled_disk_templates.split(",") > + else: > + enabled_disk_templates = > list(constants.DEFAULT_ENABLED_DISK_TEMPLATES) > It seems safer (or at least more elegant) to me. + > + if opts.vg_name and not utils.IsLvmEnabled(enabled_disk_templates): > + ToStderr("Use option --vg-name only if you are enabling at least one > of the" > + " following disk templates: %s" % > + utils.CommaJoin(utils.GetLvmDiskTemplates())) > return 1 > > vg_name = opts.vg_name > - if opts.lvm_storage and not opts.vg_name: > + if utils.IsLvmEnabled(enabled_disk_templates) and not opts.vg_name: > vg_name = constants.DEFAULT_VG > > if not opts.drbd_storage and opts.drbd_helper: > @@ -193,12 +214,6 @@ def InitCluster(opts, args): > > hv_state = dict(opts.hv_state) > > - enabled_disk_templates = opts.enabled_disk_templates > - if enabled_disk_templates: > - enabled_disk_templates = enabled_disk_templates.split(",") > - else: > - enabled_disk_templates = > list(constants.DEFAULT_ENABLED_DISK_TEMPLATES) > - > bootstrap.InitCluster(cluster_name=args[0], > secondary_ip=opts.secondary_ip, > vg_name=vg_name, > @@ -961,7 +976,7 @@ def SetClusterParams(opts, args): > @return: the desired exit code > > """ > - if not (not opts.lvm_storage or opts.vg_name or > + if not (opts.vg_name or > not opts.drbd_storage or opts.drbd_helper or > opts.enabled_hypervisors or opts.hvparams or > opts.beparams or opts.nicparams or > @@ -991,13 +1006,26 @@ def SetClusterParams(opts, args): > ToStderr("Please give at least one of the parameters.") > return 1 > > - vg_name = opts.vg_name > - if not opts.lvm_storage and opts.vg_name: > - ToStderr("Options --no-lvm-storage and --vg-name conflict.") > + if _CheckNoLvmStorageOptDeprecated(opts): > return 1 > + enabled_disk_templates = opts.enabled_disk_templates > + vg_name = opts.vg_name > + # Note that this only checks consistency between the vg_name and the > + # enabled lvm-based disk templates if both parameters --vg-name and > + # --enabled-disk-templates are used in this operation. If the option > + # --enabled-disk-templates is not explicitly used, we cannot make a > statement > + # about the consistency with the already configured enabled disk > templates > + # at this point. > + if enabled_disk_templates: > + enabled_disk_templates = enabled_disk_templates.split(",") > Similar comment here as above about the changing type. > + if not utils.IsLvmEnabled(enabled_disk_templates) and opts.vg_name: > + ToStderr("Use option --vg-name only if you are enabling at least > one of" > + " the following disk templates: %s" % > + utils.CommaJoin(utils.GetLvmDiskTemplates())) > + return 1 > > - if not opts.lvm_storage: > - vg_name = "" > + if not utils.IsLvmEnabled(enabled_disk_templates): > + vg_name = "" > > drbd_helper = opts.drbd_helper > if not opts.drbd_storage and opts.drbd_helper: > @@ -1011,10 +1039,6 @@ def SetClusterParams(opts, args): > if hvlist is not None: > hvlist = hvlist.split(",") > > - enabled_disk_templates = opts.enabled_disk_templates > - if enabled_disk_templates: > - enabled_disk_templates = enabled_disk_templates.split(",") > - > # a list of (name, dict) we can pass directly to dict() (or []) > hvparams = dict(opts.hvparams) > for hv_params in hvparams.values(): > -- > 1.8.1.3 > > Thanks, Michele
