[PATCH] add mechanism for general purpose parameter passing to Image Factory

[Ian McLeod]

We originally added semi-generic parameter passing for OVA support and
then overloaded it for Vagrant support, before breaking that back out
as specific image types.

This change adds a fairly generic mechanism for parameter passthrough
to plugins and the base builder that does not require patching the
builder and CLI each time a new class of parameter is added to Factory.

The immediate reason for doing this is to expose the Docker plugin's
ability to set custom labels, environments and commands, but it is
likely to be useful for other things in the future.
---
 builder/kojid | 39 ++++++++++++++++++++++++++++++++++-----
 cli/koji      | 15 ++++++++++++++-
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/builder/kojid b/builder/kojid
index 4ea93f2..ad427cd 100755
--- a/builder/kojid
+++ b/builder/kojid
@@ -3178,6 +3178,25 @@ class BaseImageTask(OzImageTask):
             raise koji.ApplianceError('Image status is %s: %s' %
                 (status, details))
 
+    def _mergeFactoryParams(self, img_opts, fixed_params):
+        """
+        Merge any KV pairs passed in via --factory-parameter CLI options
+        into an existing dictionary that will eventually be passed to
+        factory build commands.  This allows a fairly generic mechanism
+        for parameter passthrough to plugins and the base builder that does
+        not require patching the builder and CLI each time.
+
+        @args:
+            img_opts - an existing dict with any pre-existing parameters
+            fixed_params - list of parameters that must not be overriden
+        @returns:
+            nothing - dict is modified in place
+        """
+        if self.opts.get('factory_parameter'):
+            for kvp in self.opts.get('factory_parameter'):
+                if kvp[0] not in fixed_params:
+                    img_opts[kvp[0]] = kvp[1]
+
     def _buildBase(self, template, params, wait=True):
         """
         Build a base image using ImageFactory. This is a "raw" image.
@@ -3194,7 +3213,10 @@ class BaseImageTask(OzImageTask):
         #       may need to still upload ozlog and remove the log handler
         self.logger.info('dispatching a baseimg builder')
         self.logger.debug('templates: %s' % template)
-        self.logger.debug('params: %s' % params)
+        self.logger.debug('pre-merge params: %s' % params)
+        # We enforce various things related to the ks file - do not allow override
+        self._mergeFactoryParams(params, [ 'install_script' ])
+        self.logger.debug('post-merge params: %s' % params)
         base = self.bd.builder_for_base_image(template, parameters=params)
         if wait:
             base.base_thread.join()
@@ -3245,19 +3267,22 @@ class BaseImageTask(OzImageTask):
         if format == 'vagrant-virtualbox':
             format = 'vsphere-ova'
             img_opts['vsphere_ova_format'] = 'vagrant-virtualbox'
+            fixed_params = [ 'vsphere_ova_format' ]
         if format == 'vagrant-libvirt':
             format = 'rhevm-ova'
             img_opts['rhevm_ova_format'] = 'vagrant-libvirt'
+            fixed_params = [ 'rhevm_ova_format' ]
         if format == 'vagrant-vmware-fusion':
             format = 'vsphere-ova'
             img_opts['vsphere_ova_format'] = 'vagrant-vmware-fusion'
             # The initial disk image transform for VMWare Fusion/Workstation requires a
"standard" VMDK
             # not the stream oriented format used for VirtualBox or regular VMWare OVAs
             img_opts['vsphere_vmdk_format'] = 'standard'
+            fixed_params = [ 'vsphere_ova_format', 'vsphere_vmdk_format'
]
         targ = self._do_target_image(self.base_img.base_image.identifier,
-            format.replace('-ova', ''), img_opts=img_opts)
+            format.replace('-ova', ''), img_opts=img_opts,
fixed_params=fixed_params)
         targ2 = self._do_target_image(targ.target_image.identifier, 'OVA',
-            img_opts=img_opts)
+            img_opts=img_opts, fixed_params=fixed_params)
         return {'image': targ2.target_image.data}
 
     def _buildDocker(self, format):
@@ -3275,7 +3300,7 @@ class BaseImageTask(OzImageTask):
             'docker', img_opts=img_opts)
         return {'image': targ.target_image.data}
 
-    def _do_target_image(self, base_id, image_type, img_opts={}):
+    def _do_target_image(self, base_id, image_type, img_opts={}, fixed_params=[]):
         """
         A generic method for building what ImageFactory calls "target images".
         These are images based on a raw disk that was built before using the
