[PATCH master] Check ispecs against ipolicy on instance modify

[Helga Velroyen]

When modifying an instance, so far the specs were not checked against
the ipolicy. This patch fixes this issue.

Note that for backend parameters which have a minimum and a maximum
value (currently only memory), it checks both limits against the
ipolicy.

Signed-off-by: Helga Velroyen <hel...@google.com>
---
lib/cmdlib.py | 42 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 18c54e3..96ea827 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -12674,6 +12674,8 @@ class LUInstanceSetParams(LogicalUnit):
self.needed_locks[locking.LEVEL_NODE] = []
self.needed_locks[locking.LEVEL_NODE_RES] = []
self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
+ # Look node group to look up the ipolicy
+ self.share_locks[locking.LEVEL_NODEGROUP] = 1

def DeclareLocks(self, level):
# TODO: Acquire group lock in shared mode (disk parameters)
@@ -12799,6 +12801,11 @@ class LUInstanceSetParams(LogicalUnit):
pnode_info = self.cfg.GetNodeInfo(pnode)
self.diskparams = self.cfg.GetInstanceDiskParams(instance)

+ # dictionaries with minimum/maximum instance information after the
+ # modification
+ ispec_max = {}
+ ispec_min = {}
+
# Prepare disk/NIC modifications
self.diskmod = PrepareContainerMods(self.op.disks, None)
self.nicmod = PrepareContainerMods(self.op.nics, _InstNicModPrivate)
@@ -12883,6 +12890,18 @@ class LUInstanceSetParams(LogicalUnit):
else:
self.be_new = self.be_inst = {}
self.be_proposed = cluster.SimpleFillBE(instance.beparams)
+
+ # Fill the ispec with be params to check against the ipolicy
+ spindle_use = self.be_new.get(constants.BE_SPINDLE_USE, None)
+ ispec_max[constants.ISPEC_SPINDLE_USE] = \
+ ispec_min[constants.ISPEC_SPINDLE_USE] = spindle_use
+ ispec_max[constants.ISPEC_CPU_COUNT] = \
+ ispec_min[constants.ISPEC_CPU_COUNT] = \
+ self.be_new.get(constants.BE_VCPUS, None)
+ ispec_max[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MAXMEM, None)
+ ispec_min[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MINMEM, None)
be_old = cluster.FillBE(instance)

# CPU param validation -- checking every time a parameter is
@@ -13039,6 +13058,14 @@ class LUInstanceSetParams(LogicalUnit):
raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
" more" % constants.MAX_DISKS,
errors.ECODE_STATE)
+ ispec_max[constants.ISPEC_DISK_COUNT] = \
+ ispec_min[constants.ISPEC_DISK_COUNT] = len(disks)
+ # Retrieve the disk size from the old disks given of type
+ # ganeti.objects.Disk and of the new ones specified as dict
+ disk_sizes = [disk["size"] if type(disk) is dict else
+ disk.size for disk in disks]
+ ispec_max[constants.ISPEC_DISK_SIZE] = \
+ ispec_min[constants.ISPEC_DISK_SIZE] = disk_sizes

if self.op.offline is not None:
if self.op.offline:
@@ -13055,8 +13082,23 @@ class LUInstanceSetParams(LogicalUnit):
ApplyContainerMods("NIC", nics, self._nic_chgdesc, self.nicmod,
self._CreateNewNic, self._ApplyNicMods, None)
self._new_nics = nics
+ ispec_max[constants.ISPEC_NIC_COUNT] = \
+ ispec_min[constants.ISPEC_NIC_COUNT] = len(self._new_nics)
else:
self._new_nics = None
+ ispec_max[constants.ISPEC_NIC_COUNT] = \
+ ispec_min[constants.ISPEC_NIC_COUNT] = len(instance.nics)
+
+ pnode = self.cfg.GetNodeInfo(instance.primary_node)
+ group_info = self.cfg.GetNodeGroup(pnode.group)
+ ipolicy = ganeti.masterd.instance.CalculateGroupIPolicy(cluster, group_info)
+ res_max = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_max)
+ res_min = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min)
+ res = list(set(res_max + res_min))
+ if not self.op.ignore_ipolicy and (res_max or res_min):
+ msg = ("Instance allocation to group %s (%s) violates policy: %s" %
+ (group_info, group_info.name, utils.CommaJoin(res)))
+ raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

