LGTM. Thanks, Jose
On Thu, Feb 06, 2014 at 05:24:16PM +0100, Santi Raffa wrote: > Modify ClusterSetParams to accept and process private parameters. > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/client/gnt_os.py | 12 ++++++--- > lib/cmdlib/cluster.py | 52 > ++++++++++++++++++++++++++------------ > lib/cmdlib/common.py | 7 +++++ > man/gnt-os.rst | 10 +++++++- > src/Ganeti/Objects.hs | 1 + > src/Ganeti/OpCodes.hs | 1 + > src/Ganeti/OpParams.hs | 10 ++++++++ > test/hs/Test/Ganeti/OpCodes.hs | 48 ++++++++++++++++++++++++++--------- > test/py/cmdlib/cluster_unittest.py | 1 + > 9 files changed, 110 insertions(+), 32 deletions(-) > > diff --git a/lib/client/gnt_os.py b/lib/client/gnt_os.py > index f522633..ae7b9eb 100644 > --- a/lib/client/gnt_os.py > +++ b/lib/client/gnt_os.py > @@ -245,6 +245,11 @@ def ModifyOS(opts, args): > else: > osp = None > > + if opts.osparams_private: > + osp_private = {os: opts.osparams_private} > + else: > + osp_private = None > + > if opts.hidden is not None: > if opts.hidden: > ohid = [(constants.DDM_ADD, os)] > @@ -261,13 +266,14 @@ def ModifyOS(opts, args): > else: > oblk = None > > - if not (os_hvp or osp or ohid or oblk): > + if not (os_hvp or osp or osp_private or ohid or oblk): > ToStderr("At least one of OS parameters or hypervisor parameters" > " must be passed") > return 1 > > op = opcodes.OpClusterSetParams(os_hvp=os_hvp, > osparams=osp, > + osparams_private_cluster=osp_private, > hidden_os=ohid, > blacklisted_os=oblk) > SubmitOrSend(op, opts) > @@ -288,8 +294,8 @@ commands = { > "operating systems"), > "modify": ( > ModifyOS, ARGS_ONE_OS, > - [HVLIST_OPT, OSPARAMS_OPT, DRY_RUN_OPT, PRIORITY_OPT, > - HID_OS_OPT, BLK_OS_OPT] + SUBMIT_OPTS, > + [HVLIST_OPT, OSPARAMS_OPT, OSPARAMS_PRIVATE_OPT, > + DRY_RUN_OPT, PRIORITY_OPT, HID_OS_OPT, BLK_OS_OPT] + SUBMIT_OPTS, > "", "Modify the OS parameters"), > } > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index 5f18205..546a5e3 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -1163,22 +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] > - else: > - # check the parameter validity (remote check) > - CheckOSParams(self, False, [self.cfg.GetMasterNode()], > - os_name, self.new_osp[os_name]) > + self._BuildOSParams(cluster) > > # changes to the hypervisor list > if self.op.enabled_hypervisors is not None: > @@ -1232,6 +1217,39 @@ 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, {}), > + os_params_private=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. > @@ -1318,6 +1336,8 @@ class LUClusterSetParams(LogicalUnit): > self.cluster.ipolicy = self.new_ipolicy > if self.op.osparams: > self.cluster.osparams = self.new_osp > + if self.op.osparams_private_cluster: > + self.cluster.osparams_private_cluster = self.new_osp_private > if self.op.ndparams: > self.cluster.ndparams = self.new_ndparams > if self.op.diskparams: > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index 2a98836..8d448ba 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -32,6 +32,7 @@ from ganeti import locking > from ganeti import objects > from ganeti import opcodes > from ganeti import pathutils > +from ganeti.serializer import Private > import ganeti.rpc.node as rpc > from ganeti import ssconf > from ganeti import utils > @@ -382,6 +383,12 @@ def CheckOSParams(lu, required, node_uuids, osname, > osparams): > > """ > node_uuids = _FilterVmNodes(lu, node_uuids) > + > + # Last chance to unwrap private elements. > + for key in osparams: > + if isinstance(osparams[key], Private): > + osparams[key] = osparams[key].Get() > + > result = lu.rpc.call_os_validate(node_uuids, required, osname, > [constants.OS_VALIDATE_PARAMETERS], > osparams) > diff --git a/man/gnt-os.rst b/man/gnt-os.rst > index c3392aa..38a56b0 100644 > --- a/man/gnt-os.rst > +++ b/man/gnt-os.rst > @@ -57,6 +57,8 @@ MODIFY > ~~~~~~ > > | **modify** [\--submit] [\--print-job-id] > +| [ [ -O | --os-parameters ] =*option*=*value*] > +| [ --os-parameters-private=*option*=*value*] > | [-H *HYPERVISOR*:option=*value*[,...]] > | [\--hidden=*yes|no*] [\--blacklisted=*yes|no*] > | {*OS*} > @@ -68,8 +70,14 @@ global hypervisor parameters), you can run modify ``-H`` > with the > same syntax as in **gnt-cluster init** to override default > hypervisor parameters of the cluster for specified *OS* argument. > > +To modify the parameters passed to the OS install scripts, use the > +**--os-parameters** option. If the value of the parameter should not be > +saved to logs, use **--os-parameters-private** *and* make sure that > +no Ganeti daemon or program is running in debug mode. **ganeti-luxid** > +in particular will issue a warning at startup time if ran in debug mode. > + > To modify the hidden and blacklisted states of an OS, pass the options > -``--hidden ``*yes|no*, or respectively ``--blacklisted ...``. The > +``--hidden`` *yes|no*, or respectively ``--blacklisted ...``. The > 'hidden' state means that an OS won't be listed by default in the OS > list, but is available for installation. The 'blacklisted' state means > that the OS is not listed and is also not allowed for new instance > diff --git a/src/Ganeti/Objects.hs b/src/Ganeti/Objects.hs > index 68fb0b2..348674c 100644 > --- a/src/Ganeti/Objects.hs > +++ b/src/Ganeti/Objects.hs > @@ -72,6 +72,7 @@ module Ganeti.Objects > , OsHvParams > , ClusterBeParams > , ClusterOsParams > + , ClusterOsParamsPrivate > , ClusterNicParams > , Cluster(..) > , ConfigData(..) > diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs > index b6e26df..29a9a43 100644 > --- a/src/Ganeti/OpCodes.hs > +++ b/src/Ganeti/OpCodes.hs > @@ -213,6 +213,7 @@ $(genOpCode "OpCode" > , pClusterBeParams > , pOsHvp > , pClusterOsParams > + , pClusterOsParamsPrivate > , pDiskParams > , pCandidatePoolSize > , pMaxRunningJobs > diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs > index c5588de..efb5b90 100644 > --- a/src/Ganeti/OpParams.hs > +++ b/src/Ganeti/OpParams.hs > @@ -118,6 +118,7 @@ module Ganeti.OpParams > , pResetDefaults > , pOsHvp > , pClusterOsParams > + , pClusterOsParamsPrivate > , pInstOsParams > , pCandidatePoolSize > , pMaxRunningJobs > @@ -620,6 +621,15 @@ pClusterOsParams = > optionalField $ > simpleField "osparams" [t| GenericContainer String (JSObject JSValue) |] > > +pClusterOsParamsPrivate :: Field > +pClusterOsParamsPrivate = > + withDoc "Cluster-wide private OS parameter defaults" . > + renameField "ClusterOsParamsPrivate" . > + optionalField $ > + -- This field needs an unique name to aid Python deserialization > + simpleField "osparams_private_cluster" > + [t| GenericContainer String (JSObject (Private JSValue)) |] > + > pDiskParams :: Field > pDiskParams = > withDoc "Disk templates' parameter defaults" . > diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.hs > index ef480ba..e79b691 100644 > --- a/test/hs/Test/Ganeti/OpCodes.hs > +++ b/test/hs/Test/Ganeti/OpCodes.hs > @@ -171,18 +171,42 @@ instance Arbitrary OpCodes.OpCode where > "OP_CLUSTER_RENAME" -> > OpCodes.OpClusterRename <$> genNameNE > "OP_CLUSTER_SET_PARAMS" -> > - OpCodes.OpClusterSetParams <$> arbitrary <*> emptyMUD <*> emptyMUD > <*> > - arbitrary <*> genMaybe arbitrary <*> > - genMaybe genEmptyContainer <*> emptyMUD <*> > - genMaybe genEmptyContainer <*> genMaybe genEmptyContainer <*> > - genMaybe genEmptyContainer <*> genMaybe arbitrary <*> > - genMaybe arbitrary <*> > - arbitrary <*> arbitrary <*> arbitrary <*> > - arbitrary <*> arbitrary <*> arbitrary <*> > - emptyMUD <*> emptyMUD <*> arbitrary <*> > - arbitrary <*> emptyMUD <*> arbitrary <*> arbitrary <*> arbitrary > <*> > - arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary > <*> > - arbitrary <*> genMaybe genName <*> genMaybe genName > + OpCodes.OpClusterSetParams > + <$> arbitrary -- force > + <*> emptyMUD -- hv_state > + <*> emptyMUD -- disk_state > + <*> arbitrary -- vg_name > + <*> genMaybe arbitrary -- enabled_hypervisors > + <*> genMaybe genEmptyContainer -- hvparams > + <*> emptyMUD -- beparams > + <*> genMaybe genEmptyContainer -- os_hvp > + <*> genMaybe genEmptyContainer -- osparams > + <*> genMaybe genEmptyContainer -- osparams_private_cluster > + <*> genMaybe genEmptyContainer -- diskparams > + <*> genMaybe arbitrary -- candidate_pool_size > + <*> genMaybe arbitrary -- max_running_jobs > + <*> arbitrary -- uid_pool > + <*> arbitrary -- add_uids > + <*> arbitrary -- remove_uids > + <*> arbitrary -- maintain_node_health > + <*> arbitrary -- prealloc_wipe_disks > + <*> arbitrary -- nicparams > + <*> emptyMUD -- ndparams > + <*> emptyMUD -- ipolicy > + <*> arbitrary -- drbd_helper > + <*> arbitrary -- default_iallocator > + <*> emptyMUD -- default_iallocator_params > + <*> arbitrary -- master_netdev > + <*> arbitrary -- master_netmask > + <*> arbitrary -- reserved_lvs > + <*> arbitrary -- hidden_os > + <*> arbitrary -- blacklisted_os > + <*> arbitrary -- use_external_mip_script > + <*> arbitrary -- enabled_disk_templates > + <*> arbitrary -- modify_etc_hosts > + <*> genMaybe genName -- file_storage_dir > + <*> genMaybe genName -- shared_file_storage_dir > + <*> genMaybe genName -- gluster_file_storage_dir > "OP_CLUSTER_REDIST_CONF" -> pure OpCodes.OpClusterRedistConf > "OP_CLUSTER_ACTIVATE_MASTER_IP" -> > pure OpCodes.OpClusterActivateMasterIp > diff --git a/test/py/cmdlib/cluster_unittest.py > b/test/py/cmdlib/cluster_unittest.py > index fad9276..37be652 100644 > --- a/test/py/cmdlib/cluster_unittest.py > +++ b/test/py/cmdlib/cluster_unittest.py > @@ -752,6 +752,7 @@ class TestLUClusterSetParams(CmdlibTestCase): > } > } > self.cluster.osparams = {"other_os": {"param1": "value1"}} > + self.cluster.osparams_private_cluster = {} > op = opcodes.OpClusterSetParams(osparams=osparams) > self.ExecOpCode(op) > > -- > 1.9.0.rc1.175.g0b1dcb5 > -- 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
