This might actually work. It's an incremental improvement at best.
diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
index 7d15c36..09cea79 100644
--- a/lib/cmdlib/cluster.py
+++ b/lib/cmdlib/cluster.py
@@ -1163,44 +1163,7 @@ class LUClusterSetParams(LogicalUnit):
self.new_os_hvp[os_name][hv_name].update(hv_dict)
# os parameters
- self.new_osp = objects.FillDict(cluster.osparams, {})
- if self.op.osparams:
- for os_name, osp in self.op.osparams.items():
- if os_name not in self.new_osp:
- self.new_osp[os_name] = {}
-
- self.new_osp[os_name] = GetUpdatedParams(self.new_osp[os_name], osp,
- use_none=True)
-
- if not self.new_osp[os_name]:
- # we removed all parameters
- del self.new_osp[os_name]
-
- # private os params
- self.new_osp_private = objects.FillDict(cluster.osparams_private_cluster,
- {})
- if self.op.osparams_private_cluster:
- for os_name, osp in self.op.osparams_private_cluster.items():
- if os_name not in self.new_osp_private:
- self.new_osp_private[os_name] = {}
-
- self.new_osp_private[os_name] = GetUpdatedParams(
- self.new_osp_private[os_name], osp, use_none=True
- )
-
- if not self.new_osp_private[os_name]:
- # we removed all parameters
- del self.new_osp_private[os_name]
-
- # Remove os validity check
- changed_oses = (set(self.new_osp.keys()) |
set(self.new_osp_private.keys()))
- for os_name in changed_oses:
- os_params = cluster.SimpleFillOS(os_name,
- self.new_osp.get(os_name, {}),
- self.new_osp_private.get(os_name, {}))
- # check the parameter validity (remote check)
- CheckOSParams(self, False, [self.cfg.GetMasterNode()],
- os_name, os_params)
+ self._BuildOSParams(cluster)
# changes to the hypervisor list
if self.op.enabled_hypervisors is not None:
@@ -1254,6 +1217,37 @@ class LUClusterSetParams(LogicalUnit):
" specified" % self.op.default_iallocator,
errors.ECODE_INVAL)
+ def _BuildOSParams(self, cluster):
+ "Calculate the new OS parameters for this operation."
+
+ def _GetNewParams(source, new_params):
+ "Wrapper around GetUpdatedParams."
+ if new_params is None:
+ return source
+ result = objects.FillDict(source, {}) # deep copy of source
+ for os_name in new_params:
+ result[os_name] = GetUpdatedParams(result.get(os_name, {}),
+ new_params[os_name],
+ use_none=True)
+ if not result[os_name]:
+ del result[os_name] # we removed all parameters
+ return result
+
+ self.new_osp = _GetNewParams(cluster.osparams,
+ self.op.osparams)
+ self.new_osp_private = _GetNewParams(cluster.osparams_private_cluster,
+ self.op.osparams_private_cluster)
+
+ # Remove os validity check
+ changed_oses = (set(self.new_osp.keys()) |
set(self.new_osp_private.keys()))
+ for os_name in changed_oses:
+ os_params = cluster.SimpleFillOS(os_name,
+ self.new_osp.get(os_name, {}),
+ self.new_osp_private.get(os_name, {}))
+ # check the parameter validity (remote check)
+ CheckOSParams(self, False, [self.cfg.GetMasterNode()],
+ os_name, os_params)
+
def _CheckDiskTemplateConsistency(self):
"""Check whether the disk templates that are going to be disabled
are still in use by some instances.
On Wed, Feb 5, 2014 at 11:28 PM, Santi Raffa <[email protected]> wrote:
> On Wed, Feb 5, 2014 at 6:46 PM, Jose A. Lopes <[email protected]> wrote:
>>> - else:
>>> - # check the parameter validity (remote check)
>>> - CheckOSParams(self, False, [self.cfg.GetMasterNode()],
>>> - os_name, self.new_osp[os_name])
>>> +
>>> + # private os params
>>> + self.new_osp_private =
>>> objects.FillDict(cluster.osparams_private_cluster,
>>> + {})
>>> + if self.op.osparams_private_cluster:
>>> + for os_name, osp in self.op.osparams_private_cluster.items():
>>> + if os_name not in self.new_osp_private:
>>> + self.new_osp_private[os_name] = {}
>>> +
>>> + self.new_osp_private[os_name] = GetUpdatedParams(
>>> + self.new_osp_private[os_name], osp, use_none=True
>>> + )
>>> +
>>> + if not self.new_osp_private[os_name]:
>>> + # we removed all parameters
>>> + del self.new_osp_private[os_name]
>>> +
>>
>> Can we extract the code above to a separate function/method and thus
>> avoid the code duplication for osparams and private osparams?
>
> This refactoring makes me quite nervous. This is my best shot at it. I
> have not tested it.
>
> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
> index 7d15c36..f9da4aa 100644
> --- a/lib/cmdlib/cluster.py
> +++ b/lib/cmdlib/cluster.py
> @@ -1163,44 +1163,7 @@ class LUClusterSetParams(LogicalUnit):
> self.new_os_hvp[os_name][hv_name].update(hv_dict)
>
> # os parameters
> - self.new_osp = objects.FillDict(cluster.osparams, {})
> - if self.op.osparams:
> - for os_name, osp in self.op.osparams.items():
> - if os_name not in self.new_osp:
> - self.new_osp[os_name] = {}
> -
> - self.new_osp[os_name] = GetUpdatedParams(self.new_osp[os_name], osp,
> - use_none=True)
> -
> - if not self.new_osp[os_name]:
> - # we removed all parameters
> - del self.new_osp[os_name]
> -
> - # private os params
> - self.new_osp_private = objects.FillDict(cluster.osparams_private_cluster,
> - {})
> - if self.op.osparams_private_cluster:
> - for os_name, osp in self.op.osparams_private_cluster.items():
> - if os_name not in self.new_osp_private:
> - self.new_osp_private[os_name] = {}
> -
> - self.new_osp_private[os_name] = GetUpdatedParams(
> - self.new_osp_private[os_name], osp, use_none=True
> - )
> -
> - if not self.new_osp_private[os_name]:
> - # we removed all parameters
> - del self.new_osp_private[os_name]
> -
> - # Remove os validity check
> - changed_oses = (set(self.new_osp.keys()) |
> set(self.new_osp_private.keys()))
> - for os_name in changed_oses:
> - os_params = cluster.SimpleFillOS(os_name,
> - self.new_osp.get(os_name, {}),
> - self.new_osp_private.get(os_name, {}))
> - # check the parameter validity (remote check)
> - CheckOSParams(self, False, [self.cfg.GetMasterNode()],
> - os_name, os_params)
> + self._BuildOSParams(cluster, os_name)
>
> # changes to the hypervisor list
> if self.op.enabled_hypervisors is not None:
> @@ -1254,6 +1217,35 @@ class LUClusterSetParams(LogicalUnit):
> " specified" % self.op.default_iallocator,
> errors.ECODE_INVAL)
>
> + def _BuildOSParams(self, cluster, os_name):
> + "Calculate the new OS parameters for this operation."
> +
> + def _GetNewParams(source, new_params):
> + "Wrapper around GetUpdatedParams."
> + result = objects.FillDict(source, {}) # deep copy of source
> + for os_name in new_params:
> + result[os_name] = GetUpdatedParams(result.get(os_name, {}),
> + new_params[os_name],
> + use_none=True)
> + if not result[os_name]:
> + del result[os_name] # we removed all parameters
> + return result
> +
> + self.new_osp = _GetNewParams(cluster.osparams,
> + self.op.osparams)
> + self.new_osp_private = _GetNewParams(cluster.osparams_private_cluster,
> + self.op.osparams_private_cluster)
> +
> + # Remove os validity check
> + changed_oses = (set(self.new_osp.keys()) |
> set(self.new_osp_private.keys()))
> + for os_name in changed_oses:
> + os_params = cluster.SimpleFillOS(os_name,
> + self.new_osp.get(os_name, {}),
> + self.new_osp_private.get(os_name, {}))
> + # check the parameter validity (remote check)
> + CheckOSParams(self, False, [self.cfg.GetMasterNode()],
> + os_name, os_params)
> +
> def _CheckDiskTemplateConsistency(self):
> """Check whether the disk templates that are going to be disabled
> are still in use by some instances.
>
>
> --
> Raffa Santi
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
--
Raffa Santi
Google Germany GmbH
Dienerstr. 12
80331 München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