def _ConvertPlainToDrbd(self, feedback_fn):
"""Converts an instance from plain to drbd.
--
1.7.7.3

[Guido Trotter]

On Thu, Nov 8, 2012 at 12:43 PM, Helga Velroyen <hel...@google.com> wrote:
> When modifying an instance, so far the specs were not checked against
> the ipolicy. This patch fixes this issue.
>
> Note that for backend parameters which have a minimum and a maximum
> value (currently only memory), it checks both limits against the
> ipolicy.
>
> Signed-off-by: Helga Velroyen <hel...@google.com>
> ---
> lib/cmdlib.py | 42 ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 42 insertions(+), 0 deletions(-)
>
> diff --git a/lib/cmdlib.py b/lib/cmdlib.py
> index 18c54e3..96ea827 100644
> --- a/lib/cmdlib.py
> +++ b/lib/cmdlib.py
> @@ -12674,6 +12674,8 @@ class LUInstanceSetParams(LogicalUnit):
> self.needed_locks[locking.LEVEL_NODE] = []
> self.needed_locks[locking.LEVEL_NODE_RES] = []
> self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
> + # Look node group to look up the ipolicy
> + self.share_locks[locking.LEVEL_NODEGROUP] = 1
>

This is not enough to actually lock the node group. This way you are
just declaring that any lock at that level is shared, but you also
need to do the recalculate_locks at nodegroup level, and then acquire
the actual job, when the recalculation happens.

I'm not sure: why do we need to check twice, against two disks that
look very much the same?

thanks,

Guido

[Guido Trotter]

Ok, ok, I now see the difference between the two dictionaries.
Maybe we could change ispec to be able to accomodate range of
resources, and avoid having to do this here?
(if we can't then perhaps we should just make one dict, then copy and
just modify the memory to make things more readable)

thanks,

Guido

[Helga Velroyen]

Hi!

On Thu, Nov 8, 2012 at 1:31 PM, Guido Trotter <ultr...@google.com> wrote:

Thanks for the hint, I will fix it and resend the patch.

We need to check this twice, because for some parameters (currently only memory), we have a lower and an upper bound and both have to comply the ipolicy. I discussed that with Iustin, that in the future also other parameters will get that (for example CPU count), then there will not be so much double-checking anymore.

One thing that is not so nice right now is that the error message does not include if the upper or the lower bound fails. 

Cheers,

Helga

[Iustin Pop]

Changing ispec is uglier, because the more resources we have with
min/max, the more complex this becomes.

On the other hand, as Helga replied in the other sub-thread, doing two
verification passes, once for all minimum values, and the other time for
all maximum values, seems cleaner and more extensible.

> (if we can't then perhaps we should just make one dict, then copy and
> just modify the memory to make things more readable)

Yes, I was thinking that as well, this is quite hard to read.

thanks,
iustin

[Guido Trotter]

Ok, let's go for this option then.

Thanks,

Guido

[Helga Velroyen]

When modifying an instance, so far the specs were not checked against
the ipolicy. This patch fixes this issue.

Note that for backend parameters which have a minimum and a maximum
value (currently only memory), it checks both limits against the
ipolicy.

Because locking of the instance's node group was necessary, a TODO
of commit b8925b86 was fixed as well.

Signed-off-by: Helga Velroyen <hel...@google.com>
---

lib/cmdlib.py | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 18c54e3..0e2da90 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -12669,14 +12669,24 @@ class LUInstanceSetParams(LogicalUnit):

def ExpandNames(self):
self._ExpandAndLockInstance()
+ self.needed_locks[locking.LEVEL_NODEGROUP] = []
# Can't even acquire node locks in shared mode as upcoming changes in
# Ganeti 2.6 will start to modify the node object on disk conversion

self.needed_locks[locking.LEVEL_NODE] = []
self.needed_locks[locking.LEVEL_NODE_RES] = []
self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE

+ self.recalculate_locks[locking.LEVEL_NODEGROUP] = constants.LOCKS_REPLACE

+ # Look node group to look up the ipolicy
+ self.share_locks[locking.LEVEL_NODEGROUP] = 1

def DeclareLocks(self, level):
- # TODO: Acquire group lock in shared mode (disk parameters)
+ if level == locking.LEVEL_NODEGROUP:
+ assert not self.needed_locks[locking.LEVEL_NODEGROUP]
+ # Acquire locks for the instance's nodegroups optimistically. Needs
+ # to be verified in CheckPrereq
+ instance_groups = self.cfg.GetInstanceNodeGroups(self.op.instance_name)
+ self.needed_locks[locking.LEVEL_NODEGROUP] = instance_groups
+
if level == locking.LEVEL_NODE:
self._LockInstancesNodes()
if self.op.disk_template and self.op.remote_node:
@@ -12788,17 +12798,26 @@ class LUInstanceSetParams(LogicalUnit):
This only checks the instance list against the existing names.

"""
- # checking the new params on the primary/secondary nodes
-
+ assert self.op.instance_name in self.owned_locks(locking.LEVEL_INSTANCE)
instance = self.instance = self.cfg.GetInstanceInfo(self.op.instance_name)
+
cluster = self.cluster = self.cfg.GetClusterInfo()
assert self.instance is not None, \
"Cannot retrieve locked instance %s" % self.op.instance_name
+
pnode = instance.primary_node
+ assert pnode in self.owned_locks(locking.LEVEL_NODE)
nodelist = list(instance.all_nodes)

pnode_info = self.cfg.GetNodeInfo(pnode)
self.diskparams = self.cfg.GetInstanceDiskParams(instance)

+ #_CheckInstanceNodeGroups(self.cfg, self.op.instance_name, owned_groups)
+ assert pnode_info.group in self.owned_locks(locking.LEVEL_NODEGROUP)
+ group_info = self.cfg.GetNodeGroup(pnode_info.group)
+
+ # dictionary with instance information after the modification
+ ispec = {}

+
# Prepare disk/NIC modifications
self.diskmod = PrepareContainerMods(self.op.disks, None)
self.nicmod = PrepareContainerMods(self.op.nics, _InstNicModPrivate)

@@ -13039,6 +13058,12 @@ class LUInstanceSetParams(LogicalUnit):

raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
" more" % constants.MAX_DISKS,
errors.ECODE_STATE)

+ ispec[constants.ISPEC_DISK_COUNT] = len(disks)

+ # Retrieve the disk size from the old disks given of type
+ # ganeti.objects.Disk and of the new ones specified as dict
+ disk_sizes = [disk["size"] if type(disk) is dict else
+ disk.size for disk in disks]

+ ispec[constants.ISPEC_DISK_SIZE] = disk_sizes

if self.op.offline is not None:
if self.op.offline:

@@ -13055,8 +13080,33 @@ class LUInstanceSetParams(LogicalUnit):

ApplyContainerMods("NIC", nics, self._nic_chgdesc, self.nicmod,
self._CreateNewNic, self._ApplyNicMods, None)
self._new_nics = nics

+ ispec[constants.ISPEC_NIC_COUNT] = len(self._new_nics)
else:
self._new_nics = None
+ ispec[constants.ISPEC_NIC_COUNT] = len(instance.nics)
+

+ ipolicy = ganeti.masterd.instance.CalculateGroupIPolicy(cluster, group_info)
+

+ # Fill ispec with backend parameters
+ ispec[constants.ISPEC_SPINDLE_USE] = \
+ self.be_new.get(constants.BE_SPINDLE_USE, None)
+ ispec[constants.ISPEC_CPU_COUNT] = self.be_new.get(constants.BE_VCPUS, None)
+
+ # Copy ispec to verify parameters with min/max values separately
+ ispec_max = ispec.copy()

+ ispec_max[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MAXMEM, None)

+ res_max = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_max)

+ ispec_min = ispec.copy()

+ ispec_min[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MINMEM, None)

+ res_min = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min)
+
+ res = list(set(res_max + res_min))
+ if not self.op.ignore_ipolicy and (res_max or res_min):
+ msg = ("Instance allocation to group %s (%s) violates policy: %s" %
+ (group_info, group_info.name, utils.CommaJoin(res)))
+ raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

[Michael Hanselmann]

2012/11/8 Helga Velroyen <hel...@google.com>:

> --- a/lib/cmdlib.py
> +++ b/lib/cmdlib.py
> @@ -12669,14 +12669,24 @@ class LUInstanceSetParams(LogicalUnit):
>
> def ExpandNames(self):
> self._ExpandAndLockInstance()
> + self.needed_locks[locking.LEVEL_NODEGROUP] = []
> # Can't even acquire node locks in shared mode as upcoming changes in
> # Ganeti 2.6 will start to modify the node object on disk conversion
> self.needed_locks[locking.LEVEL_NODE] = []
> self.needed_locks[locking.LEVEL_NODE_RES] = []
> self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
> + self.recalculate_locks[locking.LEVEL_NODEGROUP] = constants.LOCKS_REPLACE