@@ -3287,6 +3312,8 @@ class BaseImageTask(OzImageTask):
                 uses this to figure out what plugin to run
             img_opts - a dict of additional options that specific to the target
                 type we pass in via image_type
+            fixed_params - a list of parameter keys that should not be overriden
+                           by the --factory-parameter CLI
         @returns:
             A Builder() object from ImageFactory that contains information
             about the image building include state and progress.
@@ -3294,7 +3321,9 @@ class BaseImageTask(OzImageTask):
         # TODO: test the failure case where IF itself throws an exception
         #       ungracefully (missing a plugin for example)
         #       may need to still upload ozlog and remove the log handler
-        self.logger.debug('img_opts: %s' % img_opts)
+        self.logger.debug('img_opts_pre_merge: %s' % img_opts)
+        self._mergeFactoryParams(img_opts, fixed_params)
+        self.logger.debug('img_opts_post_merge: %s' % img_opts)
         target = self.bd.builder_for_target_image(image_type,
             image_id=base_id, template=None, parameters=img_opts)
         target.target_thread.join()
diff --git a/cli/koji b/cli/koji
index 64c5356..cbfa2d0 100755
--- a/cli/koji
+++ b/cli/koji
@@ -5306,6 +5306,11 @@ def handle_image_build(options, session, args):
     parser.add_option("--ova-option", action="append",
         help=_("Override a value in the OVA description XML. Provide a value "
+
                "in a name=value format, such as
'ovf_memory_mb=6144'"))
+    parser.add_option("--factory-parameter", nargs=2,
action="append",
+        help=_("Pass a parameter to Image Factory. The results are highly specific
" +
+               "to the image format being created. This is a two argument parameter
" +
+               "that can be specified an arbitrary number of times. For example:
"
+               "--factory-parameter docker_cmd '[ \"/bin/echo Hello
World\" ]'"))
     parser.add_option("--release", help=_("Forcibly set the release
field"))
     parser.add_option("--repo", action="append",
         help=_("Specify a repo that will override the repo used to install " +
@@ -5355,6 +5360,14 @@ def handle_image_build(options, session, args):
             for k, v in config.items(section):
                 task_options.ova_option.append('%s=%s' % (k, v))
 
+        # as do factory-parameters
+        section = 'factory-parameters'
+        if config.has_section(section):
+            task_options.factory_parameter = [ ]
+            for k, v in config.items(section):
+                # We do this, rather than a dict, to match what argparse spits out
+                task_options.factory_parameter.append( (k, v) )
+
     else:
         if len(args) < 5:
             parser.error(_("At least five arguments are required: a name, " +
@@ -5483,7 +5496,7 @@ def _build_image_oz(options, task_opts, session, args):
     hub_opts = {}
     for opt in ('ksurl', 'ksversion', 'kickstart',
'scratch', 'repo',
                 'release', 'skip_tag', 'specfile',
'distro', 'format',
-                'disk_size', 'ova_option'):
+                'disk_size', 'ova_option', 'factory_parameter'):
         val = getattr(task_opts, opt, None)
         if val is not None:
             hub_opts[opt] = val
-- 
2.1.0

[Mike McLean]

On 10/27/2015 11:30 PM, Ian McLeod wrote:
>  We originally added semi-generic parameter passing for OVA support
> and
>  then overloaded it for Vagrant support, before breaking that back out
>  as specific image types.
>  
>  This change adds a fairly generic mechanism for parameter passthrough
>  to plugins and the base builder that does not require patching the
>  builder and CLI each time a new class of parameter is added to Factory.
>  
>  The immediate reason for doing this is to expose the Docker plugin's
>  ability to set custom labels, environments and commands, but it is
>  likely to be useful for other things in the future. 
The mutable default args in _do_target_image jump out at me. (One was
there before, the other has been added). Currently we never call the
function without either arg, so the code as-is won't hit that particular
python gotcha.  Still, as long as we're touching this bit of code, we
should fix that to avoid future problems.

Other than that, looks sane.  I'll pull in and tweak the args.

>  ---
>   builder/kojid | 39 ++++++++++++++++++++++++++++++++++-----
>   cli/koji      | 15 ++++++++++++++-
>   2 files changed, 48 insertions(+), 6 deletions(-)
>  
>  diff --git a/builder/kojid b/builder/kojid
>  index 4ea93f2..ad427cd 100755
>  --- a/builder/kojid
>  +++ b/builder/kojid
>  @@ -3178,6 +3178,25 @@ class BaseImageTask(OzImageTask):
>               raise koji.ApplianceError('Image status is %s: %s' %
>                   (status, details))
>   
>  +    def _mergeFactoryParams(self, img_opts, fixed_params):
>  +        """
>  +        Merge any KV pairs passed in via --factory-parameter CLI options
>  +        into an existing dictionary that will eventually be passed to
>  +        factory build commands.  This allows a fairly generic mechanism
>  +        for parameter passthrough to plugins and the base builder that does
>  +        not require patching the builder and CLI each time.
>  +
>  +        @args:
>  +            img_opts - an existing dict with any pre-existing parameters
>  +            fixed_params - list of parameters that must not be overriden
>  +        @returns:
>  +            nothing - dict is modified in place
>  +        """
>  +        if self.opts.get('factory_parameter'):
>  +            for kvp in self.opts.get('factory_parameter'):
>  +                if kvp[0] not in fixed_params:
>  +                    img_opts[kvp[0]] = kvp[1]
>  +
>       def _buildBase(self, template, params, wait=True):
>           """
>           Build a base image using ImageFactory. This is a "raw" image.
>  @@ -3194,7 +3213,10 @@ class BaseImageTask(OzImageTask):
>           #       may need to still upload ozlog and remove the log handler
>           self.logger.info('dispatching a baseimg builder')
>           self.logger.debug('templates: %s' % template)
>  -        self.logger.debug('params: %s' % params)
>  +        self.logger.debug('pre-merge params: %s' % params)
>  +        # We enforce various things related to the ks file - do not allow override
>  +        self._mergeFactoryParams(params, [ 'install_script' ])
>  +        self.logger.debug('post-merge params: %s' % params)
>           base = self.bd.builder_for_base_image(template, parameters=params)
>           if wait:
>               base.base_thread.join()
>  @@ -3245,19 +3267,22 @@ class BaseImageTask(OzImageTask):
>           if format == 'vagrant-virtualbox':
>               format = 'vsphere-ova'
>               img_opts['vsphere_ova_format'] = 'vagrant-virtualbox'
>  +            fixed_params = [ 'vsphere_ova_format' ]
>           if format == 'vagrant-libvirt':
>               format = 'rhevm-ova'
>               img_opts['rhevm_ova_format'] = 'vagrant-libvirt'
>  +            fixed_params = [ 'rhevm_ova_format' ]
>           if format == 'vagrant-vmware-fusion':
>               format = 'vsphere-ova'
>               img_opts['vsphere_ova_format'] = 'vagrant-vmware-fusion'
>               # The initial disk image transform for VMWare Fusion/Workstation requires a
> "standard" VMDK
>               # not the stream oriented format used for VirtualBox or regular VMWare OVAs
>               img_opts['vsphere_vmdk_format'] = 'standard'
>  +            fixed_params = [ 'vsphere_ova_format', 'vsphere_vmdk_format'
> ]
>           targ = self._do_target_image(self.base_img.base_image.identifier,
>  -            format.replace('-ova', ''), img_opts=img_opts)
>  +            format.replace('-ova', ''), img_opts=img_opts,
> fixed_params=fixed_params)
>           targ2 = self._do_target_image(targ.target_image.identifier, 'OVA',
>  -            img_opts=img_opts)
>  +            img_opts=img_opts, fixed_params=fixed_params)
>           return {'image': targ2.target_image.data}
>   
>       def _buildDocker(self, format):
>  @@ -3275,7 +3300,7 @@ class BaseImageTask(OzImageTask):
>               'docker', img_opts=img_opts)
>           return {'image': targ.target_image.data}
>   
>  -    def _do_target_image(self, base_id, image_type, img_opts={}):
>  +    def _do_target_image(self, base_id, image_type, img_opts={}, fixed_params=[]):
>           """
>           A generic method for building what ImageFactory calls "target
> images".
>           These are images based on a raw disk that was built before using the
>  @@ -3287,6 +3312,8 @@ class BaseImageTask(OzImageTask):
>                   uses this to figure out what plugin to run
>               img_opts - a dict of additional options that specific to the target
>                   type we pass in via image_type
>  +            fixed_params - a list of parameter keys that should not be overriden
>  +                           by the --factory-parameter CLI
>           @returns:
>               A Builder() object from ImageFactory that contains information
>               about the image building include state and progress.
>  @@ -3294,7 +3321,9 @@ class BaseImageTask(OzImageTask):
>           # TODO: test the failure case where IF itself throws an exception
>           #       ungracefully (missing a plugin for example)
>           #       may need to still upload ozlog and remove the log handler
>  -        self.logger.debug('img_opts: %s' % img_opts)
>  +        self.logger.debug('img_opts_pre_merge: %s' % img_opts)
>  +        self._mergeFactoryParams(img_opts, fixed_params)
>  +        self.logger.debug('img_opts_post_merge: %s' % img_opts)
>           target = self.bd.builder_for_target_image(image_type,
>               image_id=base_id, template=None, parameters=img_opts)
>           target.target_thread.join()
>  diff --git a/cli/koji b/cli/koji
>  index 64c5356..cbfa2d0 100755
>  --- a/cli/koji
>  +++ b/cli/koji
>  @@ -5306,6 +5306,11 @@ def handle_image_build(options, session, args):
>       parser.add_option("--ova-option", action="append",
>           help=_("Override a value in the OVA description XML. Provide a value "
> +
>                  "in a name=value format, such as
> 'ovf_memory_mb=6144'"))
>  +    parser.add_option("--factory-parameter", nargs=2,
> action="append",
>  +        help=_("Pass a parameter to Image Factory. The results are highly specific
> " +
>  +               "to the image format being created. This is a two argument parameter
> " +
>  +               "that can be specified an arbitrary number of times. For example:
> "
>  +               "--factory-parameter docker_cmd '[ \"/bin/echo Hello
> World\" ]'"))
>       parser.add_option("--release", help=_("Forcibly set the release
> field"))
>       parser.add_option("--repo", action="append",
>           help=_("Specify a repo that will override the repo used to install "
> +
>  @@ -5355,6 +5360,14 @@ def handle_image_build(options, session, args):
>               for k, v in config.items(section):
>                   task_options.ova_option.append('%s=%s' % (k, v))
>   
>  +        # as do factory-parameters
>  +        section = 'factory-parameters'
>  +        if config.has_section(section):
>  +            task_options.factory_parameter = [ ]
>  +            for k, v in config.items(section):
>  +                # We do this, rather than a dict, to match what argparse spits out
>  +                task_options.factory_parameter.append( (k, v) )
>  +
>       else:
>           if len(args) < 5:
>               parser.error(_("At least five arguments are required: a name, " +
>  @@ -5483,7 +5496,7 @@ def _build_image_oz(options, task_opts, session, args):
>       hub_opts = {}
>       for opt in ('ksurl', 'ksversion', 'kickstart',
> 'scratch', 'repo',
>                   'release', 'skip_tag', 'specfile',
> 'distro', 'format',
>  -                'disk_size', 'ova_option'):
>  +                'disk_size', 'ova_option',
> 'factory_parameter'):
>           val = getattr(task_opts, opt, None)
>           if val is not None:
>               hub_opts[opt] = val
>

[PATCH] remove mutable default args in _do_target_image

[Mike McLean]

---
 builder/kojid | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builder/kojid b/builder/kojid
index ad427cd..d0ab9de 100755
--- a/builder/kojid
+++ b/builder/kojid
@@ -3300,7 +3300,7 @@ class BaseImageTask(OzImageTask):
             'docker', img_opts=img_opts)
         return {'image': targ.target_image.data}
 
-    def _do_target_image(self, base_id, image_type, img_opts={}, fixed_params=[]):
+    def _do_target_image(self, base_id, image_type, img_opts=None, fixed_params=None):
         """
         A generic method for building what ImageFactory calls "target images".
         These are images based on a raw disk that was built before using the
@@ -3321,6 +3321,10 @@ class BaseImageTask(OzImageTask):
         # TODO: test the failure case where IF itself throws an exception
         #       ungracefully (missing a plugin for example)
         #       may need to still upload ozlog and remove the log handler
+        if img_opts is None:
+            img_opts = {}
+        if fixed_params is None:
+            fixed_params = []
         self.logger.debug('img_opts_pre_merge: %s' % img_opts)
         self._mergeFactoryParams(img_opts, fixed_params)
         self.logger.debug('img_opts_post_merge: %s' % img_opts)
-- 
2.1.0