LGTM but you need to test it.
Thanks,
Jose
On Thu, Feb 06, 2014 at 12:37:14AM +0100, Santi Raffa wrote:
> 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
--
Jose Antonio Lopes
Ganeti Engineering
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
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