I don't think you need this. “recalculate_locks” is only used by one
or two functions and you don't seem to use them.

> + # Look node group to look up the ipolicy
> + self.share_locks[locking.LEVEL_NODEGROUP] = 1
>
> def DeclareLocks(self, level):
> - # TODO: Acquire group lock in shared mode (disk parameters)
> + if level == locking.LEVEL_NODEGROUP:
> + assert not self.needed_locks[locking.LEVEL_NODEGROUP]
> + # Acquire locks for the instance's nodegroups optimistically. Needs
> + # to be verified in CheckPrereq
> + instance_groups = self.cfg.GetInstanceNodeGroups(self.op.instance_name)
> + self.needed_locks[locking.LEVEL_NODEGROUP] = instance_groups

No need for the variable:

self.needed_locks[locking.LEVEL_NODEGROUP] = \
self.cfg.GetInstanceNodeGroups(self.op.instance_name)

> +
> if level == locking.LEVEL_NODE:

This should be an “elif” now.

> self._LockInstancesNodes()
> if self.op.disk_template and self.op.remote_node:

> […]

> + # dictionary with instance information after the modification
> + ispec = {}
> +
> # Prepare disk/NIC modifications
> self.diskmod = PrepareContainerMods(self.op.disks, None)
> self.nicmod = PrepareContainerMods(self.op.nics, _InstNicModPrivate)
> @@ -13039,6 +13058,12 @@ class LUInstanceSetParams(LogicalUnit):
> raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
> " more" % constants.MAX_DISKS,
> errors.ECODE_STATE)
> + ispec[constants.ISPEC_DISK_COUNT] = len(disks)
> + # Retrieve the disk size from the old disks given of type
> + # ganeti.objects.Disk and of the new ones specified as dict
> + disk_sizes = [disk["size"] if type(disk) is dict else
> + disk.size for disk in disks]

Please don't use the “foo if x else bar” syntax yet. You should use
“isinstance” instead of directly comparing types. Also, why would you
expect two different forms here?

> + ispec[constants.ISPEC_DISK_SIZE] = disk_sizes

Michael

[Helga Velroyen]

Hi!

Thanks for the review, Michael. My answers/comments inline:

On Thu, Nov 8, 2012 at 8:11 PM, Michael Hanselmann <han...@google.com> wrote:

2012/11/8 Helga Velroyen <hel...@google.com>:

> --- a/lib/cmdlib.py
> +++ b/lib/cmdlib.py
> @@ -12669,14 +12669,24 @@ class LUInstanceSetParams(LogicalUnit):
>
>    def ExpandNames(self):
>      self._ExpandAndLockInstance()
> +    self.needed_locks[locking.LEVEL_NODEGROUP] = []
>      # Can't even acquire node locks in shared mode as upcoming changes in
>      # Ganeti 2.6 will start to modify the node object on disk conversion
>      self.needed_locks[locking.LEVEL_NODE] = []
>      self.needed_locks[locking.LEVEL_NODE_RES] = []
>      self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
> +    self.recalculate_locks[locking.LEVEL_NODEGROUP] = constants.LOCKS_REPLACE

I don't think you need this. “recalculate_locks” is only used by one
or two functions and you don't seem to use them.

I call the _LockInstancesNodes method in my DeclareLocks method and it checks if self.recalculate_locks is set for that particular level. I tried without setting the "recalculate_locks" and I actually get the error "_LockInstancesNodes helper function called with no nodes to recalculate".

 

> +    # Look node group to look up the ipolicy
> +    self.share_locks[locking.LEVEL_NODEGROUP] = 1
>
>    def DeclareLocks(self, level):
> -    # TODO: Acquire group lock in shared mode (disk parameters)
> +    if level == locking.LEVEL_NODEGROUP:
> +      assert not self.needed_locks[locking.LEVEL_NODEGROUP]
> +      # Acquire locks for the instance's nodegroups optimistically. Needs
> +      # to be verified in CheckPrereq
> +      instance_groups = self.cfg.GetInstanceNodeGroups(self.op.instance_name)
> +      self.needed_locks[locking.LEVEL_NODEGROUP] = instance_groups

No need for the variable:

self.needed_locks[locking.LEVEL_NODEGROUP] = \
  self.cfg.GetInstanceNodeGroups(self.op.instance_name)

