Merge pull request #3355 from tbielawa/idempotency_please_work

Idempotency please work

playbooks/common/openshift-cluster/upgrades/etcd/backup.yml

@@ -29,12 +29,18 @@
   - name: Check available disk space for etcd backup
     shell: df --output=avail -k {{ openshift.common.data_dir }} | tail -n 1
     register: avail_disk
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
 
   # TODO: replace shell module with command and update later checks
   - name: Check current embedded etcd disk usage
     shell: du -k {{ openshift.etcd.etcd_data_dir }} | tail -n 1 | cut -f1
     register: etcd_disk_usage
     when: embedded_etcd | bool
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
 
   - name: Abort if insufficient disk space for etcd backup
     fail:

playbooks/common/openshift-cluster/upgrades/etcd/upgrade.yml

@@ -9,21 +9,36 @@
     register: etcd_rpm_version
     failed_when: false
     when: not openshift.common.is_containerized | bool
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
+
   - name: Record containerized etcd version
     command: docker exec etcd_container rpm -qa --qf '%{version}' etcd\*
     register: etcd_container_version
     failed_when: false
     when: openshift.common.is_containerized | bool
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
+
   - name: Record containerized etcd version
     command: docker exec etcd_container rpm -qa --qf '%{version}' etcd\*
     register: etcd_container_version
     failed_when: false
     when: openshift.common.is_containerized | bool and not openshift.common.is_etcd_system_container | bool
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
+
   - name: Record containerized etcd version
     command: runc exec etcd_container rpm -qa --qf '%{version}' etcd\*
     register: etcd_container_version
     failed_when: false
     when: openshift.common.is_containerized | bool and openshift.common.is_etcd_system_container | bool
+    # AUDIT:changed_when: `false` because we are only inspecting
+    # state, not manipulating anything
+    changed_when: false
 
 # I really dislike this copy/pasta but I wasn't able to find a way to get it to loop
 # through hosts, then loop through tasks only when appropriate

playbooks/common/openshift-cluster/upgrades/post_control_plane.yml

@@ -45,6 +45,9 @@
       '{"spec":{"template":{"spec":{"containers":[{"name":"router","image":"{{ router_image }}","livenessProbe":{"tcpSocket":null,"httpGet":{"path": "/healthz", "port": 1936, "host": "localhost", "scheme": "HTTP"},"initialDelaySeconds":10,"timeoutSeconds":1}}]}}}}'
       --api-version=v1
     with_items: "{{ haproxy_routers }}"
+    # AUDIT:changed_when_note: `false` not being set here. What we
+    # need to do is check the current router image version and see if
+    # this task needs to be ran.
 
   - name: Check for default registry
     oc_obj:
@@ -62,6 +65,9 @@
       {{ oc_cmd }} patch dc/docker-registry -n default -p
       '{"spec":{"template":{"spec":{"containers":[{"name":"registry","image":"{{ registry_image }}"}]}}}}'
       --api-version=v1
+    # AUDIT:changed_when_note: `false` not being set here. What we
+    # need to do is check the current registry image version and see
+    # if this task needs to be ran.
 
   roles:
   - openshift_manageiq

roles/openshift_facts/tasks/main.yml

@@ -1,52 +1,64 @@
 ---
-- name: Detecting Operating System
-  stat:
-    path: /run/ostree-booted
-  register: ostree_booted
+- block:
+  - name: Detecting Operating System
+    stat:
+      path: /run/ostree-booted
+    register: ostree_booted
 
-# Locally setup containerized facts for now
-- set_fact:
-    l_is_atomic: "{{ ostree_booted.stat.exists }}"
-- set_fact:
-    l_is_containerized: "{{ (l_is_atomic | bool) or (containerized | default(false) | bool) }}"
-    l_is_openvswitch_system_container: "{{ (use_openvswitch_system_container | default(use_system_containers) | bool) }}"
-    l_is_node_system_container: "{{ (use_node_system_container | default(use_system_containers) | bool) }}"
-    l_is_master_system_container: "{{ (use_master_system_container | default(use_system_containers) | bool) }}"
-    l_is_etcd_system_container: "{{ (use_etcd_system_container | default(use_system_containers) | bool) }}"
+  # Locally setup containerized facts for now
+  - set_fact:
+      l_is_atomic: "{{ ostree_booted.stat.exists }}"
+  - set_fact:
+      l_is_containerized: "{{ (l_is_atomic | bool) or (containerized | default(false) | bool) }}"
+      l_is_openvswitch_system_container: "{{ (use_openvswitch_system_container | default(use_system_containers) | bool) }}"
+      l_is_node_system_container: "{{ (use_node_system_container | default(use_system_containers) | bool) }}"
+      l_is_master_system_container: "{{ (use_master_system_container | default(use_system_containers) | bool) }}"
+      l_is_etcd_system_container: "{{ (use_etcd_system_container | default(use_system_containers) | bool) }}"
 
-- name: Ensure various deps are installed
-  package: name={{ item }} state=present
-  with_items: "{{ required_packages }}"
-  when: not l_is_atomic | bool
+  - name: Ensure various deps are installed
+    package: name={{ item }} state=present
+    with_items: "{{ required_packages }}"
+    when: not l_is_atomic | bool
 
-- name: Gather Cluster facts and set is_containerized if needed
-  openshift_facts:
-    role: common
-    local_facts:
-      debug_level: "{{ openshift_debug_level | default(2) }}"
-      # TODO: Deprecate deployment_type in favor of openshift_deployment_type
-      deployment_type: "{{ openshift_deployment_type | default(deployment_type) }}"
-      deployment_subtype: "{{ openshift_deployment_subtype | default(None) }}"
-      cluster_id: "{{ openshift_cluster_id | default('default') }}"
-      hostname: "{{ openshift_hostname | default(None) }}"
-      ip: "{{ openshift_ip | default(None) }}"
-      is_containerized: "{{ l_is_containerized | default(None) }}"
-      is_openvswitch_system_container: "{{ l_is_openvswitch_system_container | default(false) }}"
-      is_node_system_container: "{{ l_is_node_system_container | default(false) }}"
-      is_master_system_container: "{{ l_is_master_system_container | default(false) }}"
-      is_etcd_system_container: "{{ l_is_etcd_system_container | default(false) }}"
-      system_images_registry: "{{ system_images_registry | default('') }}"
-      public_hostname: "{{ openshift_public_hostname | default(None) }}"
-      public_ip: "{{ openshift_public_ip | default(None) }}"
-      portal_net: "{{ openshift_portal_net | default(openshift_master_portal_net) | default(None) }}"
-      http_proxy: "{{ openshift_http_proxy | default(None) }}"
-      https_proxy: "{{ openshift_https_proxy | default(None) }}"
-      no_proxy: "{{ openshift_no_proxy | default(None) }}"
-      generate_no_proxy_hosts: "{{ openshift_generate_no_proxy_hosts | default(True) }}"
-      no_proxy_internal_hostnames: "{{ openshift_no_proxy_internal_hostnames | default(None) }}"
-      sdn_network_plugin_name: "{{ os_sdn_network_plugin_name | default(None) }}"
-      use_openshift_sdn: "{{ openshift_use_openshift_sdn | default(None) }}"
+  - name: Gather Cluster facts and set is_containerized if needed
+    openshift_facts:
+      role: common
+      local_facts:
+        debug_level: "{{ openshift_debug_level | default(2) }}"
+        # TODO: Deprecate deployment_type in favor of openshift_deployment_type
+        deployment_type: "{{ openshift_deployment_type | default(deployment_type) }}"
+        deployment_subtype: "{{ openshift_deployment_subtype | default(None) }}"
+        cluster_id: "{{ openshift_cluster_id | default('default') }}"
+        hostname: "{{ openshift_hostname | default(None) }}"
+        ip: "{{ openshift_ip | default(None) }}"
+        is_containerized: "{{ l_is_containerized | default(None) }}"
+        is_openvswitch_system_container: "{{ l_is_openvswitch_system_container | default(false) }}"
+        is_node_system_container: "{{ l_is_node_system_container | default(false) }}"
+        is_master_system_container: "{{ l_is_master_system_container | default(false) }}"
+        is_etcd_system_container: "{{ l_is_etcd_system_container | default(false) }}"
+        system_images_registry: "{{ system_images_registry | default('') }}"
+        public_hostname: "{{ openshift_public_hostname | default(None) }}"
+        public_ip: "{{ openshift_public_ip | default(None) }}"
+        portal_net: "{{ openshift_portal_net | default(openshift_master_portal_net) | default(None) }}"
+        http_proxy: "{{ openshift_http_proxy | default(None) }}"
+        https_proxy: "{{ openshift_https_proxy | default(None) }}"
+        no_proxy: "{{ openshift_no_proxy | default(None) }}"
+        generate_no_proxy_hosts: "{{ openshift_generate_no_proxy_hosts | default(True) }}"
+        no_proxy_internal_hostnames: "{{ openshift_no_proxy_internal_hostnames | default(None) }}"
+        sdn_network_plugin_name: "{{ os_sdn_network_plugin_name | default(None) }}"
+        use_openshift_sdn: "{{ openshift_use_openshift_sdn | default(None) }}"
 
-- name: Set repoquery command
+  - name: Set repoquery command
+    set_fact:
+      repoquery_cmd: "{{ 'dnf repoquery --latest-limit 1 -d 0' if ansible_pkg_mgr == 'dnf' else 'repoquery --plugins' }}"
+
+  # This `when` allows us to skip this expensive block of tasks on
+  # subsequent calls to the `openshift_facts` role. You will notice
+  # speed-ups in proportion to the size of your cluster as this will
+  # skip all tasks on the next calls to the `openshift_facts` role.
+  when:
+  - openshift_facts_init is not defined
+
+- name: Record that openshift_facts has initialized
   set_fact:
-    repoquery_cmd: "{{ 'dnf repoquery --latest-limit 1 -d 0' if ansible_pkg_mgr == 'dnf' else 'repoquery --plugins' }}"
+    openshift_facts_init: true

roles/openshift_hosted_templates/tasks/main.yml

@@ -4,6 +4,8 @@
   become: False
   register: copy_hosted_templates_mktemp
   run_once: True
+  # AUDIT:changed_when: not set here because this task actually
+  # creates something
 
 - name: Create tar of OpenShift examples
   local_action: command tar -C "{{ role_path }}/files/{{ content_version }}/{{ hosted_deployment_type }}" -cvf "{{ copy_hosted_templates_mktemp.stdout }}/openshift-hosted-templates.tar" .

roles/openshift_manageiq/tasks/main.yaml

@@ -47,6 +47,9 @@
   register: oshawkular_create_cluster_role
   failed_when: "'already exists' not in oshawkular_create_cluster_role.stderr and oshawkular_create_cluster_role.rc != 0"
   changed_when: oshawkular_create_cluster_role.rc == 0
+  # AUDIT:changed_when_note: Checking the return code is insufficient
+  # here. We really need to verify the if the role even exists before
+  # we run this task.
 
 - name: Configure role/user permissions
   command: >
@@ -56,6 +59,10 @@
   register: osmiq_perm_task
   failed_when: "'already exists' not in osmiq_perm_task.stderr and osmiq_perm_task.rc != 0"
   changed_when: osmiq_perm_task.rc == 0
+  # AUDIT:changed_when_note: Checking the return code is insufficient
+  # here. We really need to compare the current role/user permissions
+  # with their expected state. I think we may have a module for this?
+
 
 - name: Configure 3_2 role/user permissions
   command: >

roles/openshift_node_upgrade/tasks/main.yml

@@ -75,3 +75,9 @@
   # so containerized services should restart quickly as well.
   retries: 24
   delay: 5
+  # AUDIT:changed_when: `false` because we are only inspecting the
+  # state of the node, we aren't changing anything (we changed node
+  # service state in the previous task). You could say we shouldn't
+  # override this because something will be changing (the state of a
+  # service), but that should be part of the last task.
+  changed_when: false