kubevirt_vm: Improve create VM from template

changelogs/fragments/kubevirt_vm_fix_craete_template.yml

@@ -0,0 +1,3 @@
+---
+minor_changes:
+  - Improve creating VM from template. Merge VM disks/interfaces with the template defaults.

lib/ansible/module_utils/kubevirt.py

@@ -7,9 +7,11 @@
 from collections import defaultdict
 from distutils.version import Version
 
+from ansible.module_utils.common._collections_compat import Sequence
 from ansible.module_utils.k8s.common import list_dict_str
 from ansible.module_utils.k8s.raw import KubernetesRawModule
 
+import copy
 import re
 
 MAX_SUPPORTED_API_VERSION = 'v1alpha3'
@@ -126,21 +128,25 @@ def __init__(self, *args, **kwargs):
         super(KubeVirtRawModule, self).__init__(*args, **kwargs)
 
     @staticmethod
-    def merge_dicts(x, y):
+    def merge_dicts(base_dict, merging_dicts):
+        """This function merges a base dictionary with one or more other dictionaries.
+        The base dictionary takes precedence when there is a key collision.
+        merging_dicts can be a dict or a list or tuple of dicts.  In the latter case, the
+        dictionaries at the front of the list have higher precedence over the ones at the end.
         """
-        This function merge two dictionaries, where the first dict has
-        higher priority in merging two same keys.
-        """
-        for k in set(x.keys()).union(y.keys()):
-            if k in x and k in y:
-                if isinstance(x[k], dict) and isinstance(y[k], dict):
-                    yield (k, dict(KubeVirtRawModule.merge_dicts(x[k], y[k])))
-                else:
-                    yield (k, y[k])
-            elif k in x:
-                yield (k, x[k])
-            else:
-                yield (k, y[k])
+        if not merging_dicts:
+            merging_dicts = ({},)
+
+        if not isinstance(merging_dicts, Sequence):
+            merging_dicts = (merging_dicts,)
+
+        new_dict = {}
+        for d in reversed(merging_dicts):
+            new_dict.update(d)
+
+        new_dict.update(base_dict)
+
+        return new_dict
 
     def get_resource(self, resource):
         try:
@@ -215,16 +221,23 @@ def _define_cloud_init(self, cloud_init_nocloud, template_spec):
                 'disk': {'bus': 'virtio'},
             })
 
-    def _define_interfaces(self, interfaces, template_spec):
+    def _define_interfaces(self, interfaces, template_spec, defaults):
         """
         Takes interfaces parameter of Ansible and create kubevirt API interfaces
         and networks strucutre out from it.
         """
+        if not interfaces and defaults and 'interfaces' in defaults:
+            interfaces = copy.deepcopy(defaults['interfaces'])
+            for d in interfaces:
+                d['network'] = defaults['networks'][0]
+
         if interfaces:
             # Extract interfaces k8s specification from interfaces list passed to Ansible:
             spec_interfaces = []
             for i in interfaces:
-                spec_interfaces.append(dict((k, v) for k, v in i.items() if k != 'network'))
+                spec_interfaces.append(
+                    self.merge_dicts(dict((k, v) for k, v in i.items() if k != 'network'), defaults['interfaces'])
+                )
             if 'interfaces' not in template_spec['domain']['devices']:
                 template_spec['domain']['devices']['interfaces'] = []
             template_spec['domain']['devices']['interfaces'].extend(spec_interfaces)
@@ -234,21 +247,28 @@ def _define_interfaces(self, interfaces, template_spec):
             for i in interfaces:
                 net = i['network']
                 net['name'] = i['name']
-                spec_networks.append(net)
+                spec_networks.append(self.merge_dicts(net, defaults['networks']))
             if 'networks' not in template_spec:
                 template_spec['networks'] = []
             template_spec['networks'].extend(spec_networks)
 
-    def _define_disks(self, disks, template_spec):
+    def _define_disks(self, disks, template_spec, defaults):
         """
         Takes disks parameter of Ansible and create kubevirt API disks and
         volumes strucutre out from it.
         """
+        if not disks and defaults and 'disks' in defaults:
+            disks = copy.deepcopy(defaults['disks'])
+            for d in disks:
+                d['volume'] = defaults['volumes'][0]
+
         if disks:
             # Extract k8s specification from disks list passed to Ansible:
             spec_disks = []
             for d in disks:
-                spec_disks.append(dict((k, v) for k, v in d.items() if k != 'volume'))
+                spec_disks.append(
+                    self.merge_dicts(dict((k, v) for k, v in d.items() if k != 'volume'), defaults['disks'])
+                )
             if 'disks' not in template_spec['domain']['devices']:
                 template_spec['domain']['devices']['disks'] = []
             template_spec['domain']['devices']['disks'].extend(spec_disks)
@@ -258,7 +278,7 @@ def _define_disks(self, disks, template_spec):
             for d in disks:
                 volume = d['volume']
                 volume['name'] = d['name']
-                spec_volumes.append(volume)
+                spec_volumes.append(self.merge_dicts(volume, defaults['volumes']))
             if 'volumes' not in template_spec:
                 template_spec['volumes'] = []
             template_spec['volumes'].extend(spec_volumes)
@@ -274,7 +294,7 @@ def find_supported_resource(self, kind):
         self.fail("API versions {0} are too recent. Max supported is {1}/{2}.".format(
             str([r.api_version for r in sr]), API_GROUP, MAX_SUPPORTED_API_VERSION))
 
-    def _construct_vm_definition(self, kind, definition, template, params):
+    def _construct_vm_definition(self, kind, definition, template, params, defaults=None):
         self.client = self.get_api_client()
 
         disks = params.get('disks', [])
@@ -328,7 +348,7 @@ def _construct_vm_definition(self, kind, definition, template, params):
             template_spec['domain']['cpu']['model'] = cpu_model
 
         if labels:
-            template['metadata']['labels'] = dict(self.merge_dicts(labels, template['metadata']['labels']))
+            template['metadata']['labels'] = self.merge_dicts(labels, template['metadata']['labels'])
 
         if machine_type:
             template_spec['domain']['machine']['type'] = machine_type
@@ -343,26 +363,23 @@ def _construct_vm_definition(self, kind, definition, template, params):
             template_spec['domain']['devices']['autoattachGraphicsDevice'] = not headless
 
         # Define disks
-        self._define_disks(disks, template_spec)
+        self._define_disks(disks, template_spec, defaults)
 
         # Define cloud init disk if defined:
         # Note, that this must be called after _define_disks, so the cloud_init
         # is not first in order and it's not used as boot disk:
         self._define_cloud_init(cloud_init_nocloud, template_spec)
 
         # Define interfaces:
-        self._define_interfaces(interfaces, template_spec)
+        self._define_interfaces(interfaces, template_spec, defaults)
 
         # Define datavolumes:
         self._define_datavolumes(datavolumes, definition['spec'])
 
-        # Perform create/absent action:
-        definition = dict(self.merge_dicts(self.resource_definitions[0], definition))
-        resource = self.find_supported_resource(kind)
-        return dict(self.merge_dicts(self.resource_definitions[0], definition))
+        return self.merge_dicts(definition, self.resource_definitions[0])
 
-    def construct_vm_definition(self, kind, definition, template):
-        definition = self._construct_vm_definition(kind, definition, template, self.params)
+    def construct_vm_definition(self, kind, definition, template, defaults=None):
+        definition = self._construct_vm_definition(kind, definition, template, self.params, defaults)
         resource = self.find_supported_resource(kind)
         definition = self.set_defaults(resource, definition)
         return resource, definition

lib/ansible/modules/cloud/kubevirt/kubevirt_preset.py

@@ -129,8 +129,11 @@ def execute_module(self):
         # attributes there, remove when we do:
         definition['spec']['domain']['devices'] = dict()
 
+        # defaults for template
+        defaults = {'disks': [], 'volumes': [], 'interfaces': [], 'networks': []}
+
         # Execute the CURD of VM:
-        dummy, definition = self.construct_vm_definition(KIND, definition, definition)
+        dummy, definition = self.construct_vm_definition(KIND, definition, definition, defaults)
         result_crud = self.execute_crud(KIND, definition)
         changed = result_crud['changed']
         result = result_crud.pop('result')

lib/ansible/modules/cloud/kubevirt/kubevirt_pvc.py

@@ -435,7 +435,7 @@ def execute_module(self):
             spec['volumeName'] = self.params.get('volume_name')
 
         # 'resource_definition:' has lower priority than module parameters
-        definition = dict(KubeVirtRawModule.merge_dicts(self.resource_definitions[0], definition))
+        definition = KubeVirtRawModule.merge_dicts(self.resource_definitions[0], definition)
 
         self.client = self.get_api_client()
         resource = self.find_resource(KIND, API, fail=True)

lib/ansible/modules/cloud/kubevirt/kubevirt_rs.py

@@ -173,9 +173,12 @@ def execute_module(self):
         if replicas is not None:
             definition['spec']['replicas'] = replicas
 
+        # defaults for template
+        defaults = {'disks': [], 'volumes': [], 'interfaces': [], 'networks': []}
+
         # Execute the CURD of VM:
         template = definition['spec']['template']
-        dummy, definition = self.construct_vm_definition(KIND, definition, template)
+        dummy, definition = self.construct_vm_definition(KIND, definition, template, defaults)
         result_crud = self.execute_crud(KIND, definition)
         changed = result_crud['changed']
         result = result_crud.pop('result')

lib/ansible/modules/cloud/kubevirt/kubevirt_vm.py

@@ -217,8 +217,6 @@
 import copy
 import traceback
 
-from ansible.module_utils.k8s.common import AUTH_ARG_SPEC
-
 from ansible.module_utils.k8s.common import AUTH_ARG_SPEC
 from ansible.module_utils.kubevirt import (
     virtdict,
@@ -317,11 +315,31 @@ def manage_vm_state(self, new_state, already_changed):
 
         return changed, k8s_obj
 
+    def _process_template_defaults(self, proccess_template, processedtemplate, defaults):
+        def set_template_default(default_name, default_name_index, definition_spec):
+            default_value = proccess_template['metadata']['annotations'][default_name]
+            if default_value:
+                values = definition_spec[default_name_index]
+                default_values = [d for d in values if d.get('name') == default_value]
+                defaults[default_name_index] = default_values
+                if definition_spec[default_name_index] is None:
+                    definition_spec[default_name_index] = []
+                definition_spec[default_name_index].extend([d for d in values if d.get('name') != default_value])
+
+        devices = processedtemplate['spec']['template']['spec']['domain']['devices']
+        spec = processedtemplate['spec']['template']['spec']
+
+        set_template_default('defaults.template.cnv.io/disk', 'disks', devices)
+        set_template_default('defaults.template.cnv.io/volume', 'volumes', spec)
+        set_template_default('defaults.template.cnv.io/nic', 'interfaces', devices)
+        set_template_default('defaults.template.cnv.io/network', 'networks', spec)
+
     def construct_definition(self, kind, our_state, ephemeral):
         definition = virtdict()
         processedtemplate = {}
 
         # Construct the API object definition:
+        defaults = {'disks': [], 'volumes': [], 'interfaces': [], 'networks': []}
         vm_template = self.params.get('template')
         if vm_template:
             # Find the template the VM should be created from:
@@ -338,14 +356,16 @@ def construct_definition(self, kind, our_state, ephemeral):
             processedtemplates_res = self.client.resources.get(api_version='template.openshift.io/v1', kind='Template', name='processedtemplates')
             processedtemplate = processedtemplates_res.create(proccess_template.to_dict()).to_dict()['objects'][0]
 
+            # Process defaults of the template:
+            self._process_template_defaults(proccess_template, processedtemplate, defaults)
+
         if not ephemeral:
             definition['spec']['running'] = our_state == 'running'
         template = definition if ephemeral else definition['spec']['template']
         template['metadata']['labels']['vm.cnv.io/name'] = self.params.get('name')
-        dummy, definition = self.construct_vm_definition(kind, definition, template)
-        definition = dict(self.merge_dicts(processedtemplate, definition))
+        dummy, definition = self.construct_vm_definition(kind, definition, template, defaults)
 
-        return definition
+        return self.merge_dicts(definition, processedtemplate)
 
     def execute_module(self):
         # Parse parameters specific to this module:
@@ -370,16 +390,18 @@ def execute_module(self):
             if our_state != 'absent':
                 self.params['state'] = k8s_state = 'present'
 
+        # Start with fetching the current object to make sure it exists
+        # If it does, but we end up not performing any operations on it, at least we'll be able to return
+        # its current contents as part of the final json
         self.client = self.get_api_client()
         self._kind_resource = self.find_supported_resource(kind)
         k8s_obj = self.get_resource(self._kind_resource)
         if not self.check_mode and not vm_spec_change and k8s_state != 'absent' and not k8s_obj:
             self.fail("It's impossible to create an empty VM or change state of a non-existent VM.")
 
-        # Changes in VM's spec or any changes to VMIs warrant a full CRUD, the latter because
-        # VMIs don't really have states to manage; they're either present or don't exist
+        # If there are (potential) changes to `spec:` or we want to delete the object, that warrants a full CRUD
         # Also check_mode always warrants a CRUD, as that'll produce a sane result
-        if vm_spec_change or ephemeral or k8s_state == 'absent' or self.check_mode:
+        if vm_spec_change or k8s_state == 'absent' or self.check_mode:
             definition = self.construct_definition(kind, our_state, ephemeral)
             result = self.execute_crud(kind, definition)
             changed = result['changed']

test/units/modules/cloud/kubevirt/test_kubevirt_vm.py

@@ -136,18 +136,18 @@ def test_simple_merge_dicts(self):
     dict1 = {'labels': {'label1': 'value'}}
     dict2 = {'labels': {'label2': 'value'}}
     dict3 = json.dumps({'labels': {'label1': 'value', 'label2': 'value'}}, sort_keys=True)
-    assert dict3 == json.dumps(dict(mymodule.KubeVirtVM.merge_dicts(dict1, dict2)), sort_keys=True)
+    assert dict3 == json.dumps(mymodule.KubeVirtVM.merge_dicts(dict1, dict2), sort_keys=True)
 
 
 def test_simple_multi_merge_dicts(self):
     dict1 = {'labels': {'label1': 'value', 'label3': 'value'}}
     dict2 = {'labels': {'label2': 'value'}}
     dict3 = json.dumps({'labels': {'label1': 'value', 'label2': 'value', 'label3': 'value'}}, sort_keys=True)
-    assert dict3 == json.dumps(dict(mymodule.KubeVirtVM.merge_dicts(dict1, dict2)), sort_keys=True)
+    assert dict3 == json.dumps(mymodule.KubeVirtVM.merge_dicts(dict1, dict2), sort_keys=True)
 
 
 def test_double_nested_merge_dicts(self):
     dict1 = {'metadata': {'labels': {'label1': 'value', 'label3': 'value'}}}
     dict2 = {'metadata': {'labels': {'label2': 'value'}}}
     dict3 = json.dumps({'metadata': {'labels': {'label1': 'value', 'label2': 'value', 'label3': 'value'}}}, sort_keys=True)
-    assert dict3 == json.dumps(dict(mymodule.KubeVirtVM.merge_dicts(dict1, dict2)), sort_keys=True)
+    assert dict3 == json.dumps(mymodule.KubeVirtVM.merge_dicts(dict1, dict2), sort_keys=True)