Okay, yes.

 

> +
>      if level == locking.LEVEL_NODE:

This should be an “elif” now.

Sure.

 

>        self._LockInstancesNodes()
>        if self.op.disk_template and self.op.remote_node:

> […]

> +    # dictionary with instance information after the modification
> +    ispec = {}
> +
>      # Prepare disk/NIC modifications
>      self.diskmod = PrepareContainerMods(self.op.disks, None)
>      self.nicmod = PrepareContainerMods(self.op.nics, _InstNicModPrivate)
> @@ -13039,6 +13058,12 @@ class LUInstanceSetParams(LogicalUnit):
>        raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
>                                   " more" % constants.MAX_DISKS,
>                                   errors.ECODE_STATE)
> +    ispec[constants.ISPEC_DISK_COUNT] = len(disks)
> +    # Retrieve the disk size from the old disks given of type
> +    # ganeti.objects.Disk and of the new ones specified as dict
> +    disk_sizes = [disk["size"] if type(disk) is dict else
> +                  disk.size for disk in disks]

Please don't use the “foo if x else bar” syntax yet. You should use
“isinstance” instead of directly comparing types. Also, why would you
expect two different forms here?

I did not expect that, but I found that out empirically, because it crashed. Seems that "disks" contains the old disks as ganeti.objects.Disk and the newly specified disk as a dictionary. Here the example excerpt from my logs when I call

gnt-instance modify --disk add:size=4G myinstance

// old disk:

disk: <DRBD8(hosts=[...]/0-[...]/2, port=11001, configured as [...]:[...] [...]:[...], backend=<LogicalVolume(/dev/xenvg/bc6c3a63-b294-4183-b923-b7497c9586a4.disk0_data, not visible, size=1024m)>, metadev=<LogicalVolume(/dev/xenvg/bc6c3a63-b294-4183-b923-b7497c9586a4.disk0_meta, not visible, size=128m)>, visible as /dev/disk/0, size=1024m)>

// new disk:

disk: {'mode': 'rw', 'size': 4096}

I take from your question that this i not as intended? 

Cheers,

Helga

[Helga Velroyen]

(Disregard this last email, it was send accidentally.)

[Michael Hanselmann]

2012/11/9 Helga Velroyen <hel...@google.com>:

> On Thu, Nov 8, 2012 at 8:11 PM, Michael Hanselmann <han...@google.com>
> wrote:
>> 2012/11/8 Helga Velroyen <hel...@google.com>:
>> > --- a/lib/cmdlib.py
>> > +++ b/lib/cmdlib.py

>> > + self.recalculate_locks[locking.LEVEL_NODEGROUP] =
>> > constants.LOCKS_REPLACE
>>
>> I don't think you need this. “recalculate_locks” is only used by one
>> or two functions and you don't seem to use them.
>
> I call the _LockInstancesNodes method in my DeclareLocks method and it
> checks if self.recalculate_locks is set for that particular level. I tried
> without setting the "recalculate_locks" and I actually get the error
> "_LockInstancesNodes helper function called with no nodes to recalculate".

Agreed for the nodes. Do you actually need it for groups (which you
now add), though?

>> > + ispec[constants.ISPEC_DISK_COUNT] = len(disks)
>> > + # Retrieve the disk size from the old disks given of type
>> > + # ganeti.objects.Disk and of the new ones specified as dict
>> > + disk_sizes = [disk["size"] if type(disk) is dict else
>> > + disk.size for disk in disks]
>>
>> Please don't use the “foo if x else bar” syntax yet. You should use
>> “isinstance” instead of directly comparing types. Also, why would you
>> expect two different forms here?
>
>
> I did not expect that, but I found that out empirically, because it crashed.
> Seems that "disks" contains the old disks as ganeti.objects.Disk and the
> newly specified disk as a dictionary. Here the example excerpt from my logs
> when I call
>
> gnt-instance modify --disk add:size=4G myinstance
>
> // old disk:
> disk: <DRBD8(hosts=[...]/0-[...]/2, port=11001, configured as [...]:[...]
> [...]:[...],
> backend=<LogicalVolume(/dev/xenvg/bc6c3a63-b294-4183-b923-b7497c9586a4.disk0_data,
> not visible, size=1024m)>,
> metadev=<LogicalVolume(/dev/xenvg/bc6c3a63-b294-4183-b923-b7497c9586a4.disk0_meta,
> not visible, size=128m)>, visible as /dev/disk/0, size=1024m)>
> // new disk:
> disk: {'mode': 'rw', 'size': 4096}
>
> I take from your question that this i not as intended?

