Skip to content

Conversation

@robhoes
Copy link
Member

@robhoes robhoes commented Aug 16, 2016

I have tested this by removing the restrict_xen_motion key from the license_params of the slave in a two-host pool. It used to be restrict_xen_motion = "false", i.e. XenMotion is enabled, on both the master and slave. Then I checked the resulting value of pool.restrictions. Before the patch, pool.restrictions had restrict_xen_motion = "true" (i.e. XenMotion disabled). With the patch, it correctly had restrict_xen_motion = "false".

@lindig
Copy link
Contributor

lindig commented Aug 17, 2016

I wonder: wouldn't it be simpler to express feature sets using string sets (Set.Make(String))?

@robhoes
Copy link
Member Author

robhoes commented Aug 17, 2016

@lindig Using a Set may work, but then based on the feature type. I'm not sure if that's going to be a lot simpler than what we have now, based on lists, because duplicates are not really a problem. Also, changing the types was really a step further than I wanted to go in this bugfix.

@lindig
Copy link
Contributor

lindig commented Aug 17, 2016

One comment about names;

  • compute_pool_restrictions
  • find_additional_flags
  • compute_additional_restrictions

These names suggest actions (compute, find) but represent property lists. I would have used names that focus on these properties: common_features, extra_features, extra_restrictions or similar. No need to change them, though.

This makes it a little easier to understand. There are no functional changes.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
The XenMotion and AD feature recently had restrictions put in place, so that
they can be switched on and off by feature flags. A feature is considered to be
enabled only if all hosts in the pool have it enabled. During a rolling pool
upgrade (RPU) from a pool that does not have the new feature flags, the absence
of the flags was treated as a feature restriction, such that the pool as a
whole had XenMotion and AD restricted. Luckily the restrictions were only
enforced on hosts that were already upgraded, so that VMs could still migrate
off hosts that still need to be upgraded.

This patch introduces a list of features (currently just XenMotion and AD) that
are considered to be enabled if the feature flag is absent, to avoid the issues
described above.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
@robhoes
Copy link
Member Author

robhoes commented Aug 17, 2016

We talked about the names above, and seem to have to accept that different styles which can all be justified in some manner :)

I did update the first patch to better explain the terminology in a comment on top of the file.

@lindig
Copy link
Contributor

lindig commented Aug 17, 2016

:shipit:

@robhoes robhoes merged commit 745c932 into xapi-project:master Aug 17, 2016
@robhoes robhoes deleted the ca212079 branch August 17, 2016 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants