From 9d6e86c0217c97d33aecdcb47c35521a6ee91a29 Mon Sep 17 00:00:00 2001 From: Michael Gugino Date: Wed, 27 Sep 2017 12:03:43 -0400 Subject: Migrate enterprise registry logic to docker role Currently, the enterprise registry to forcefully added in openshift_facts. Recently, the docker role has been modified to consume registry variables directly, bypassing openshift_facts. This commit cleans up unused code in openshift_facts, and migrates enterprise registry logic to the docker role. Fixes: https://github.com/openshift/openshift-ansible/issues/5557 --- .../initialize_oo_option_facts.yml | 9 -------- roles/docker/defaults/main.yml | 2 ++ roles/docker/tasks/package_docker.yml | 8 ++++++++ roles/docker/tasks/systemcontainer_crio.yml | 12 +++++------ roles/docker/tasks/systemcontainer_docker.yml | 6 +++--- roles/openshift_docker_facts/tasks/main.yml | 9 -------- roles/openshift_facts/library/openshift_facts.py | 24 ---------------------- .../openshift_checks/docker_image_availability.py | 2 +- .../test/docker_image_availability_test.py | 10 ++++----- 9 files changed, 25 insertions(+), 57 deletions(-) diff --git a/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml b/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml index ac3c702a0..dab17aaa9 100644 --- a/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml +++ b/playbooks/common/openshift-cluster/initialize_oo_option_facts.yml @@ -4,15 +4,6 @@ tags: - always tasks: - - set_fact: - openshift_docker_additional_registries: "{{ lookup('oo_option', 'docker_additional_registries') }}" - when: openshift_docker_additional_registries is not defined - - set_fact: - openshift_docker_insecure_registries: "{{ lookup('oo_option', 'docker_insecure_registries') }}" - when: openshift_docker_insecure_registries is not defined - - set_fact: - openshift_docker_blocked_registries: "{{ lookup('oo_option', 'docker_blocked_registries') }}" - when: openshift_docker_blocked_registries is not defined - set_fact: openshift_docker_options: "{{ lookup('oo_option', 'docker_options') }}" when: openshift_docker_options is not defined diff --git a/roles/docker/defaults/main.yml b/roles/docker/defaults/main.yml index 274fd8603..e36dfa7b9 100644 --- a/roles/docker/defaults/main.yml +++ b/roles/docker/defaults/main.yml @@ -9,6 +9,8 @@ openshift_docker_additional_registries: [] openshift_docker_blocked_registries: [] openshift_docker_insecure_registries: [] +openshift_docker_ent_reg: 'registry.access.redhat.com' + # The l2_docker_* variables convert csv strings to lists, if # necessary. These variables should be used in place of their respective # openshift_docker_* counterparts to ensure the properly formatted lists are diff --git a/roles/docker/tasks/package_docker.yml b/roles/docker/tasks/package_docker.yml index 0c5621259..4215dc5bd 100644 --- a/roles/docker/tasks/package_docker.yml +++ b/roles/docker/tasks/package_docker.yml @@ -50,6 +50,14 @@ src: custom.conf.j2 when: not os_firewall_use_firewalld | default(False) | bool +- name: Add enterprise registry, if necessary + set_fact: + l2_docker_additional_registries: "{{ l2_docker_additional_registries + [openshift_docker_ent_reg] }}" + when: + - openshift.common.deployment_type == 'openshift-enterprise' + - openshift_docker_ent_reg != '' + - openshift_docker_ent_reg not in l2_docker_additional_registries + - stat: path=/etc/sysconfig/docker register: docker_check diff --git a/roles/docker/tasks/systemcontainer_crio.yml b/roles/docker/tasks/systemcontainer_crio.yml index 5b02b72be..66ce475e1 100644 --- a/roles/docker/tasks/systemcontainer_crio.yml +++ b/roles/docker/tasks/systemcontainer_crio.yml @@ -1,17 +1,17 @@ --- # TODO: Much of this file is shared with container engine tasks - set_fact: - l_insecure_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(openshift.docker.insecure_registries)) }}" - when: openshift.docker.insecure_registries + l_insecure_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l2_docker_insecure_registries)) }}" + when: l2_docker_insecure_registries - set_fact: - l_crio_registries: "{{ openshift.docker.additional_registries + ['docker.io'] }}" - when: openshift.docker.additional_registries + l_crio_registries: "{{ l2_docker_additional_registries + ['docker.io'] }}" + when: l2_docker_additional_registries - set_fact: l_crio_registries: "{{ ['docker.io'] }}" - when: not openshift.docker.additional_registries + when: not l2_docker_additional_registries - set_fact: l_additional_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l_crio_registries)) }}" - when: openshift.docker.additional_registries + when: l2_docker_additional_registries - name: Ensure container-selinux is installed package: diff --git a/roles/docker/tasks/systemcontainer_docker.yml b/roles/docker/tasks/systemcontainer_docker.yml index 146e5f430..8b43393cb 100644 --- a/roles/docker/tasks/systemcontainer_docker.yml +++ b/roles/docker/tasks/systemcontainer_docker.yml @@ -148,10 +148,10 @@ # Set local versions of facts that must be in json format for container-daemon.json # NOTE: When jinja2.9+ is used the container-daemon.json file can move to using tojson - set_fact: - l_docker_insecure_registries: "{{ docker_insecure_registries | default([]) | to_json }}" + l_docker_insecure_registries: "{{ l2_docker_insecure_registries | default([]) | to_json }}" l_docker_log_options: "{{ docker_log_options | default({}) | to_json }}" - l_docker_additional_registries: "{{ docker_additional_registries | default([]) | to_json }}" - l_docker_blocked_registries: "{{ docker_blocked_registries | default([]) | to_json }}" + l_docker_additional_registries: "{{ l2_docker_additional_registries | default([]) | to_json }}" + l_docker_blocked_registries: "{{ l2_docker_blocked_registries | default([]) | to_json }}" l_docker_selinux_enabled: "{{ docker_selinux_enabled | default(true) | to_json }}" # Configure container-engine using the container-daemon.json file diff --git a/roles/openshift_docker_facts/tasks/main.yml b/roles/openshift_docker_facts/tasks/main.yml index 334150f63..5a3e50678 100644 --- a/roles/openshift_docker_facts/tasks/main.yml +++ b/roles/openshift_docker_facts/tasks/main.yml @@ -6,9 +6,6 @@ with_items: - role: docker local_facts: - additional_registries: "{{ openshift_docker_additional_registries | default(None) }}" - blocked_registries: "{{ openshift_docker_blocked_registries | default(None) }}" - insecure_registries: "{{ openshift_docker_insecure_registries | default(None) }}" selinux_enabled: "{{ openshift_docker_selinux_enabled | default(None) }}" log_driver: "{{ openshift_docker_log_driver | default(None) }}" log_options: "{{ openshift_docker_log_options | default(None) }}" @@ -23,12 +20,6 @@ sdn_mtu: "{{ openshift_node_sdn_mtu | default(None) }}" - set_fact: - docker_additional_registries: "{{ openshift.docker.additional_registries - | default(omit) }}" - docker_blocked_registries: "{{ openshift.docker.blocked_registries - | default(omit) }}" - docker_insecure_registries: "{{ openshift.docker.insecure_registries - | default(omit) }}" docker_selinux_enabled: "{{ openshift.docker.selinux_enabled | default(omit) }}" docker_log_driver: "{{ openshift.docker.log_driver | default(omit) }}" docker_log_options: "{{ openshift.docker.log_options | default(omit) }}" diff --git a/roles/openshift_facts/library/openshift_facts.py b/roles/openshift_facts/library/openshift_facts.py index 1c2c91a5a..11ef9fa97 100755 --- a/roles/openshift_facts/library/openshift_facts.py +++ b/roles/openshift_facts/library/openshift_facts.py @@ -55,9 +55,6 @@ def migrate_docker_facts(facts): """ Apply migrations for docker facts """ params = { 'common': ( - 'additional_registries', - 'insecure_registries', - 'blocked_registries', 'options' ), 'node': ( @@ -768,14 +765,6 @@ def set_deployment_facts_if_unset(facts): service_type = 'origin' facts['common']['service_type'] = service_type - if 'docker' in facts: - deployment_type = facts['common']['deployment_type'] - if deployment_type == 'openshift-enterprise': - addtl_regs = facts['docker'].get('additional_registries', []) - ent_reg = 'registry.access.redhat.com' - if ent_reg not in addtl_regs: - facts['docker']['additional_registries'] = addtl_regs + [ent_reg] - for role in ('master', 'node'): if role in facts: deployment_type = facts['common']['deployment_type'] @@ -2248,19 +2237,6 @@ class OpenShiftFacts(object): protected_facts_to_overwrite) if 'docker' in new_local_facts: - # remove duplicate and empty strings from registry lists, preserving order - for cat in ['additional', 'blocked', 'insecure']: - key = '{0}_registries'.format(cat) - if key in new_local_facts['docker']: - val = new_local_facts['docker'][key] - if isinstance(val, string_types): - val = [x.strip() for x in val.split(',')] - seen = set() - new_local_facts['docker'][key] = list() - for registry in val: - if registry not in seen and registry != '': - seen.add(registry) - new_local_facts['docker'][key].append(registry) # Convert legacy log_options comma sep string to a list if present: if 'log_options' in new_local_facts['docker'] and \ isinstance(new_local_facts['docker']['log_options'], string_types): diff --git a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py index 98372d979..93a5973d4 100644 --- a/roles/openshift_health_checker/openshift_checks/docker_image_availability.py +++ b/roles/openshift_health_checker/openshift_checks/docker_image_availability.py @@ -153,7 +153,7 @@ class DockerImageAvailability(DockerHostMixin, OpenShiftCheck): def known_docker_registries(self): """Build a list of docker registries available according to inventory vars.""" - regs = list(self.get_var("openshift.docker.additional_registries", default=[])) + regs = list(self.get_var("openshift_docker_additional_registries", default=[])) deployment_type = self.get_var("openshift_deployment_type") if deployment_type == "origin" and "docker.io" not in regs: diff --git a/roles/openshift_health_checker/test/docker_image_availability_test.py b/roles/openshift_health_checker/test/docker_image_availability_test.py index 952fa9aa6..c523ffd5c 100644 --- a/roles/openshift_health_checker/test/docker_image_availability_test.py +++ b/roles/openshift_health_checker/test/docker_image_availability_test.py @@ -72,7 +72,7 @@ def test_all_images_available_remotely(task_vars, available_locally): return {'images': [], 'failed': available_locally} return {} - task_vars['openshift']['docker']['additional_registries'] = ["docker.io", "registry.access.redhat.com"] + task_vars['openshift_docker_additional_registries'] = ["docker.io", "registry.access.redhat.com"] task_vars['openshift_image_tag'] = 'v3.4' check = DockerImageAvailability(execute_module, task_vars) check._module_retry_interval = 0 @@ -90,7 +90,7 @@ def test_all_images_unavailable(task_vars): return {} # docker_image_facts failure - task_vars['openshift']['docker']['additional_registries'] = ["docker.io"] + task_vars['openshift_docker_additional_registries'] = ["docker.io"] task_vars['openshift_deployment_type'] = "openshift-enterprise" task_vars['openshift_image_tag'] = 'latest' check = DockerImageAvailability(execute_module, task_vars) @@ -121,9 +121,9 @@ def test_no_known_registries(): service_type='origin', is_containerized=False, is_atomic=False, - ), - docker=dict(additional_registries=["docker.io"]), + ) ), + openshift_docker_additional_registries=["docker.io"], openshift_deployment_type="openshift-enterprise", openshift_image_tag='latest', group_names=['nodes', 'masters'], @@ -154,7 +154,7 @@ def test_skopeo_update_failure(task_vars, message, extra_words): return {} - task_vars['openshift']['docker']['additional_registries'] = ["unknown.io"] + task_vars['openshift_docker_additional_registries'] = ["unknown.io"] task_vars['openshift_deployment_type'] = "openshift-enterprise" check = DockerImageAvailability(execute_module, task_vars) check._module_retry_interval = 0 -- cgit v1.2.1