No, this shouldn't happen. Lists should, in most cases, contain
uniform types. Can you try to figure out why there are different
types? I guess it just works 'cause the serialization format is the
same for both, but I can only guess.

Michael

[Helga Velroyen]

Hi!

On Fri, Nov 9, 2012 at 2:52 PM, Michael Hanselmann <han...@google.com> wrote:

2012/11/9 Helga Velroyen <hel...@google.com>:

> On Thu, Nov 8, 2012 at 8:11 PM, Michael Hanselmann <han...@google.com>
> wrote:
>> 2012/11/8 Helga Velroyen <hel...@google.com>:
>> > --- a/lib/cmdlib.py
>> > +++ b/lib/cmdlib.py

>> > +    self.recalculate_locks[locking.LEVEL_NODEGROUP] =
>> > constants.LOCKS_REPLACE
>>
>> I don't think you need this. “recalculate_locks” is only used by one
>> or two functions and you don't seem to use them.
>
> I call the _LockInstancesNodes method in my DeclareLocks method and it
> checks if self.recalculate_locks is set for that particular level. I tried
> without setting the "recalculate_locks" and I actually get the error
> "_LockInstancesNodes helper function called with no nodes to recalculate".

Agreed for the nodes. Do you actually need it for groups (which you
now add), though?

Right, I don't need it. Removed it and will resend the patch. 

I investigated this, and found out it is a bug in the ApplyContainerMods method. I discussed this with Iustin and opened an issue (http://code.google.com/p/ganeti/issues/detail?id=304). I will fix this patch so that it does not use the if/else, but retrieves the old disk sizes from the disks list and the new ones from the diskmod.

Cheers,

Helga

[Helga Velroyen]

When modifying an instance, so far the specs were not checked against
the ipolicy. This patch fixes this issue.

Note that for backend parameters which have a minimum and a maximum
value (currently only memory), it checks both limits against the
ipolicy.
Because locking of the instance's node group was necessary, a TODO
of commit b8925b86 was fixed as well.

Signed-off-by: Helga Velroyen <hel...@google.com>
---

lib/cmdlib.py | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 18c54e3..5da23f4 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -12669,15 +12669,23 @@ class LUInstanceSetParams(LogicalUnit):

def ExpandNames(self):
self._ExpandAndLockInstance()
+ self.needed_locks[locking.LEVEL_NODEGROUP] = []
# Can't even acquire node locks in shared mode as upcoming changes in
# Ganeti 2.6 will start to modify the node object on disk conversion
self.needed_locks[locking.LEVEL_NODE] = []
self.needed_locks[locking.LEVEL_NODE_RES] = []
self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE

+ # Look node group to look up the ipolicy
+ self.share_locks[locking.LEVEL_NODEGROUP] = 1

def DeclareLocks(self, level):
- # TODO: Acquire group lock in shared mode (disk parameters)

- if level == locking.LEVEL_NODE:

+ if level == locking.LEVEL_NODEGROUP:
+ assert not self.needed_locks[locking.LEVEL_NODEGROUP]
+ # Acquire locks for the instance's nodegroups optimistically. Needs
+ # to be verified in CheckPrereq

+ self.needed_locks[locking.LEVEL_NODEGROUP] = \
+ self.cfg.GetInstanceNodeGroups(self.op.instance_name)
+ elif level == locking.LEVEL_NODE:

self._LockInstancesNodes()
if self.op.disk_template and self.op.remote_node:

self.op.remote_node = _ExpandNodeName(self.cfg, self.op.remote_node)
@@ -12788,17 +12796,26 @@ class LUInstanceSetParams(LogicalUnit):

@@ -13039,6 +13056,11 @@ class LUInstanceSetParams(LogicalUnit):

raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
" more" % constants.MAX_DISKS,
errors.ECODE_STATE)

+ disk_sizes = [disk.size for disk in instance.disks]
+ disk_sizes.extend(params["size"] for (op, idx, params, private) in
+ self.diskmod)
+ ispec[constants.ISPEC_DISK_COUNT] = len(disk_sizes)

+ ispec[constants.ISPEC_DISK_SIZE] = disk_sizes

if self.op.offline is not None:
if self.op.offline:

@@ -13055,8 +13077,35 @@ class LUInstanceSetParams(LogicalUnit):

