Re: [PATCH master 0/6] Clear/remove public/private OS params in gnt-instance modify/reinstall

['Federico Pareschi' via ganeti-devel]

Aside from the couple of nitpicks that I've already commented in the rest
of the patchset, this looks like a reasonable patch to me.

I haven't had the time to test it yet, but other than that it looks good to
me.

On 1 December 2016 at 10:34, Yiannis Tsiouris  wrote:

> This patchset extends 'gnt-instance modify' and 'gnt-instance reinstall'
> functionality by allowing partial/total removal of current OS parameters
> of an
> instance. This is specifically useful for preparing or performing an
> instance
> reinstall to a different OS provider or the same OS provider that changed
> its
> OS parameter policy/supported list.
>
> Nikos Skalkotos (1):
>   Add cmdlib tests for removing instance OS params
>
> Yiannis Tsiouris (5):
>   Add clear OS params options in gnt-instance modify
>   Support OS params removal in gnt-instance modify
>   Test new options of gnt-instance modify in RAPI
>   Support clear OS params in gnt-instance reinstall
>   Add OS params removal in gnt-instance reinstall
>
>  lib/cli_opts.py   | 32 +++
>  lib/client/gnt_instance.py| 44 +++--
>  lib/cmdlib/instance_operation.py  | 16 
>  lib/cmdlib/instance_set_params.py | 45 -
>  lib/rapi/rlib2.py | 16 +++-
>  man/gnt-instance.rst  | 25 
>  src/Ganeti/HTools/Repair.hs   |  8 
>  src/Ganeti/OpCodes.hs |  8 
>  src/Ganeti/OpParams.hs| 48 +++
>  test/hs/Test/Ganeti/OpCodes.hs|  7 
>  test/py/cmdlib/instance_unittest.py   | 36 +
>  test/py/ganeti.rapi.rlib2_unittest.py | 73 +-
> -
>  12 files changed, 325 insertions(+), 33 deletions(-)
>
> --
> 2.10.2
>
>

Re: [PATCH master 3/6] Add cmdlib tests for removing instance OS params

['Federico Pareschi' via ganeti-devel]

On 1 December 2016 at 10:34, Yiannis Tsiouris  wrote:

> From: Nikos Skalkotos 
>
> Add LUInstanceSetParams tests for removing individual public & private
> parameters, as well as, for clearing out public & private parameters.
>
> Signed-off-by: Nikos Skalkotos 
> Signed-off-by: Yiannis Tsiouris 
> ---
>  test/py/cmdlib/instance_unittest.py | 36 ++
> ++
>  1 file changed, 36 insertions(+)
>
> diff --git a/test/py/cmdlib/instance_unittest.py
> b/test/py/cmdlib/instance_unittest.py
> index d725015..3701805 100644
> --- a/test/py/cmdlib/instance_unittest.py
> +++ b/test/py/cmdlib/instance_unittest.py
> @@ -2237,6 +2237,42 @@ class TestLUInstanceSetParams(CmdlibTestCase):
>   })
>  self.ExecOpCode(op)
>
> +  def testRemoveOsParam(self):
> +os_param = self.os.supported_parameters[0]
> +self.inst.osparams[os_param] = "test_param_val"
> +op = self.CopyOpCode(self.op, remove_osparams=[
> +self.os.supported_parameters[0]])
> +self.ExecOpCode(op)
> +self.assertFalse(os_param in self.inst.osparams)
> +
> +  def testClearOsParams(self):
> +os = self.cfg.CreateOs(supported_parameters=["param1", "param2"])
> +inst = self.cfg.AddNewInstance(osparams={
> + os.supported_parameters[0]: "val0",
> + os.supported_parameters[1]: "val1"})
> +
> +op = self.CopyOpCode(self.op, clear_osparams=True, instance_name=
> inst.name)
> +self.ExecOpCode(op)
> +self.assertEqual(len(inst.osparams), 0)
> +
> +  def testRemovePrivateOsParam(self):
> +os_param = self.os.supported_parameters[0]
> +self.inst.osparams_private[os_param] = "test_param_val"
> +op = self.CopyOpCode(self.op, remove_osparams_private=[
> +self.os.supported_parameters[0]])


Maybe move "remove_osparams_private=..." to a newline under self.op
so it indents better and you don't need to wrap its value instead.


>

