docker_container: fix various idempotency problems and non-working options (#45905)

* Sorting args.

* Doing comparisons of options with container parameters in a more context-sensitive way.

This prevents unnecessary restarts, or missing restarts (f.ex. if parameters are removed from ``cmd``).

* Make blkio_weight work.

* Fix cap_drop idempotency problem.

* Making groups idempotent if it contains integers.

* Make cpuset_mems work.

* Make dns_opts work.

* Fixing log_opts: docker expects string values, returns error for integer.

* Adding tests from felixfontein/ansible-docker_container-test#2.

* Make uts work.

* Adding changelog entry.

* Forgot option security_opts.

* Fixing typo.

* Explain strict set(dict) comparison a bit more.

* Improving idempotency tests.

* Making dns_servers a list, since the ordering is relevant.

* Making dns_search_domains a list, since the ordering is relevant.

* Improving dns_search_domains/dns_servers.

* Fixing entrypoint test.

* Making sure options are only supported for correct docker-py versions.

changelogs/fragments/docker_container-idempotency.yaml

@@ -0,0 +1,7 @@
+bugfixes:
+- "docker_container - Makes ``blkio_weight``, ``cpuset_mems``, ``dns_opts`` and ``uts`` options actually work."
+- "docker_container - Fix idempotency problems with ``cap_drop`` and ``groups`` (when numeric group IDs were used)."
+- "docker_container - Fix type conversion errors for ``log_options``."
+- "docker_container - Fixing various comparison/idempotency problems related to wrong comparisons.
+   In particular, comparisons for ``command`` and ``entrypoint`` (both lists) no longer ignore missing
+   elements during idempotency checks."

lib/ansible/modules/cloud/docker/docker_container.py

@@ -752,12 +752,18 @@ class TaskParameters(DockerBaseClass):
 
         for key, value in client.module.params.items():
             setattr(self, key, value)
+        self.comparisons = client.comparisons
 
         # If state is 'absent', parameters do not have to be parsed or interpreted.
         # Only the container's name is needed.
         if self.state == 'absent':
             return
 
+        if self.groups:
+            # In case integers are passed as groups, we need to convert them to
+            # strings as docker internally treats them as strings.
+            self.groups = [str(g) for g in self.groups]
+
         for param_name in REQUIRES_CONVERSION_TO_BYTES:
             if client.module.params.get(param_name):
                 try:
@@ -826,7 +832,6 @@ class TaskParameters(DockerBaseClass):
         '''
 
         update_parameters = dict(
-            blkio_weight='blkio_weight',
             cpu_period='cpu_period',
             cpu_quota='cpu_quota',
             cpu_shares='cpu_shares',
@@ -836,6 +841,15 @@ class TaskParameters(DockerBaseClass):
             memswap_limit='memory_swap',
             kernel_memory='kernel_memory',
         )
+
+        if self.client.HAS_BLKIO_WEIGHT_OPT:
+            # blkio_weight is only supported in docker>=1.9
+            update_parameters['blkio_weight'] = 'blkio_weight'
+
+        if self.client.HAS_CPUSET_MEMS_OPT:
+            # cpuset_mems is only supported in docker>=2.3
+            update_parameters['cpuset_mems'] = 'cpuset_mems'
+
         result = dict()
         for key, value in update_parameters.items():
             if getattr(self, value, None) is not None:
@@ -932,6 +946,7 @@ class TaskParameters(DockerBaseClass):
             links='links',
             privileged='privileged',
             dns='dns_servers',
+            dns_opt='dns_opts',
             dns_search='dns_search_domains',
             binds='volume_binds',
             volumes_from='volumes_from',
@@ -962,6 +977,10 @@ class TaskParameters(DockerBaseClass):
             # auto_remove is only supported in docker>=2
             host_config_params['auto_remove'] = 'auto_remove'
 
+        if self.client.HAS_BLKIO_WEIGHT_OPT:
+            # blkio_weight is only supported in docker>=1.9
+            host_config_params['blkio_weight'] = 'blkio_weight'
+
         if HAS_DOCKER_PY_3:
             # cpu_shares and volume_driver moved to create_host_config in > 3
             host_config_params['cpu_shares'] = 'cpu_shares'
@@ -970,6 +989,9 @@ class TaskParameters(DockerBaseClass):
         if self.client.HAS_INIT_OPT:
             host_config_params['init'] = 'init'
 
+        if self.client.HAS_UTS_MODE_OPT:
+            host_config_params['uts_mode'] = 'uts'
+
         params = dict()
         for key, value in host_config_params.items():
             if getattr(self, value, None) is not None:
@@ -1153,7 +1175,9 @@ class TaskParameters(DockerBaseClass):
         )
 
         if self.log_options is not None:
-            options['Config'] = self.log_options
+            options['Config'] = dict()
+            for k, v in self.log_options.items():
+                options['Config'][k] = str(v)
 
         try:
             return LogConfig(**options)
@@ -1223,6 +1247,19 @@ class Container(DockerBaseClass):
         self.parameters.expected_sysctls = None
         self.parameters.expected_etc_hosts = None
         self.parameters.expected_env = None
+        self.parameters_map = dict()
+        self.parameters_map['expected_links'] = 'links'
+        self.parameters_map['expected_ports'] = 'published_ports'
+        self.parameters_map['expected_exposed'] = 'exposed_ports'
+        self.parameters_map['expected_volumes'] = 'volumes'
+        self.parameters_map['expected_ulimits'] = 'ulimits'
+        self.parameters_map['expected_sysctls'] = 'sysctls'
+        self.parameters_map['expected_etc_hosts'] = 'etc_hosts'
+        self.parameters_map['expected_env'] = 'env'
+        self.parameters_map['expected_entrypoint'] = 'entrypoint'
+        self.parameters_map['expected_binds'] = 'volumes'
+        self.parameters_map['expected_cmd'] = 'command'
+        self.parameters_map['expected_devices'] = 'devices'
 
     def fail(self, msg):
         self.parameters.client.module.fail_json(msg=msg)
@@ -1238,6 +1275,80 @@ class Container(DockerBaseClass):
                 return True
         return False
 
+    def _compare_dict_allow_more_present(self, av, bv):
+        '''
+        Compare two dictionaries for whether every entry of the first is in the second.
+        '''
+        for key, value in av.items():
+            if key not in bv:
+                return False
+            if bv[key] != value:
+                return False
+        return True
+
+    def _compare(self, a, b, compare):
+        '''
+        Compare values a and b as described in compare.
+        '''
+        method = compare['comparison']
+        if method == 'ignore':
+            return True
+        # If a or b is None:
+        if a is None or b is None:
+            # If both are None: equality
+            if a == b:
+                return True
+            # Otherwise, not equal for values, and equal
+            # if the other is empty for set/list/dict
+            if compare['type'] == 'value':
+                return False
+            return len(b if a is None else a) == 0
+        # Do proper comparison (both objects not None)
+        if compare['type'] == 'value':
+            return a == b
+        elif compare['type'] == 'list':
+            if method == 'strict':
+                return a == b
+            else:
+                set_a = set(a)
+                set_b = set(b)
+                return set_b >= set_a
+        elif compare['type'] == 'dict':
+            if method == 'strict':
+                return a == b
+            else:
+                return self._compare_dict_allow_more_present(a, b)
+        elif compare['type'] == 'set':
+            set_a = set(a)
+            set_b = set(b)
+            if method == 'strict':
+                return set_a == set_b
+            else:
+                return set_b >= set_a
+        elif compare['type'] == 'set(dict)':
+            for av in a:
+                found = False
+                for bv in b:
+                    if self._compare_dict_allow_more_present(av, bv):
+                        found = True
+                        break
+                if not found:
+                    return False
+            if method == 'strict':
+                # If we would know that both a and b do not contain duplicates,
+                # we could simply compare len(a) to len(b) to finish this test.
+                # We can assume that b has no duplicates (as it is returned by
+                # docker), but we don't know for a.
+                for bv in b:
+                    found = False
+                    for av in a:
+                        if self._compare_dict_allow_more_present(av, bv):
+                            found = True
+                            break
+                    if not found:
+                        return False
+            return True
+
     def has_different_configuration(self, image):
         '''
         Diff parameters vs existing container config. Returns tuple: (True | False, List of differences)
@@ -1288,6 +1399,7 @@ class Container(DockerBaseClass):
             detach=detach,
             interactive=config.get('OpenStdin'),
             capabilities=host_config.get('CapAdd'),
+            cap_drop=host_config.get('CapDrop'),
             expected_devices=host_config.get('Devices'),
             dns_servers=host_config.get('Dns'),
             dns_opts=host_config.get('DnsOptions'),
@@ -1341,36 +1453,10 @@ class Container(DockerBaseClass):
 
         differences = []
         for key, value in config_mapping.items():
-            self.log('check differences %s %s vs %s' % (key, getattr(self.parameters, key), str(value)))
+            compare = self.parameters.client.comparisons[self.parameters_map.get(key, key)]
+            self.log('check differences %s %s vs %s (%s)' % (key, getattr(self.parameters, key), str(value), compare))
             if getattr(self.parameters, key, None) is not None:
-                if isinstance(getattr(self.parameters, key), list) and isinstance(value, list):
-                    if len(getattr(self.parameters, key)) > 0 and isinstance(getattr(self.parameters, key)[0], dict):
-                        # compare list of dictionaries
-                        self.log("comparing list of dict: %s" % key)
-                        match = self._compare_dictionary_lists(getattr(self.parameters, key), value)
-                    else:
-                        # compare two lists. Is list_a in list_b?
-                        self.log("comparing lists: %s" % key)
-                        set_a = set(getattr(self.parameters, key))
-                        set_b = set(value)
-                        match = (set_b >= set_a)
-                elif isinstance(getattr(self.parameters, key), list) and not len(getattr(self.parameters, key)) \
-                        and value is None:
-                    # an empty list and None are ==
-                    continue
-                elif isinstance(getattr(self.parameters, key), dict) and isinstance(value, dict):
-                    # compare two dicts
-                    self.log("comparing two dicts: %s" % key)
-                    match = self._compare_dicts(getattr(self.parameters, key), value)
-
-                elif isinstance(getattr(self.parameters, key), dict) and \
-                        not len(list(getattr(self.parameters, key).keys())) and value is None:
-                    # an empty dict and None are ==
-                    continue
-                else:
-                    # primitive compare
-                    self.log("primitive compare: %s" % key)
-                    match = (getattr(self.parameters, key) == value)
+                match = self._compare(getattr(self.parameters, key), value, compare)
 
                 if not match:
                     # no match. record the differences
@@ -1384,43 +1470,6 @@ class Container(DockerBaseClass):
         has_differences = True if len(differences) > 0 else False
         return has_differences, differences
 
-    def _compare_dictionary_lists(self, list_a, list_b):
-        '''
-        If all of list_a exists in list_b, return True
-        '''
-        if not isinstance(list_a, list) or not isinstance(list_b, list):
-            return False
-        matches = 0
-        for dict_a in list_a:
-            for dict_b in list_b:
-                if self._compare_dicts(dict_a, dict_b):
-                    matches += 1
-                    break
-        result = (matches == len(list_a))
-        return result
-
-    def _compare_dicts(self, dict_a, dict_b):
-        '''
-        If dict_a in dict_b, return True
-        '''
-        if not isinstance(dict_a, dict) or not isinstance(dict_b, dict):
-            return False
-        for key, value in dict_a.items():
-            if isinstance(value, dict):
-                match = self._compare_dicts(value, dict_b.get(key))
-            elif isinstance(value, list):
-                if len(value) > 0 and isinstance(value[0], dict):
-                    match = self._compare_dictionary_lists(value, dict_b.get(key))
-                else:
-                    set_a = set(value)
-                    set_b = set(dict_b.get(key))
-                    match = (set_a == set_b)
-            else:
-                match = (value == dict_b.get(key))
-            if not match:
-                return False
-        return True
-
     def has_different_resource_limits(self):
         '''
         Diff parameters and container resource limits
@@ -1434,7 +1483,6 @@ class Container(DockerBaseClass):
             cpu_period=host_config.get('CpuPeriod'),
             cpu_quota=host_config.get('CpuQuota'),
             cpuset_cpus=host_config.get('CpusetCpus'),
-            cpuset_mems=host_config.get('CpusetMems'),
             kernel_memory=host_config.get("KernelMemory"),
             memory=host_config.get('Memory'),
             memory_reservation=host_config.get('MemoryReservation'),
@@ -1443,13 +1491,25 @@ class Container(DockerBaseClass):
             oom_killer=host_config.get('OomKillDisable'),
         )
 
+        if self.parameters.client.HAS_BLKIO_WEIGHT_OPT:
+            # blkio_weight is only supported in docker>=1.9
+            config_mapping['blkio_weight'] = host_config.get('BlkioWeight')
+
+        if self.parameters.client.HAS_CPUSET_MEMS_OPT:
+            # cpuset_mems is only supported in docker>=2.3
+            config_mapping['cpuset_mems'] = host_config.get('CpusetMems')
+
         if HAS_DOCKER_PY_3:
             # cpu_shares moved to create_host_config in > 3
             config_mapping['cpu_shares'] = host_config.get('CpuShares')
 
         differences = []
         for key, value in config_mapping.items():
-            if getattr(self.parameters, key, None) and getattr(self.parameters, key) != value:
+            if getattr(self.parameters, key, None):
+                compare = self.parameters.client.comparisons[self.parameters_map.get(key, key)]
+                match = self._compare(getattr(self.parameters, key), value, compare)
+
+                if not match:
                     # no match. record the differences
                     item = dict()
                     item[key] = dict(
@@ -1805,7 +1865,7 @@ class ContainerManager(DockerBaseClass):
                 # Existing container
                 different, differences = container.has_different_configuration(image)
                 image_different = False
-                if not self.parameters.ignore_image:
+                if self.parameters.comparisons['image']['comparison'] == 'strict':
                     image_different = self._image_is_different(image, container)
                 if image_different or different or self.parameters.recreate:
                     self.diff['differences'] = differences
@@ -2065,6 +2125,51 @@ class ContainerManager(DockerBaseClass):
 
 class AnsibleDockerClientContainer(AnsibleDockerClient):
 
+    def _setup_comparisons(self):
+        comparisons = {}
+        comp_aliases = {}
+        # Put in defaults
+        explicit_types = dict(
+            command='list',
+            devices='set(dict)',
+            dns_search_domains='list',
+            dns_servers='list',
+            env='set',
+            entrypoint='list',
+            etc_hosts='set',
+            ulimits='set(dict)',
+        )
+        for option, data in self.module.argument_spec.items():
+            # Ignore options which aren't used as container properties
+            if option in ('docker_host', 'tls_hostname', 'api_version', 'timeout', 'cacert_path', 'cert_path',
+                          'key_path', 'ssl_version', 'tls', 'tls_verify', 'debug', 'env_file', 'force_kill',
+                          'keep_volumes', 'ignore_image', 'name', 'pull', 'purge_networks', 'recreate',
+                          'restart', 'state', 'stop_timeout', 'trust_image_content', 'networks'):
+                continue
+            # Determine option type
+            if option in explicit_types:
+                type = explicit_types[option]
+            elif data['type'] == 'list':
+                type = 'set'
+            elif data['type'] == 'dict':
+                type = 'dict'
+            else:
+                type = 'value'
+            # Determine comparison type
+            if type in ('list', 'value'):
+                comparison = 'strict'
+            else:
+                comparison = 'allow_more_present'
+            comparisons[option] = dict(type=type, comparison=comparison, name=option)
+            # Keep track of aliases
+            comp_aliases[option] = option
+            for alias in data.get('aliases', []):
+                comp_aliases[alias] = option
+        # Process legacy ignore options
+        if self.module.params['ignore_image']:
+            comparisons['image']['comparison'] = 'ignore'
+        self.comparisons = comparisons
+
     def __init__(self, **kwargs):
         super(AnsibleDockerClientContainer, self).__init__(**kwargs)
 
@@ -2078,11 +2183,30 @@ class AnsibleDockerClientContainer(AnsibleDockerClient):
             self.fail("docker or docker-py version is %s. Minimum version required is 2.2 to set init option. "
                       "If you use the 'docker-py' module, you have to switch to the docker 'Python' package." % (docker_version,))
 
+        uts_mode_supported = LooseVersion(docker_version) >= LooseVersion('3.5')
+        if self.module.params.get("uts") is not None and not uts_mode_supported:
+            self.fail("docker or docker-py version is %s. Minimum version required is 3.5 to set uts option. "
+                      "If you use the 'docker-py' module, you have to switch to the docker 'Python' package." % (docker_version,))
+
+        blkio_weight_supported = LooseVersion(docker_version) >= LooseVersion('1.9')
+        if self.module.params.get("blkio_weight") is not None and not blkio_weight_supported:
+            self.fail("docker or docker-py version is %s. Minimum version required is 1.9 to set blkio_weight option.")
+
+        cpuset_mems_supported = LooseVersion(docker_version) >= LooseVersion('2.3')
+        if self.module.params.get("cpuset_mems") is not None and not cpuset_mems_supported:
+            self.fail("docker or docker-py version is %s. Minimum version required is 2.3 to set cpuset_mems option. "
+                      "If you use the 'docker-py' module, you have to switch to the docker 'Python' package." % (docker_version,))
+
         self.HAS_INIT_OPT = init_supported
+        self.HAS_UTS_MODE_OPT = uts_mode_supported
+        self.HAS_BLKIO_WEIGHT_OPT = blkio_weight_supported
+        self.HAS_CPUSET_MEMS_OPT = cpuset_mems_supported
         self.HAS_AUTO_REMOVE_OPT = HAS_DOCKER_PY_2 or HAS_DOCKER_PY_3
         if self.module.params.get('auto_remove') and not self.HAS_AUTO_REMOVE_OPT:
             self.fail("'auto_remove' is not compatible with the 'docker-py' Python package. It requires the newer 'docker' Python package.")
 
+        self._setup_comparisons()
+
 
 def main():
     argument_spec = dict(
@@ -2103,9 +2227,9 @@ def main():
         dns_opts=dict(type='list'),
         dns_search_domains=dict(type='list'),
         domainname=dict(type='str'),
+        entrypoint=dict(type='list'),
         env=dict(type='dict'),
         env_file=dict(type='path'),
-        entrypoint=dict(type='list'),
         etc_hosts=dict(type='dict'),
         exposed_ports=dict(type='list', aliases=['exposed', 'expose']),
         force_kill=dict(type='bool', default=False, aliases=['forcekill']),
@@ -2130,7 +2254,6 @@ def main():
         memory_swappiness=dict(type='int'),
         name=dict(type='str', required=True),
         network_mode=dict(type='str'),
-        userns_mode=dict(type='str'),
         networks=dict(type='list'),
         oom_killer=dict(type='bool'),
         oom_score_adj=dict(type='int'),
@@ -2146,21 +2269,22 @@ def main():
         restart=dict(type='bool', default=False),
         restart_policy=dict(type='str', choices=['no', 'on-failure', 'always', 'unless-stopped']),
         restart_retries=dict(type='int', default=None),
-        shm_size=dict(type='str'),
         security_opts=dict(type='list'),
+        shm_size=dict(type='str'),
         state=dict(type='str', choices=['absent', 'present', 'started', 'stopped'], default='started'),
         stop_signal=dict(type='str'),
         stop_timeout=dict(type='int'),
+        sysctls=dict(type='dict'),
         tmpfs=dict(type='list'),
         trust_image_content=dict(type='bool', default=False),
         tty=dict(type='bool', default=False),
         ulimits=dict(type='list'),
-        sysctls=dict(type='dict'),
         user=dict(type='str'),
+        userns_mode=dict(type='str'),
         uts=dict(type='str'),
+        volume_driver=dict(type='str'),
         volumes=dict(type='list'),
         volumes_from=dict(type='list'),
-        volume_driver=dict(type='str'),
         working_dir=dict(type='str'),
     )
 

test/integration/targets/docker_container/files/env-file

@@ -0,0 +1,2 @@
+TEST3=val3
+TEST4=val4

test/integration/targets/docker_container/tasks/main.yml

@@ -1,9 +1,9 @@
 ---
-- block:
 - name: Create random container name prefix
   set_fact:
     cname_prefix: "{{ 'ansible-test-%0x' % ((2**32) | random) }}"
     cnames: []
+    dnetworks: []
 
 - debug:
     msg: "Using container name prefix {{ cname_prefix }}"
@@ -20,4 +20,12 @@
       state: absent
       stop_timeout: 1
     with_items: "{{ cnames }}"
-  when: ansible_os_family != 'RedHat' or ansible_distribution_major_version != '6'
+  - name: "Make sure all networks are removed"
+    docker_network:
+      name: "{{ item }}"
+      state: absent
+      force: yes
+    with_items: "{{ dnetworks }}"
+
+  # Skip for CentOS 6
+  when: ansible_distribution != 'CentOS' or ansible_distribution_major_version|int > 6

test/integration/targets/docker_container/tasks/tests/regression-45700-dont-parse-on-absent.yml

@@ -10,7 +10,7 @@
 - name: Start container
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
 

test/integration/targets/docker_container/tasks/tests/start-stop.yml

@@ -13,7 +13,7 @@
 - name: Create container (check)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   check_mode: yes
@@ -22,7 +22,7 @@
 - name: Create container
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   register: create_2
@@ -30,7 +30,7 @@
 - name: Create container (idempotent)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   register: create_3
@@ -38,7 +38,7 @@
 - name: Create container (idempotent check)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   check_mode: yes
@@ -95,7 +95,7 @@
 - name: Present check for running container (check)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   check_mode: yes
@@ -104,7 +104,7 @@
 - name: Present check for running container
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: present
   register: present_check_2
@@ -127,7 +127,7 @@
 - name: Start container from scratch (check)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   check_mode: yes
@@ -136,7 +136,7 @@
 - name: Start container from scratch
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   register: start_scratch_2
@@ -144,7 +144,7 @@
 - name: Start container from scratch (idempotent)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   register: start_scratch_3
@@ -152,7 +152,7 @@
 - name: Start container from scratch (idempotent check)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started
   check_mode: yes
@@ -254,7 +254,7 @@
 - name: Start container (setup for removing from running)
   docker_container:
     image: alpine:3.8
-    command: '/bin/sh -c "sleep 1h"'
+    command: '/bin/sh -c "sleep 10m"'
     name: "{{ cname }}"
     state: started