+ # FIXME: Improve error message by including information about whether the
+ # upper or lower limit of the parameter fails the ipolicy.

[Michael Hanselmann]

2012/11/12 Helga Velroyen <hel...@google.com>:

> + res = list(set(res_max + res_min))

You don't have to convert to list, CommaJoin can work with sets too.
Maybe you should use “sorted” instead?

> + # FIXME: Improve error message by including information about whether the
> + # upper or lower limit of the parameter fails the ipolicy.
> + if not self.op.ignore_ipolicy and (res_max or res_min):
> + msg = ("Instance allocation to group %s (%s) violates policy: %s" %
> + (group_info, group_info.name, utils.CommaJoin(res)))
> + raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

Rest LGTM.

Michael

[Helga Velroyen]

Hi!

On Mon, Nov 12, 2012 at 5:57 PM, Michael Hanselmann <han...@google.com> wrote:

2012/11/12 Helga Velroyen <hel...@google.com>:

> +    res = list(set(res_max + res_min))

You don't have to convert to list, CommaJoin can work with sets too.
Maybe you should use “sorted” instead?

I removed the "list(...)". I don't really see the point in sorting, or do we insist on alphabetically sorted error messages anywhere else?

 

> +    # FIXME: Improve error message by including information about whether the
> +    # upper or lower limit of the parameter fails the ipolicy.
> +    if not self.op.ignore_ipolicy and (res_max or res_min):
> +      msg = ("Instance allocation to group %s (%s) violates policy: %s" %
> +             (group_info, group_info.name, utils.CommaJoin(res)))
> +      raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

Rest LGTM.

Thanks, I resend the patch.

Cheers,

Helga

[Helga Velroyen]

When modifying an instance, so far the specs were not checked against
the ipolicy. This patch fixes this issue.

Note that for backend parameters which have a minimum and a maximum
value (currently only memory), it checks both limits against the
ipolicy.
Because locking of the instance's node group was necessary, a TODO
of commit b8925b86 was fixed as well.

Signed-off-by: Helga Velroyen <hel...@google.com>
---
lib/cmdlib.py | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py

index 18c54e3..237f4d6 100644

+ res = set(res_max + res_min)

+ # FIXME: Improve error message by including information about whether the
+ # upper or lower limit of the parameter fails the ipolicy.
+ if not self.op.ignore_ipolicy and (res_max or res_min):
+ msg = ("Instance allocation to group %s (%s) violates policy: %s" %
+ (group_info, group_info.name, utils.CommaJoin(res)))
+ raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

[Michael Hanselmann]

2012/11/13 Helga Velroyen <hel...@google.com>:

> + if not self.op.ignore_ipolicy and (res_max or res_min):

Thinking about it now, do you actually have to do the whole
computation if the result is ignored anyway?

> + msg = ("Instance allocation to group %s (%s) violates policy: %s" %
> + (group_info, group_info.name, utils.CommaJoin(res)))
> + raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

Rest LGTM.

Thanks,
Michael

[Helga Velroyen]

Hi!

Sorry for the delay, I had to fix some technical problems with my environment before coming back to this.

On Tue, Nov 13, 2012 at 2:59 PM, Michael Hanselmann <han...@google.com> wrote:

2012/11/13 Helga Velroyen <hel...@google.com>:

> +    if not self.op.ignore_ipolicy and (res_max or res_min):

Thinking about it now, do you actually have to do the whole
computation if the result is ignored anyway?

Right. I rearranged that and will send the patch again. 

Cheers,

Helga

[Helga Velroyen]

When modifying an instance, so far the specs were not checked against
the ipolicy. This patch fixes this issue.

Note that for backend parameters which have a minimum and a maximum
value (currently only memory), it checks both limits against the
ipolicy.
Because locking of the instance's node group was necessary, a TODO
of commit b8925b86 was fixed as well.

Signed-off-by: Helga Velroyen <hel...@google.com>
---

lib/cmdlib.py | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index aaf1c64..298e82c 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -12671,15 +12671,23 @@ class LUInstanceSetParams(LogicalUnit):

def ExpandNames(self):
self._ExpandAndLockInstance()
+ self.needed_locks[locking.LEVEL_NODEGROUP] = []
# Can't even acquire node locks in shared mode as upcoming changes in
# Ganeti 2.6 will start to modify the node object on disk conversion
self.needed_locks[locking.LEVEL_NODE] = []
self.needed_locks[locking.LEVEL_NODE_RES] = []
self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
+ # Look node group to look up the ipolicy
+ self.share_locks[locking.LEVEL_NODEGROUP] = 1

