Thanks for the patch, this is mostly okay aside from a few small
comments inline.

On 1 December 2016 at 10:34, Yiannis Tsiouris <ts...@grnet.gr> wrote:

> This patch adds two new options in 'gnt-instance modify' which allow the
> user to clear all current public/private OS parameters of an instance.
> This might be useful for OS providers that consider no parameters as
> valid or, more commonly, for changing OS provider (and parameters)
> before performing a reinstall. E.g.:
>
> $ gnt-instance modify --clear-os-parameters <instance_name>
>
> or
>
> $ gnt-instance modify --clear-os-parameters-private <instance_name>
>
> Signed-off-by: Yiannis Tsiouris <ts...@grnet.gr>
> ---
>  lib/cli_opts.py                   | 15 +++++++++++++++
>  lib/client/gnt_instance.py        |  8 +++++++-
>  lib/cmdlib/instance_set_params.py | 23 ++++++++++++++++++++++-
>  man/gnt-instance.rst              |  6 ++++++
>  src/Ganeti/OpCodes.hs             |  2 ++
>  src/Ganeti/OpParams.hs            | 12 ++++++++++++
>  test/hs/Test/Ganeti/OpCodes.hs    |  2 ++
>  7 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/lib/cli_opts.py b/lib/cli_opts.py
> index 334c961..3faf2bf 100644
> --- a/lib/cli_opts.py
> +++ b/lib/cli_opts.py
> @@ -58,6 +58,8 @@ __all__ = [
>    "CAPAB_MASTER_OPT",
>    "CAPAB_VM_OPT",
>    "CLEANUP_OPT",
> +  "CLEAR_OSPARAMS_OPT",
> +  "CLEAR_OSPARAMS_PRIVATE_OPT",
>    "cli_option",
>    "CLUSTER_DOMAIN_SECRET_OPT",
>    "COMMIT_OPT",
> @@ -729,6 +731,19 @@ OSPARAMS_SECRET_OPT = cli_option("--os-parameters-
> secret",
>                                        " saved; you must supply these for
> every"
>                                        " operation.)")
>
> +CLEAR_OSPARAMS_OPT = cli_option("--clear-os-parameters",
> +                                dest="clear_osparams",
> +                                action="store_true",
> +                                default=False,
> +                                help="Clear currnet OS parameters")
> +
> +CLEAR_OSPARAMS_PRIVATE_OPT = cli_option("--clear-os-parameters-private",
> +                                        dest="clear_osparams_private",
> +                                        action="store_true",
> +                                        default=False,
> +                                        help="Clear current private OS"
> +                                             " parameters")
> +
>  FORCE_VARIANT_OPT = cli_option("--force-variant", dest="force_variant",
>                                 action="store_true", default=False,
>                                 help="Force an unknown variant")
> diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
> index dd1013f..f6effd0 100644
> --- a/lib/client/gnt_instance.py
> +++ b/lib/client/gnt_instance.py
> @@ -1371,6 +1371,7 @@ def SetInstanceParams(opts, args):
>    """
>    if not (opts.nics or opts.disks or opts.disk_template or opts.hvparams
> or
>            opts.beparams or opts.os or opts.osparams or
> opts.osparams_private
> +          or opts.clear_osparams or opts.clear_osparams_private
>            or opts.offline_inst or opts.online_inst or opts.runtime_mem or
>            opts.new_primary_node or opts.instance_communication is not
> None):
>      ToStderr("Please give at least one of the parameters.")
> @@ -1435,6 +1436,8 @@ def SetInstanceParams(opts, args):
>
>    instance_comm = opts.instance_communication
>
> +  clear_osparams_priv = opts.clear_osparams_private
> +
>    op = opcodes.OpInstanceSetParams(instance_name=args[0],
>                                     nics=nics,
>                                     disks=disks,
> @@ -1453,6 +1456,8 @@ def SetInstanceParams(opts, args):
>                                     os_name=opts.os,
>                                     osparams=opts.osparams,
>                                     osparams_private=opts.
> osparams_private,
> +                                   clear_osparams=opts.clear_osparams,
> +                                   clear_osparams_private=clear_
> osparams_priv,
>                                     force_variant=opts.force_variant,
>                                     force=opts.force,
>                                     wait_for_sync=opts.wait_for_sync,
> @@ -1668,7 +1673,8 @@ commands = {
>       OFFLINE_INST_OPT, ONLINE_INST_OPT, IGNORE_IPOLICY_OPT,
> RUNTIME_MEM_OPT,
>       NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT, HOTPLUG_OPT,
>       HOTPLUG_IF_POSSIBLE_OPT, INSTANCE_COMMUNICATION_OPT,
> -     EXT_PARAMS_OPT, FILESTORE_DRIVER_OPT, FILESTORE_DIR_OPT],
> +     EXT_PARAMS_OPT, FILESTORE_DRIVER_OPT, FILESTORE_DIR_OPT,
> +     CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT],
>      "<instance>", "Alters the parameters of an instance"),
>    "shutdown": (
>      GenericManyOps("shutdown", _ShutdownInstance), [ArgInstance()],
> diff --git a/lib/cmdlib/instance_set_params.py b/lib/cmdlib/instance_set_
> params.py
> index 486c14e..b4553c8 100644
> --- a/lib/cmdlib/instance_set_params.py
> +++ b/lib/cmdlib/instance_set_params.py
> @@ -337,6 +337,7 @@ class LUInstanceSetParams(LogicalUnit):
>              self.op.hvparams or self.op.beparams or self.op.os_name or
>              self.op.osparams or self.op.offline is not None or
>              self.op.runtime_mem or self.op.pnode or
> self.op.osparams_private or
> +            self.op.clear_osparams or self.op.clear_osparams_private or
>              self.op.instance_communication is not None):
>        raise errors.OpPrereqError("No changes submitted",
> errors.ECODE_INVAL)
>
> @@ -991,15 +992,25 @@ class LUInstanceSetParams(LogicalUnit):
>                     if self.op.os_name and not self.op.force
>                     else self.instance.os)
>
> -    if self.op.osparams or self.op.osparams_private:
> +    if (self.op.osparams or self.op.osparams_private or
> +        self.op.clear_osparams or self.op.clear_osparams_private):
>        public_parms = self.op.osparams or {}
>        private_parms = self.op.osparams_private or {}
> +      self.os_inst_removed = self.os_inst_private_removed = []
>

This is a dangerous line, in python a = b = [] ends up pointing to the
same mutable reference. This means that changes to b are reflected on a
as well. While there's no immediate bug that I can see from this, it's
just better to initialize them on two different lines.

Also, shouldn't both os_inst_removed and os_inst_privatE_removed be {}
instead of []?


>        dupe_keys = utils.GetRepeatedKeys(public_parms, private_parms)
>
>        if dupe_keys:
>          raise errors.OpPrereqError("OS parameters repeated multiple
> times: %s" %
>                                     utils.CommaJoin(dupe_keys))
>
> +      if self.op.clear_osparams:
> +        self.os_inst_removed = self.instance.osparams
> +        self.instance.osparams = {}
> +
> +      if self.op.clear_osparams_private:
> +        self.os_inst_private_removed = self.instance.osparams_private
> +        self.instance.osparams_private = {}
> +
>        self.os_inst = GetUpdatedParams(self.instance.osparams,
>                                        public_parms)
>        self.os_inst_private = GetUpdatedParams(self.
> instance.osparams_private,
> @@ -1955,6 +1966,16 @@ class LUInstanceSetParams(LogicalUnit):
>        self.instance.os = self.op.os_name
>
>      # osparams changes
> +    if self.op.clear_osparams:
> +      self.instance.osparams = self.os_inst
> +      for osp in self.os_inst_removed:
> +        result.append(("os/%s" % osp, "<removed>"))
> +
> +    if self.op.clear_osparams_private:
> +      self.instance.osparams_private = self.os_inst_private
> +      for osp in self.os_inst_private_removed:
> +        result.append(("os_private/%s" % osp, "<removed>"))
> +
>      if self.op.osparams:
>        self.instance.osparams = self.os_inst
>        for key, val in self.op.osparams.iteritems():
> diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst
> index 283392c..8408a8f 100644
> --- a/man/gnt-instance.rst
> +++ b/man/gnt-instance.rst
> @@ -1378,6 +1378,8 @@ MODIFY
>  | [\--os-type=*OS* [\--force-variant]]
>  | [{-O|\--os-parameters} *param*=*value*... ]
>  | [--os-parameters-private *param*=*value*... ]
> +| [--clear-os-parameters]
> +| [--clear-os-parameters-private]
>  | [\--offline \| \--online]
>  | [\--submit] [\--print-jobid]
>  | [\--ignore-ipolicy]
> @@ -1395,6 +1397,10 @@ and ``-O (--os-parameters)`` options specifies
> hypervisor, backend and
>  OS parameter options in the form of name=value[,...]. For details
>  which options can be specified, see the **add** command.
>
> +The ``--clear-os-parameters`` option will clear all current (public)
> +instance OS parameters and the ``--clear-os-parameters-private`` will
> +clear all current private OS parameters.
> +
>  The ``-t (--disk-template)`` option will change the disk template of
>  the instance.  Currently, conversions between all the available
>  templates are supported, except the ``diskless`` and the ``blockdev``
> diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs
> index 4a3660e..c26a085 100644
> --- a/src/Ganeti/OpCodes.hs
> +++ b/src/Ganeti/OpCodes.hs
> @@ -720,6 +720,8 @@ $(genOpCode "OpCode"
>       , pOsNameChange
>       , pInstOsParams
>       , pInstOsParamsPrivate
> +     , pInstOsParamsClear
> +     , pInstOsParamsPrivateClear
>       , pWaitForSync
>       , withDoc "Whether to mark the instance as offline" pOffline
>       , pIpConflictsCheck
> diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs
> index e507a4a..2b4b70f 100644
> --- a/src/Ganeti/OpParams.hs
> +++ b/src/Ganeti/OpParams.hs
> @@ -139,6 +139,8 @@ module Ganeti.OpParams
>    , pInstOsParams
>    , pInstOsParamsPrivate
>    , pInstOsParamsSecret
> +  , pInstOsParamsClear
> +  , pInstOsParamsPrivateClear
>    , pCandidatePoolSize
>    , pMaxRunningJobs
>    , pMaxTrackedJobs
> @@ -1248,6 +1250,16 @@ pInstOsParamsSecret =
>    optionalField $
>    simpleField "osparams_secret" [t| JSObject (Secret JSValue) |]
>
> +pInstOsParamsClear :: Field
> +pInstOsParamsClear =
> +  withDoc "Clear current OS parameters from instance" $
> +  defaultFalse "clear_osparams"
> +
> +pInstOsParamsPrivateClear :: Field
> +pInstOsParamsPrivateClear =
> +  withDoc "Clear current private OS parameters from instance" $
> +  defaultFalse "clear_osparams_private"
> +
>  pPrimaryNode :: Field
>  pPrimaryNode =
>    withDoc "Primary node for an instance" $
> diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.
> hs
> index 405d7fe..96402c5 100644
> --- a/test/hs/Test/Ganeti/OpCodes.hs
> +++ b/test/hs/Test/Ganeti/OpCodes.hs
> @@ -431,6 +431,8 @@ genOpCodeFromId op_id =
>          <*> genMaybe genNameNE              -- os_name
>          <*> pure emptyJSObject              -- osparams
>          <*> genMaybe arbitraryPrivateJSObj  -- osparams_private
> +        <*> arbitrary                       -- clear_osparams
> +        <*> arbitrary                       -- clear_osparams_private
>          <*> arbitrary                       -- wait_for_sync
>          <*> arbitrary                       -- offline
>          <*> arbitrary                       -- conflicts_check
> --
> 2.10.2
>
>

Reply via email to