+self.ExecOpCode(op)
> +self.assertFalse(os_param in self.inst.osparams_private)
> +
> +  def testClearPrivateOsParams(self):
> +os = self.cfg.CreateOs(supported_parameters=["param1", "param2"])
> +inst = self.cfg.AddNewInstance(osparams={
> + os.supported_parameters[0]: "val0",
> + os.supported_parameters[1]: "val1"})
> +
> +op = self.CopyOpCode(self.op, clear_osparams=True, instance_name=
> inst.name)
> +self.ExecOpCode(op)
> +self.assertEqual(len(inst.osparams), 0)
> +
>def testIncreaseMemoryTooMuch(self):
>  op = self.CopyOpCode(self.running_op,
>   beparams={
> --
> 2.10.2
>
>

Re: [PATCH master 1/6] Add clear OS params options in gnt-instance modify

['Federico Pareschi' via ganeti-devel]

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

On 1 December 2016 at 10:34, Yiannis Tsiouris  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 
>
> or
>
> $ gnt-instance modify --clear-os-parameters-private 
>
> Signed-off-by: Yiannis Tsiouris 
> ---
>  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],
>  "", "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
> +++ 

Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Iustin Pop' via ganeti-devel]

On Thu, Dec 01, 2016 at 04:09:56AM -0800, Ganeti Development List wrote:
> 
> On Thursday, December 1, 2016 at 11:53:43 AM UTC, Iustin Pop wrote:
> >
> > On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote: 
> > > Following issue #1194, this patch allows Ganeti to correctly 
> > > parse drbd versions that also include a dash in their k-fix 
> > > version component. 
> >
> > This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the 
> > correct behaviour? 
> >
> 
> Yes, you are correct. I'm not sure to be honest. I've been looking around 
> the drbd and 
> also the debian packaging site but I'm not 100% sure what logic follows 
> their 'k-fix' 
> numbering scheme. In drbd, from what I can see, they always use x.y.z-k 
> except 
> in one particular (old) version where they do x.y.z.k. 
> 
> Honestly, though, as somebody correctly pointed out on the ganeti mailing 
> list, does it
> really matter? Shouldn't we mostly only care about major and minor version 
> as that 
> should be what might break compatibility? All in all I'd say we can treat 
> these as 
> interchangeable but if somebody disagrees or knows more about this, I'd be 
> happy to 
> fix it.


No, it doesn't really matter, I was just asking to confirm this is the
plan. I'd suggest updating the commit message to record this explicit
decision (the current one will not be very clear in 2 years whether it
was intended or not).

iustin

Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Federico Pareschi' via ganeti-devel]

On Thursday, December 1, 2016 at 11:53:43 AM UTC, Iustin Pop wrote:
>
> On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote: 
> > Following issue #1194, this patch allows Ganeti to correctly 
> > parse drbd versions that also include a dash in their k-fix 
> > version component. 
>
> This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the 
> correct behaviour? 
>

Yes, you are correct. I'm not sure to be honest. I've been looking around 
the drbd and 
also the debian packaging site but I'm not 100% sure what logic follows 
their 'k-fix' 
numbering scheme. In drbd, from what I can see, they always use x.y.z-k 
except 
in one particular (old) version where they do x.y.z.k. 

Honestly, though, as somebody correctly pointed out on the ganeti mailing 
list, does it
really matter? Shouldn't we mostly only care about major and minor version 
as that 
should be what might break compatibility? All in all I'd say we can treat 
these as 
interchangeable but if somebody disagrees or knows more about this, I'd be 
happy to 
fix it.
 

>
> > Signed-off-by: Federico Morg Pareschi  
> > --- 
> >  lib/storage/drbd_info.py| 17 +++-- 
> >  test/py/ganeti.storage.drbd_unittest.py | 10 ++ 
> >  2 files changed, 25 insertions(+), 2 deletions(-) 
> > 
> > diff --git a/lib/storage/drbd_info.py b/lib/storage/drbd_info.py 
> > index 99605f1..469ed7f 100644 
> > --- a/lib/storage/drbd_info.py 
> > +++ b/lib/storage/drbd_info.py 
> > @@ -164,13 +164,15 @@ class DRBD8Info(object): 
> >   
> >""" 
> >   
> > -  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.(\d+))?" 
> > +  _VERSION_RE = re.compile(r"^version: 
> (\d+)\.(\d+)\.(\d+)(?:[.-](\d+))?" 
> > r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)") 
> >_VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$") 
> > +  _K_FIX_DASH_SEPARATOR_RE = re.compile(r"^version: 
> (\d+)\.(\d+)\.(\d+)(?:-)") 
> >   
> > +  def _GetKFixSeparator(self, lines): 
> > +"""Check, in case of a K-fix version, if the separator is a dash or 
> dot.""" 
> > + 
> > +first_line = lines[0].strip() 
> > +match = self._K_FIX_DASH_SEPARATOR_RE.match(first_line) 
> > +if match is None: 
> > +  return "." 
> > +else: 
> > +  return "-" 
>
> This seems to be done in two steps. Would it be simpler to have 
> K_FIX_DASH_SEPARATOR itself extract the separator, instead of it having 
> to match - and if not, return . vs -? 
>
> You could even get rid of _K_FIX_DASH_SEPARATOR_RE, and extract the 
> separator from the _VERSION_RE, after changing the RE to have the 
> separator a capturing group. 
>

I was trying to get as little regex pain as possible but what you're saying 
actually
makes more sense, I'll see what I can do with capture groups and fix the 
code 
accordingly, thanks.

Morg.

Re: [PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Iustin Pop' via ganeti-devel]

On Thu, Dec 01, 2016 at 11:38:35AM +, Ganeti Development List wrote:
> Following issue #1194, this patch allows Ganeti to correctly
> parse drbd versions that also include a dash in their k-fix
> version component.

This means 8.4.8-1 and 8.4.8.1 will be treated the same. Is this the
correct behaviour?

> Signed-off-by: Federico Morg Pareschi 
> ---
>  lib/storage/drbd_info.py| 17 +++--
>  test/py/ganeti.storage.drbd_unittest.py | 10 ++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/storage/drbd_info.py b/lib/storage/drbd_info.py
> index 99605f1..469ed7f 100644
> --- a/lib/storage/drbd_info.py
> +++ b/lib/storage/drbd_info.py
> @@ -164,13 +164,15 @@ class DRBD8Info(object):
>  
>"""
>  
> -  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.(\d+))?"
> +  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:[.-](\d+))?"
> r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)")
>_VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$")
> +  _K_FIX_DASH_SEPARATOR_RE = re.compile(r"^version: 
> (\d+)\.(\d+)\.(\d+)(?:-)")
>  
> +  def _GetKFixSeparator(self, lines):
> +"""Check, in case of a K-fix version, if the separator is a dash or 
> dot."""
> +
> +first_line = lines[0].strip()
> +match = self._K_FIX_DASH_SEPARATOR_RE.match(first_line)
> +if match is None:
> +  return "."
> +else:
> +  return "-"

This seems to be done in two steps. Would it be simpler to have
K_FIX_DASH_SEPARATOR itself extract the separator, instead of it having
to match - and if not, return . vs -?

You could even get rid of _K_FIX_DASH_SEPARATOR_RE, and extract the
separator from the _VERSION_RE, after changing the RE to have the
separator a capturing group.

thanks,
iustin

[PATCH stable-2.15] Fix for incorrect parsing of DRBD versions

['Federico Morg Pareschi' via ganeti-devel]

Following issue #1194, this patch allows Ganeti to correctly
parse drbd versions that also include a dash in their k-fix
version component.

Signed-off-by: Federico Morg Pareschi 
---
 lib/storage/drbd_info.py| 17 +++--
 test/py/ganeti.storage.drbd_unittest.py | 10 ++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/lib/storage/drbd_info.py b/lib/storage/drbd_info.py
index 99605f1..469ed7f 100644
--- a/lib/storage/drbd_info.py
+++ b/lib/storage/drbd_info.py
@@ -164,13 +164,15 @@ class DRBD8Info(object):
 
   """
 
-  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:\.(\d+))?"
+  _VERSION_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:[.-](\d+))?"
r" \(api:(\d+)/proto:(\d+)(?:-(\d+))?\)")
   _VALID_LINE_RE = re.compile("^ *([0-9]+): cs:([^ ]+).*$")
+  _K_FIX_DASH_SEPARATOR_RE = re.compile(r"^version: (\d+)\.(\d+)\.(\d+)(?:-)")
 
   def __init__(self, lines):
 self._version = self._ParseVersion(lines)
 self._minors, self._line_per_minor = self._JoinLinesPerMinor(lines)
+self._k_fix_separator = self._GetKFixSeparator(lines)
 
   def GetVersion(self):
 """Return the DRBD version.
@@ -195,7 +197,7 @@ class DRBD8Info(object):
 retval = "%d.%d.%d" % \
  (version["k_major"], version["k_minor"], version["k_point"])
 if "k_fix" in version:
-  retval += ".%s" % version["k_fix"]
+  retval += "%s%s" % (self._k_fix_separator, version["k_fix"])
 
 retval += " (api:%d/proto:%d" % (version["api"], version["proto"])
 if "proto2" in version:
@@ -240,6 +242,17 @@ class DRBD8Info(object):
 
 return retval
 
+  def _GetKFixSeparator(self, lines):
+"""Check, in case of a K-fix version, if the separator is a dash or dot."""
+
+first_line = lines[0].strip()
+match = self._K_FIX_DASH_SEPARATOR_RE.match(first_line)
+if match is None:
+  return "."
+else:
+  return "-"
+
+
   def _JoinLinesPerMinor(self, lines):
 """Transform the raw lines into a dictionary based on the minor.
 
diff --git a/test/py/ganeti.storage.drbd_unittest.py 
b/test/py/ganeti.storage.drbd_unittest.py
index 9a1894f..3ffdb74 100755
--- a/test/py/ganeti.storage.drbd_unittest.py
+++ b/test/py/ganeti.storage.drbd_unittest.py
@@ -50,6 +50,7 @@ class TestDRBD8(testutils.GanetiTestCase):
   "version: 8.0.12 (api:76/proto:86-91)",
   "version: 8.2.7 (api:88/proto:0-100)",
   "version: 8.3.7.49 (api:188/proto:13-191)",
+  "version: 8.4.8-1 (api:1/proto:86-101)",
 ]
 result = [
   {
@@ -83,6 +84,15 @@ class TestDRBD8(testutils.GanetiTestCase):
 "api": 188,
 "proto": 13,
 "proto2": "191",
+  },
+  {
+"k_major": 8,
+"k_minor": 4,
+"k_point": 8,
+"k_fix": "1",
+"api": 1,
+"proto": 86,
+"proto2": "101",
   }
 ]
 for d, r in zip(data, result):
-- 
2.8.0.rc3.226.g39d4020

[PATCH master 0/6] Clear/remove public/private OS params in gnt-instance modify/reinstall

[Yiannis Tsiouris]

This patchset extends 'gnt-instance modify' and 'gnt-instance reinstall'
functionality by allowing partial/total removal of current OS parameters of an
instance. This is specifically useful for preparing or performing an instance
reinstall to a different OS provider or the same OS provider that changed its
OS parameter policy/supported list.

Nikos Skalkotos (1):
  Add cmdlib tests for removing instance OS params

Yiannis Tsiouris (5):
  Add clear OS params options in gnt-instance modify
  Support OS params removal in gnt-instance modify
  Test new options of gnt-instance modify in RAPI
  Support clear OS params in gnt-instance reinstall
  Add OS params removal in gnt-instance reinstall

 lib/cli_opts.py   | 32 +++
 lib/client/gnt_instance.py| 44 +++--
 lib/cmdlib/instance_operation.py  | 16 
 lib/cmdlib/instance_set_params.py | 45 -
 lib/rapi/rlib2.py | 16 +++-
 man/gnt-instance.rst  | 25 
 src/Ganeti/HTools/Repair.hs   |  8 
 src/Ganeti/OpCodes.hs |  8 
 src/Ganeti/OpParams.hs| 48 +++
 test/hs/Test/Ganeti/OpCodes.hs|  7 
 test/py/cmdlib/instance_unittest.py   | 36 +
 test/py/ganeti.rapi.rlib2_unittest.py | 73 +--
 12 files changed, 325 insertions(+), 33 deletions(-)

-- 
2.10.2

[PATCH master 2/6] Support OS params removal in gnt-instance modify

[Yiannis Tsiouris]

This patch extends 'gnt-instance modify' by allowing a user to remove a
list of public/private OS parameters from an instance. This can be
useful before performing a reinstall to a new OS provider. Example
usage:

$ gnt-instance modify --remove-os-parameters parm1,parm2 

or

$ gnt-instance modify --remove-os-parameters-private parm3 

Signed-off-by: Yiannis Tsiouris 
---
 lib/cli_opts.py   | 17 +
 lib/client/gnt_instance.py| 16 +++-
 lib/cmdlib/instance_set_params.py | 28 +---
 man/gnt-instance.rst  |  7 ++-
 src/Ganeti/OpCodes.hs |  2 ++
 src/Ganeti/OpParams.hs| 12 
 test/hs/Test/Ganeti/OpCodes.hs|  2 ++
 7 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/lib/cli_opts.py b/lib/cli_opts.py
index 3faf2bf..1624910 100644
--- a/lib/cli_opts.py
+++ b/lib/cli_opts.py
@@ -221,6 +221,8 @@ __all__ = [
   "REASON_OPT",
   "REBOOT_TYPE_OPT",
   "REMOVE_INSTANCE_OPT",
+  "REMOVE_OSPARAMS_OPT",
+  "REMOVE_OSPARAMS_PRIVATE_OPT",
   "REMOVE_RESERVED_IPS_OPT",
   "REMOVE_UIDS_OPT",
   "RESERVED_LVS_OPT",
@@ -744,6 +746,21 @@ CLEAR_OSPARAMS_PRIVATE_OPT = 
cli_option("--clear-os-parameters-private",
 help="Clear current private OS"
  " parameters")
 
+REMOVE_OSPARAMS_OPT = cli_option("--remove-os-parameters",
+ dest="remove_osparams",
+ type="list",
+ default=None,
+ help="Comma-separated list of OS parameters"
+  " that should be removed")
+
+REMOVE_OSPARAMS_PRIVATE_OPT = cli_option("--remove-os-parameters-private",
+ dest="remove_osparams_private",
+ type="list",
+ default=None,
+ help="Comma-separated list of private"
+  " OS parameters that should be"
+  " removed")
+
 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 f6effd0..bc4482b 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -1372,6 +1372,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.remove_osparams or opts.remove_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.")
@@ -1436,7 +1437,17 @@ def SetInstanceParams(opts, args):
 
   instance_comm = opts.instance_communication
 
+  if opts.clear_osparams and opts.remove_osparams is not None:
+raise errors.OpPrereqError("Using --remove-os-parameters with "
+  "--clear-os-parameters is not possible", errors.ECODE_INVAL)
+
+  if opts.clear_osparams_private and opts.remove_osparams_private is not None:
+raise errors.OpPrereqError("Using --remove-os-parameters-private with "
+  "--clear-os-parameters-private is not possible", errors.ECODE_INVAL)
+
   clear_osparams_priv = opts.clear_osparams_private
+  remove_osparams = opts.remove_osparams or []
+  remove_osps_priv = opts.remove_osparams_private or []
 
   op = opcodes.OpInstanceSetParams(instance_name=args[0],
nics=nics,
@@ -1458,6 +1469,8 @@ def SetInstanceParams(opts, args):
osparams_private=opts.osparams_private,
clear_osparams=opts.clear_osparams,
clear_osparams_private=clear_osparams_priv,
+   remove_osparams=remove_osparams,
+   remove_osparams_private=remove_osps_priv,
force_variant=opts.force_variant,
force=opts.force,
wait_for_sync=opts.wait_for_sync,
@@ -1674,7 +1687,8 @@ commands = {
  NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT, HOTPLUG_OPT,
  HOTPLUG_IF_POSSIBLE_OPT, INSTANCE_COMMUNICATION_OPT,
  EXT_PARAMS_OPT, FILESTORE_DRIVER_OPT, FILESTORE_DIR_OPT,
- CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT],
+ CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT,
+ REMOVE_OSPARAMS_OPT, REMOVE_OSPARAMS_PRIVATE_OPT],
 "", 

[PATCH master 1/6] Add clear OS params options in gnt-instance modify

[Yiannis Tsiouris]

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 

or

$ gnt-instance modify --clear-os-parameters-private 

Signed-off-by: Yiannis Tsiouris 
---
 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],
 "", "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
  

[PATCH master 6/6] Add OS params removal in gnt-instance reinstall

[Yiannis Tsiouris]

This patch adds '--remove-os-parameters' and
'--remove-os-parameters-private' options to 'gnt-instance reinstall'.
Similarly to 'gnt-instance modify', the new options can be used to
perform reinstalls to OS providers that support different parameters.
E.g.:

$ gnt-instance reinstall --remove-os-parameters param1,param2 \
  -o  -O param3=val3 

or

$ gnt-instance reinstall --remove-os-parameters param1,param2 \
  -o  -O param3=val3 

Signed-off-by: Yiannis Tsiouris 
---
 lib/client/gnt_instance.py   | 17 +++--
 lib/cmdlib/instance_operation.py | 10 ++
 lib/rapi/rlib2.py|  9 -
 man/gnt-instance.rst |  9 -
 src/Ganeti/HTools/Repair.hs  |  4 
 src/Ganeti/OpCodes.hs|  2 ++
 src/Ganeti/OpParams.hs   | 12 
 test/hs/Test/Ganeti/OpCodes.hs   |  2 ++
 8 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
index 17ac709..6fb21fd 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -398,7 +398,17 @@ def ReinstallInstance(opts, args):
   if not AskUser(usertext):
 return 1
 
+  if opts.clear_osparams and opts.remove_osparams is not None:
+raise errors.OpPrereqError("Using --remove-os-parameters with "
+  "--clear-os-parameters is not possible", errors.ECODE_INVAL)
+
+  if opts.clear_osparams_private and opts.remove_osparams_private is not None:
+raise errors.OpPrereqError("Using --remove-os-parameters-private with "
+  "--clear-os-parameters-private is not possible", errors.ECODE_INVAL)
+
   clear_osparams_priv = opts.clear_osparams_private
+  remove_osparams = opts.remove_osparams or []
+  remove_osps_priv = opts.remove_osparams_private or []
 
   jex = JobExecutor(verbose=multi_on, opts=opts)
   for instance_name in inames:
@@ -409,7 +419,9 @@ def ReinstallInstance(opts, args):
  osparams_private=opts.osparams_private,
  osparams_secret=opts.osparams_secret,
  clear_osparams=opts.clear_osparams,
- 
clear_osparams_private=clear_osparams_priv),
+ 
clear_osparams_private=clear_osparams_priv,
+ remove_osparams=remove_osparams,
+ remove_osparams_private=remove_osps_priv)
 jex.QueueJob(instance_name, op)
 
   results = jex.WaitOrShow(not opts.submit_only)
@@ -1662,7 +1674,8 @@ commands = {
  m_pri_node_tags_opt, m_sec_node_tags_opt, m_inst_tags_opt, SELECT_OS_OPT]
 + SUBMIT_OPTS + [DRY_RUN_OPT, PRIORITY_OPT, OSPARAMS_OPT,
  OSPARAMS_PRIVATE_OPT, OSPARAMS_SECRET_OPT,
- CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT],
+ CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT,
+ REMOVE_OSPARAMS_OPT, REMOVE_OSPARAMS_PRIVATE_OPT],
 "[-f] ", "Reinstall a stopped instance"),
   "remove": (
 RemoveInstance, ARGS_ONE_INSTANCE,
diff --git a/lib/cmdlib/instance_operation.py b/lib/cmdlib/instance_operation.py
index e784a1a..ef56545 100644
--- a/lib/cmdlib/instance_operation.py
+++ b/lib/cmdlib/instance_operation.py
@@ -370,6 +370,8 @@ class LUInstanceReinstall(LogicalUnit):
 self.op.osparams = self.op.osparams or {}
 self.op.osparams_private = self.op.osparams_private or {}
 self.op.osparams_secret = self.op.osparams_secret or {}
+self.op.remove_osparams = self.op.remove_osparams or []
+self.op.remove_osparams_private = self.op.remove_osparams_private or []
 
 # Handle the use of 'default' values.
 if self.op.clear_osparams:
@@ -383,6 +385,14 @@ class LUInstanceReinstall(LogicalUnit):
 self.op.osparams_private)
 params_secret = self.op.osparams_secret
 
+for osp in self.op.remove_osparams:
+  if osp in params_public:
+del params_public[osp]
+
+for osp in self.op.remove_osparams_private:
+  if osp in params_private:
+del params_private[osp]
+
 # Handle OS parameters
 if self.op.os_type is not None:
   instance_os = self.op.os_type
diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
index 92156d1..4f76d44 100644
--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -1307,13 +1307,20 @@ def _ParseInstanceReinstallRequest(name, data):
   clear_osparams_private = baserlib.CheckParameter(data,
"clear_osparams_private",
default=False)
+  remove_osparams = baserlib.CheckParameter(data, "remove_osparams",
+default=None)
+  remove_osparams_priv = baserlib.CheckParameter(data,
+ "remove_osparams_private",
+ default=None)
 
 

[PATCH master 3/6] Add cmdlib tests for removing instance OS params

[Yiannis Tsiouris]

From: Nikos Skalkotos 

Add LUInstanceSetParams tests for removing individual public & private
parameters, as well as, for clearing out public & private parameters.

Signed-off-by: Nikos Skalkotos 
Signed-off-by: Yiannis Tsiouris 
---
 test/py/cmdlib/instance_unittest.py | 36 
 1 file changed, 36 insertions(+)

diff --git a/test/py/cmdlib/instance_unittest.py 
b/test/py/cmdlib/instance_unittest.py
index d725015..3701805 100644
--- a/test/py/cmdlib/instance_unittest.py
+++ b/test/py/cmdlib/instance_unittest.py
@@ -2237,6 +2237,42 @@ class TestLUInstanceSetParams(CmdlibTestCase):
  })
 self.ExecOpCode(op)
 
+  def testRemoveOsParam(self):
+os_param = self.os.supported_parameters[0]
+self.inst.osparams[os_param] = "test_param_val"
+op = self.CopyOpCode(self.op, remove_osparams=[
+self.os.supported_parameters[0]])
+self.ExecOpCode(op)
+self.assertFalse(os_param in self.inst.osparams)
+
+  def testClearOsParams(self):
+os = self.cfg.CreateOs(supported_parameters=["param1", "param2"])
+inst = self.cfg.AddNewInstance(osparams={
+ os.supported_parameters[0]: "val0",
+ os.supported_parameters[1]: "val1"})
+
+op = self.CopyOpCode(self.op, clear_osparams=True, instance_name=inst.name)
+self.ExecOpCode(op)
+self.assertEqual(len(inst.osparams), 0)
+
+  def testRemovePrivateOsParam(self):
+os_param = self.os.supported_parameters[0]
+self.inst.osparams_private[os_param] = "test_param_val"
+op = self.CopyOpCode(self.op, remove_osparams_private=[
+self.os.supported_parameters[0]])
+self.ExecOpCode(op)
+self.assertFalse(os_param in self.inst.osparams_private)
+
+  def testClearPrivateOsParams(self):
+os = self.cfg.CreateOs(supported_parameters=["param1", "param2"])
+inst = self.cfg.AddNewInstance(osparams={
+ os.supported_parameters[0]: "val0",
+ os.supported_parameters[1]: "val1"})
+
+op = self.CopyOpCode(self.op, clear_osparams=True, instance_name=inst.name)
+self.ExecOpCode(op)
+self.assertEqual(len(inst.osparams), 0)
+
   def testIncreaseMemoryTooMuch(self):
 op = self.CopyOpCode(self.running_op,
  beparams={
-- 
2.10.2

[PATCH master 5/6] Support clear OS params in gnt-instance reinstall

[Yiannis Tsiouris]

This extends 'gnt-instance reinstall' to support
'--clear-os-parameters' and '--clear-os-parameters-private' options from
'gnt-instance modify'. E.g.:

$ gnt-instance reinstall --clear-os-parameters -o  \
  -O param1=val1,param2=val2 

or

$ gnt-instance reinstall --clear-os-parameters-private \
  -o  -O param1=val1,param2=val2 

Signed-off-by: Yiannis Tsiouris 
---
 lib/client/gnt_instance.py   |  9 +++--
 lib/cmdlib/instance_operation.py |  6 ++
 lib/rapi/rlib2.py|  9 -
 man/gnt-instance.rst |  7 +++
 src/Ganeti/HTools/Repair.hs  |  4 
 src/Ganeti/OpCodes.hs|  2 ++
 src/Ganeti/OpParams.hs   | 12 
 test/hs/Test/Ganeti/OpCodes.hs   |  1 +
 8 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
index bc4482b..17ac709 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -398,6 +398,8 @@ def ReinstallInstance(opts, args):
   if not AskUser(usertext):
 return 1
 
+  clear_osparams_priv = opts.clear_osparams_private
+
   jex = JobExecutor(verbose=multi_on, opts=opts)
   for instance_name in inames:
 op = opcodes.OpInstanceReinstall(instance_name=instance_name,
@@ -405,7 +407,9 @@ def ReinstallInstance(opts, args):
  force_variant=opts.force_variant,
  osparams=opts.osparams,
  osparams_private=opts.osparams_private,
- osparams_secret=opts.osparams_secret)
+ osparams_secret=opts.osparams_secret,
+ clear_osparams=opts.clear_osparams,
+ 
clear_osparams_private=clear_osparams_priv),
 jex.QueueJob(instance_name, op)
 
   results = jex.WaitOrShow(not opts.submit_only)
@@ -1657,7 +1661,8 @@ commands = {
  m_pri_node_opt, m_sec_node_opt, m_clust_opt, m_inst_opt, m_node_tags_opt,
  m_pri_node_tags_opt, m_sec_node_tags_opt, m_inst_tags_opt, SELECT_OS_OPT]
 + SUBMIT_OPTS + [DRY_RUN_OPT, PRIORITY_OPT, OSPARAMS_OPT,
- OSPARAMS_PRIVATE_OPT, OSPARAMS_SECRET_OPT],
+ OSPARAMS_PRIVATE_OPT, OSPARAMS_SECRET_OPT,
+ CLEAR_OSPARAMS_OPT, CLEAR_OSPARAMS_PRIVATE_OPT],
 "[-f] ", "Reinstall a stopped instance"),
   "remove": (
 RemoveInstance, ARGS_ONE_INSTANCE,
diff --git a/lib/cmdlib/instance_operation.py b/lib/cmdlib/instance_operation.py
index 048b1e4..e784a1a 100644
--- a/lib/cmdlib/instance_operation.py
+++ b/lib/cmdlib/instance_operation.py
@@ -372,6 +372,12 @@ class LUInstanceReinstall(LogicalUnit):
 self.op.osparams_secret = self.op.osparams_secret or {}
 
 # Handle the use of 'default' values.
+if self.op.clear_osparams:
+  instance.osparams.clear()
+
+if self.op.clear_osparams_private:
+  instance.osparams_private.clear()
+
 params_public = GetUpdatedParams(instance.osparams, self.op.osparams)
 params_private = GetUpdatedParams(instance.osparams_private,
 self.op.osparams_private)
diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
index 7b14e81..92156d1 100644
--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -1302,11 +1302,18 @@ def _ParseInstanceReinstallRequest(name, data):
   start = baserlib.CheckParameter(data, "start", exptype=bool,
   default=True)
   osparams = baserlib.CheckParameter(data, "osparams", default=None)
+  clear_osparams = baserlib.CheckParameter(data, "clear_osparams",
+   default=False)
+  clear_osparams_private = baserlib.CheckParameter(data,
+   "clear_osparams_private",
+   default=False)
 
   ops = [
 opcodes.OpInstanceShutdown(instance_name=name),
 opcodes.OpInstanceReinstall(instance_name=name, os_type=ostype,
-osparams=osparams),
+osparams=osparams,
+clear_osparams=clear_osparams,
+clear_osparams_private=clear_osparams_private),
 ]
 
   if start:
diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst
index f1e2640..b40ed17 100644
--- a/man/gnt-instance.rst
+++ b/man/gnt-instance.rst
@@ -1526,6 +1526,8 @@ REINSTALL
 | [{-O|\--os-parameters} *OS\_PARAMETERS*]
 | [--os-parameters-private} *OS\_PARAMETERS*]
 | [--os-parameters-secret} *OS\_PARAMETERS*]
+| [--clear-os-parameters]
+| [--clear-os-parameters-private]
 | [\--submit] [\--print-jobid]
 | {*instance*...}
 
@@ -1539,6 +1541,11 @@ available OS templates. OS parameters can be overridden 
using ``-O
 (--os-parameters)`` (more documentation for this option under the
 **add** command).
 
+The ``--clear-os-parameters`` option will 

[PATCH master 4/6] Test new options of gnt-instance modify in RAPI

[Yiannis Tsiouris]

This patch adds tests to RAPI for gnt-instance modify options:
- '--clear-os-parameters'/'--clear-os-parameters-private'
- '--remove-os-parameters'/'--remove-os-parameters-private'

Signed-off-by: Yiannis Tsiouris 
---
 test/py/ganeti.rapi.rlib2_unittest.py | 73 +--
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/test/py/ganeti.rapi.rlib2_unittest.py 
b/test/py/ganeti.rapi.rlib2_unittest.py
index fe67538..1f52ec1 100755
--- a/test/py/ganeti.rapi.rlib2_unittest.py
+++ b/test/py/ganeti.rapi.rlib2_unittest.py
@@ -1077,33 +1077,48 @@ class TestParseModifyInstanceRequest(RAPITestCase):
 for nics in [[], [(0, { constants.INIC_IP: "192.0.2.1", })]]:
   for disks in test_disks:
 for disk_template in constants.DISK_TEMPLATES:
-  data = {
-"osparams": osparams,
-"hvparams": hvparams,
-"beparams": beparams,
-"nics": nics,
-"disks": disks,
-"force": force,
-"disk_template": disk_template,
-}
-
-  op = self.getSubmittedOpcode(
-rlib2.R_2_instances_name_modify, [name], {}, data, "PUT",
-opcodes.OpInstanceSetParams
-  )
-
-  self.assertEqual(op.instance_name, name)
-  self.assertEqual(op.hvparams, hvparams)
-  self.assertEqual(op.beparams, beparams)
-  self.assertEqual(op.osparams, osparams)
-  self.assertEqual(op.force, force)
-  self.assertEqual(op.nics, nics)
-  self.assertEqual(op.disks, disks)
-  self.assertEqual(op.disk_template, disk_template)
-  self.assertTrue(op.remote_node is None)
-  self.assertTrue(op.os_name is None)
-  self.assertFalse(op.force_variant)
-  self.assertFalse(op.dry_run)
+  for clear_osparams in [False, True]:
+for clear_osparams_private in [False, True]:
+  for remove_osparams in [[], ["osp1", "osp2"]]:
+for remove_osparams_private in [[], ["priv_osp1",
+ "priv_osp2"]]:
+  data = {
+"osparams": osparams,
+"hvparams": hvparams,
+"beparams": beparams,
+"nics": nics,
+"disks": disks,
+"force": force,
+"disk_template": disk_template,
+"clear_osparams": clear_osparams,
+"clear_osparams_private": clear_osparams_private,
+"remove_osparams": remove_osparams,
+"remove_osparams_private": remove_osparams_private,
+  }
+
+  op = self.getSubmittedOpcode(
+rlib2.R_2_instances_name_modify, [name], {}, data,
+"PUT", opcodes.OpInstanceSetParams
+  )
+
+  self.assertEqual(op.instance_name, name)
+  self.assertEqual(op.hvparams, hvparams)
+  self.assertEqual(op.beparams, beparams)
+  self.assertEqual(op.osparams, osparams)
+  self.assertEqual(op.force, force)
+  self.assertEqual(op.nics, nics)
+  self.assertEqual(op.disks, disks)
+  self.assertEqual(op.disk_template, disk_template)
+  self.assertEqual(op.clear_osparams, clear_osparams)
+  self.assertEqual(op.clear_osparams_private,
+   clear_osparams_private)
+  self.assertEqual(op.remove_osparams, remove_osparams)
+  self.assertEqual(op.remove_osparams_private,
+   remove_osparams_private)
+  self.assertTrue(op.remote_node is None)
+  self.assertTrue(op.os_name is None)
+  self.assertFalse(op.force_variant)
+  self.assertFalse(op.dry_run)
 
   def testDefaults(self):
 name = "instir8aish31"
@@ -1112,7 +1127,9 @@ class TestParseModifyInstanceRequest(RAPITestCase):
  {}, "PUT", opcodes.OpInstanceSetParams)
 
 for i in ["hvparams", "beparams", "osparams", "force", "nics", "disks",
-  "disk_template", "remote_node", "os_name", "force_variant"]:
+  "disk_template", "remote_node", "os_name", "force_variant",
+