def DeclareLocks(self, level):
- # TODO: Acquire group lock in shared mode (disk parameters)
- if level == locking.LEVEL_NODE:
+ if level == locking.LEVEL_NODEGROUP:
+ assert not self.needed_locks[locking.LEVEL_NODEGROUP]
+ # Acquire locks for the instance's nodegroups optimistically. Needs
+ # to be verified in CheckPrereq
+ self.needed_locks[locking.LEVEL_NODEGROUP] = \
+ self.cfg.GetInstanceNodeGroups(self.op.instance_name)
+ elif level == locking.LEVEL_NODE:
self._LockInstancesNodes()
if self.op.disk_template and self.op.remote_node:
self.op.remote_node = _ExpandNodeName(self.cfg, self.op.remote_node)

@@ -12790,17 +12798,26 @@ class LUInstanceSetParams(LogicalUnit):

@@ -13041,6 +13058,11 @@ class LUInstanceSetParams(LogicalUnit):

raise errors.OpPrereqError("Instance has too many disks (%d), cannot add"
" more" % constants.MAX_DISKS,
errors.ECODE_STATE)
+ disk_sizes = [disk.size for disk in instance.disks]
+ disk_sizes.extend(params["size"] for (op, idx, params, private) in
+ self.diskmod)
+ ispec[constants.ISPEC_DISK_COUNT] = len(disk_sizes)
+ ispec[constants.ISPEC_DISK_SIZE] = disk_sizes

if self.op.offline is not None:
if self.op.offline:

@@ -13057,8 +13079,38 @@ class LUInstanceSetParams(LogicalUnit):

ApplyContainerMods("NIC", nics, self._nic_chgdesc, self.nicmod,
self._CreateNewNic, self._ApplyNicMods, None)
self._new_nics = nics
+ ispec[constants.ISPEC_NIC_COUNT] = len(self._new_nics)
else:
self._new_nics = None
+ ispec[constants.ISPEC_NIC_COUNT] = len(instance.nics)
+

+ if not self.op.ignore_ipolicy:
+ ipolicy = ganeti.masterd.instance.CalculateGroupIPolicy(cluster,
+ group_info)

+
+ # Fill ispec with backend parameters
+ ispec[constants.ISPEC_SPINDLE_USE] = \
+ self.be_new.get(constants.BE_SPINDLE_USE, None)
+ ispec[constants.ISPEC_CPU_COUNT] = self.be_new.get(constants.BE_VCPUS,

+ None)

+
+ # Copy ispec to verify parameters with min/max values separately
+ ispec_max = ispec.copy()
+ ispec_max[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MAXMEM, None)
+ res_max = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_max)
+ ispec_min = ispec.copy()
+ ispec_min[constants.ISPEC_MEM_SIZE] = \
+ self.be_new.get(constants.BE_MINMEM, None)
+ res_min = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min)
+

+ if (res_max or res_min):

+ res = set(res_max + res_min)
+ # FIXME: Improve error message by including information about whether

+ # the upper or lower limit of the parameter fails the ipolicy.

+ msg = ("Instance allocation to group %s (%s) violates policy: %s" %
+ (group_info, group_info.name, utils.CommaJoin(res)))
+ raise errors.OpPrereqError(msg, errors.ECODE_INVAL)

[Michael Hanselmann]

2012/11/20 Helga Velroyen <hel...@google.com>:

> When modifying an instance, so far the specs were not checked against
> the ipolicy. This patch fixes this issue.
>
> Note that for backend parameters which have a minimum and a maximum
> value (currently only memory), it checks both limits against the
> ipolicy.
> Because locking of the instance's node group was necessary, a TODO
> of commit b8925b86 was fixed as well.

LGTM, thanks!

[Guido Trotter]

You're missing a recalculate_locks at level nodegroup here.

Thanks,

Guido

[Michael Hanselmann]

2012/11/21 Guido Trotter <ultr...@google.com>:

> On 20 Nov 2012 18:16, "Helga Velroyen" <hel...@google.com> wrote:
>> self.recalculate_locks[locking.LEVEL_NODE] = constants.LOCKS_REPLACE
>
> You're missing a recalculate_locks at level nodegroup here.

No, we discussed that earlier.

Michael