Hi Morg,
On 12/01, Federico Pareschi wrote:
> Thanks for the patch, this is mostly okay aside from a few small
> comments inline.
Thanks for the review! Please find my answers 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(-)
< snip >
> > @@ -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.
Oops! Good catch! I'll split this to two lines.
> Also, shouldn't both os_inst_removed and os_inst_privatE_removed be {}
> instead of []?
os_inst_removed/os_inst_private_removed just store the keys of the
public/private osparams for reporting removal to the user later on so using a
list is ok.
I'm resending this.
> > 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
> >
> >
