LGTM. Thanks, Jose
On Thu, Feb 06, 2014 at 05:24:17PM +0100, Santi Raffa wrote: > Modify InstanceCreate to accept process private and secret parameters > > Signed-off-by: Santi Raffa <[email protected]> > --- > lib/backend.py | 4 +++ > lib/cli.py | 2 ++ > lib/client/gnt_instance.py | 7 ++--- > lib/cmdlib/instance.py | 34 +++++++++++++++++++++--- > man/gnt-instance.rst | 14 ++++++++++ > src/Ganeti/Constants.hs | 3 +++ > src/Ganeti/OpCodes.hs | 2 ++ > src/Ganeti/OpParams.hs | 14 ++++++++++ > test/hs/Test/Ganeti/OpCodes.hs | 52 > ++++++++++++++++++++++++++++--------- > test/py/cmdlib/instance_unittest.py | 1 + > 10 files changed, 114 insertions(+), 19 deletions(-) > > diff --git a/lib/backend.py b/lib/backend.py > index 3d51704..dfc8690 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -3266,6 +3266,10 @@ def FinalizeExport(instance, snap_disks): > for name, value in instance.osparams.items(): > config.set(constants.INISECT_OSP, name, str(value)) > > + config.add_section(constants.INISECT_OSP_PRIVATE) > + for name, value in instance.osparams_private.items(): > + config.set(constants.INISECT_OSP_PRIVATE, name, str(value.Get())) > + > utils.WriteFile(utils.PathJoin(destdir, constants.EXPORT_CONF_FILE), > data=config.Dumps()) > shutil.rmtree(finaldestdir, ignore_errors=True) > diff --git a/lib/cli.py b/lib/cli.py > index 32c0f7a..39ef2df 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -2794,6 +2794,8 @@ def GenericInstanceCreate(mode, opts, args): > hvparams=hvparams, > beparams=opts.beparams, > osparams=opts.osparams, > + osparams_private=opts.osparams_private, > + osparams_secret=opts.osparams_secret, > mode=mode, > start=start, > os_type=os_type, > diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py > index 61483ce..b40ae3d 100644 > --- a/lib/client/gnt_instance.py > +++ b/lib/client/gnt_instance.py > @@ -1485,7 +1485,8 @@ add_opts = [ > > commands = { > "add": ( > - AddInstance, [ArgHost(min=1, max=1)], COMMON_CREATE_OPTS + add_opts, > + AddInstance, [ArgHost(min=1, max=1)], > + COMMON_CREATE_OPTS + add_opts + [OSPARAMS_PRIVATE_OPT, > OSPARAMS_SECRET_OPT], > "[...] -t disk-type -n node[:secondary-node] -o os-type <name>", > "Creates and adds a new instance to the cluster"), > "batch-create": ( > @@ -1548,7 +1549,7 @@ commands = { > m_pri_node_opt, m_sec_node_opt, m_clust_opt, m_inst_opt, > m_node_tags_opt, > m_pri_node_tags_opt, m_sec_node_tags_opt, m_inst_tags_opt, > SELECT_OS_OPT] > + SUBMIT_OPTS + [DRY_RUN_OPT, PRIORITY_OPT, OSPARAMS_OPT, > - OSPARAMS_NOLOG_OPT, OSPARAMS_NOLOG_NOSAVE_OPT], > + OSPARAMS_PRIVATE_OPT, OSPARAMS_SECRET_OPT], > "[-f] <instance>", "Reinstall a stopped instance"), > "remove": ( > RemoveInstance, ARGS_ONE_INSTANCE, > @@ -1572,7 +1573,7 @@ commands = { > SetInstanceParams, ARGS_ONE_INSTANCE, > [BACKEND_OPT, DISK_OPT, FORCE_OPT, HVOPTS_OPT, NET_OPT] + SUBMIT_OPTS + > [DISK_TEMPLATE_OPT, SINGLE_NODE_OPT, OS_OPT, FORCE_VARIANT_OPT, > - OSPARAMS_OPT, OSPARAMS_NOLOG_OPT, DRY_RUN_OPT, PRIORITY_OPT, NWSYNC_OPT, > + OSPARAMS_OPT, OSPARAMS_PRIVATE_OPT, DRY_RUN_OPT, PRIORITY_OPT, > NWSYNC_OPT, > OFFLINE_INST_OPT, ONLINE_INST_OPT, IGNORE_IPOLICY_OPT, RUNTIME_MEM_OPT, > NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT, HOTPLUG_OPT, > HOTPLUG_IF_POSSIBLE_OPT], > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > index 4784c52..3e17934 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -37,6 +37,7 @@ from ganeti import masterd > from ganeti import netutils > from ganeti import objects > from ganeti import pathutils > +from ganeti import serializer > import ganeti.rpc.node as rpc > from ganeti import utils > > @@ -847,6 +848,12 @@ class LUInstanceCreate(LogicalUnit): > if name not in self.op.osparams: > self.op.osparams[name] = value > > + if einfo.has_section(constants.INISECT_OSP_PRIVATE): > + # use the parameters, without overriding > + for name, value in einfo.items(constants.INISECT_OSP_PRIVATE): > + if name not in self.op.osparams_private: > + self.op.osparams_private[name] = serializer.Private(value, > descr=name) > + > def _RevertToDefaults(self, cluster): > """Revert the instance parameters to the default values. > > @@ -873,6 +880,12 @@ class LUInstanceCreate(LogicalUnit): > if name in os_defs and os_defs[name] == self.op.osparams[name]: > del self.op.osparams[name] > > + os_defs_ = cluster.SimpleFillOS(self.op.os_type, {}, > + os_params_private={}) > + for name in self.op.osparams_private.keys(): > + if name in os_defs_ and os_defs_[name] == > self.op.osparams_private[name]: > + del self.op.osparams_private[name] > + > def _CalculateFileStorageDir(self): > """Calculate final instance file storage dir. > > @@ -977,7 +990,17 @@ class LUInstanceCreate(LogicalUnit): > self.be_full = _ComputeFullBeParams(self.op, cluster) > > # build os parameters > - self.os_full = cluster.SimpleFillOS(self.op.os_type, self.op.osparams) > + if self.op.osparams_private is None: > + self.op.osparams_private = serializer.PrivateDict() > + if self.op.osparams_secret is None: > + self.op.osparams_secret = serializer.PrivateDict() > + > + self.os_full = cluster.SimpleFillOS( > + self.op.os_type, > + self.op.osparams, > + os_params_private=self.op.osparams_private, > + os_params_secret=self.op.osparams_secret > + ) > > # now that hvp/bep are in final format, let's reset to defaults, > # if told to do so > @@ -1332,6 +1355,7 @@ class LUInstanceCreate(LogicalUnit): > hvparams=self.op.hvparams, > hypervisor=self.op.hypervisor, > osparams=self.op.osparams, > + osparams_private=self.op.osparams_private, > ) > > if self.op.tags: > @@ -1428,7 +1452,9 @@ class LUInstanceCreate(LogicalUnit): > feedback_fn("* running the instance OS create scripts...") > # FIXME: pass debug option from opcode to backend > os_add_result = \ > - self.rpc.call_instance_os_add(self.pnode.uuid, (iobj, None), > False, > + self.rpc.call_instance_os_add(self.pnode.uuid, > + (iobj, self.op.osparams_secret), > + False, > self.op.debug_level) > if pause_sync: > feedback_fn("* resuming disk sync") > @@ -3008,9 +3034,9 @@ class LUInstanceSetParams(LogicalUnit): > hvspecs) > > # osparams processing > - if self.op.osparams or self.op.osparams_private_cluster: > + if self.op.osparams or self.op.osparams_private: > public_parms = self.op.osparams or {} > - private_parms = self.op.osparams_private_cluster or {} > + private_parms = self.op.osparams_private or {} > dupe_keys = utils.GetRepeatedKeys(public_parms, private_parms) > > if dupe_keys: > diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst > index 0b47334..0afd3ed 100644 > --- a/man/gnt-instance.rst > +++ b/man/gnt-instance.rst > @@ -37,6 +37,8 @@ ADD > | [{-B|\--backend-parameters} *BEPARAMS*] > | [{-H|\--hypervisor-parameters} *HYPERVISOR* [: option=*value*... ]] > | [{-O|\--os-parameters} *param*=*value*... ] > +| [--os-parameters-private *param*=*value*... ] > +| [--os-parameters-secret *param*=*value*... ] > | [\--file-storage-dir *dir\_path*] [\--file-driver {loop \| blktap \| > blktap2}] > | {{-n|\--node} *node[:secondary-node]* \| {-I|\--iallocator} *name*} > | {{-o|\--os-type} *os-type*} > @@ -827,6 +829,18 @@ a hypothetical ``dhcp`` parameter to yes can be achieved > by:: > > gnt-instance add -O dhcp=yes ... > > +You can also specify OS parameters that should not be logged but reused > +at the next reinstall with ``--os-parameters-private`` and OS parameters > +that should not be logged or saved to configuration with > +``--os-parameters-secret``. Bear in mind that: > + > + * Launching the daemons in debug mode will cause debug logging to > + happen, which leaks private and secret parameters to the log files. > + Do not use the debug mode in production. Deamons will emit a warning > + on startup if they are in debug mode. > + * You will have to pass again all ``--os-parameters-secret`` parameters > + should you want to reinstall this instance. > + > The ``-I (--iallocator)`` option specifies the instance allocator plugin > to use (``.`` means the default allocator). If you pass in this option > the allocator will select nodes for this instance automatically, so you > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > index 7595283..c9000a9 100644 > --- a/src/Ganeti/Constants.hs > +++ b/src/Ganeti/Constants.hs > @@ -1104,6 +1104,9 @@ inisectIns = "instance" > inisectOsp :: String > inisectOsp = "os" > > +inisectOspPrivate :: String > +inisectOspPrivate = "os_private" > + > -- * Dynamic device modification > > ddmAdd :: String > diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs > index 29a9a43..9dfc1ac 100644 > --- a/src/Ganeti/OpCodes.hs > +++ b/src/Ganeti/OpCodes.hs > @@ -438,6 +438,8 @@ $(genOpCode "OpCode" > , pInstNics > , pNoInstall > , pInstOsParams > + , pInstOsParamsPrivate > + , pInstOsParamsSecret > , pInstOs > , pPrimaryNode > , pPrimaryNodeUuid > diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs > index efb5b90..3da9530 100644 > --- a/src/Ganeti/OpParams.hs > +++ b/src/Ganeti/OpParams.hs > @@ -120,6 +120,8 @@ module Ganeti.OpParams > , pClusterOsParams > , pClusterOsParamsPrivate > , pInstOsParams > + , pInstOsParamsPrivate > + , pInstOsParamsSecret > , pCandidatePoolSize > , pMaxRunningJobs > , pUidPool > @@ -1098,6 +1100,18 @@ pInstOsParams = > defaultField [| toJSObject [] |] $ > simpleField "osparams" [t| JSObject JSValue |] > > +pInstOsParamsPrivate :: Field > +pInstOsParamsPrivate = > + withDoc "Private OS parameters for instance" . > + optionalField $ > + simpleField "osparams_private" [t| JSObject (Private JSValue) |] > + > +pInstOsParamsSecret :: Field > +pInstOsParamsSecret = > + withDoc "Secret OS parameters for instance" . > + optionalField $ > + simpleField "osparams_secret" [t| JSObject (Private JSValue) |] > + > pPrimaryNode :: Field > pPrimaryNode = > withDoc "Primary node for an instance" $ > diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.hs > index e79b691..3deddad 100644 > --- a/test/hs/Test/Ganeti/OpCodes.hs > +++ b/test/hs/Test/Ganeti/OpCodes.hs > @@ -255,18 +255,46 @@ instance Arbitrary OpCodes.OpCode where > return Nothing <*> genMaybe genNodeNameNE <*> return Nothing <*> > genMaybe genNameNE <*> arbitrary > "OP_INSTANCE_CREATE" -> > - OpCodes.OpInstanceCreate <$> genFQDN <*> arbitrary <*> > - arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> > - pure emptyJSObject <*> arbitrary <*> arbitrary <*> arbitrary <*> > - genMaybe genNameNE <*> pure emptyJSObject <*> arbitrary <*> > - genMaybe genNameNE <*> arbitrary <*> arbitrary <*> arbitrary <*> > - arbitrary <*> arbitrary <*> arbitrary <*> pure emptyJSObject <*> > - genMaybe genNameNE <*> genMaybe genNodeNameNE <*> return Nothing > <*> > - genMaybe genNodeNameNE <*> return Nothing <*> genMaybe (pure []) > <*> > - genMaybe genNodeNameNE <*> arbitrary <*> genMaybe genNodeNameNE <*> > - return Nothing <*> genMaybe genNodeNameNE <*> genMaybe genNameNE > <*> > - arbitrary <*> arbitrary <*> (genTags >>= mapM mkNonEmpty) <*> > - arbitrary > + OpCodes.OpInstanceCreate > + <$> genFQDN -- instance_name > + <*> arbitrary -- force_variant > + <*> arbitrary -- wait_for_sync > + <*> arbitrary -- name_check > + <*> arbitrary -- ignore_ipolicy > + <*> arbitrary -- opportunistic_locking > + <*> pure emptyJSObject -- beparams > + <*> arbitrary -- disks > + <*> arbitrary -- disk_template > + <*> arbitrary -- file_driver > + <*> genMaybe genNameNE -- file_storage_dir > + <*> pure emptyJSObject -- hvparams > + <*> arbitrary -- hypervisor > + <*> genMaybe genNameNE -- iallocator > + <*> arbitrary -- identify_defaults > + <*> arbitrary -- ip_check > + <*> arbitrary -- conflicts_check > + <*> arbitrary -- mode > + <*> arbitrary -- nics > + <*> arbitrary -- no_install > + <*> pure emptyJSObject -- osparams > + <*> genMaybe arbitraryPrivateJSObj -- osparams_private > + <*> genMaybe arbitraryPrivateJSObj -- osparams_secret > + <*> genMaybe genNameNE -- os_type > + <*> genMaybe genNodeNameNE -- pnode > + <*> return Nothing -- pnode_uuid > + <*> genMaybe genNodeNameNE -- snode > + <*> return Nothing -- snode_uuid > + <*> genMaybe (pure []) -- source_handshake > + <*> genMaybe genNodeNameNE -- source_instance_name > + <*> arbitrary -- source_shutdown_timeout > + <*> genMaybe genNodeNameNE -- source_x509_ca > + <*> return Nothing -- src_node > + <*> genMaybe genNodeNameNE -- src_node_uuid > + <*> genMaybe genNameNE -- src_path > + <*> arbitrary -- compress > + <*> arbitrary -- start > + <*> (genTags >>= mapM mkNonEmpty) -- tags > + <*> arbitrary -- instance_communication > "OP_INSTANCE_MULTI_ALLOC" -> > OpCodes.OpInstanceMultiAlloc <$> arbitrary <*> genMaybe genNameNE <*> > pure [] > diff --git a/test/py/cmdlib/instance_unittest.py > b/test/py/cmdlib/instance_unittest.py > index 23605d3..3fe3ee7 100644 > --- a/test/py/cmdlib/instance_unittest.py > +++ b/test/py/cmdlib/instance_unittest.py > @@ -465,6 +465,7 @@ class TestLUInstanceCreate(CmdlibTestCase): > osparams={ > self.os_name_variant: {} > }, > + osparams_private={}, > identify_defaults=True) > 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
