additional code reviews

roles/openshift_metrics/meta/main.yaml

@@ -1,2 +1,18 @@
+---
+galaxy_info:
+  author: OpenShift Development <dev@lists.openshift.redhat.com>
+  description: Deploy OpenShift metrics integration for the cluster
+  company: Red Hat, Inc.
+  license: license (Apache)
+  min_ansible_version: 2.2
+  platforms:
+  - name: EL
+    versions:
+    - 7
+  - name: Fedora
+    versions:
+    - all
+  categories:
+    - openshift
 dependencies:
 - { role: openshift_facts }

roles/openshift_metrics/tasks/generate_hawkular_certificates.yaml

@@ -4,31 +4,37 @@
   vars:
     component: hawkular-metrics
     hostnames: "hawkular-metrics,{{ openshift_metrics_hawkular_hostname }}"
+  changed_when: no
+
 - name: generate hawkular-cassandra certificates
   include: setup_certificate.yaml
   vars:
     component: hawkular-cassandra
     hostnames: hawkular-cassandra
+  changed_when: no
+
 - name: check existing aliases on the hawkular-cassandra truststore
   shell: >
     keytool -noprompt -list
-    -keystore {{ openshift_metrics_certs_dir }}/hawkular-cassandra.truststore
+    -keystore {{ openshift_metrics_certs_dir|quote }}/hawkular-cassandra.truststore
     -storepass "$(<
-    '{{ openshift_metrics_certs_dir }}/hawkular-cassandra-truststore.pwd')"
+    '{{ openshift_metrics_certs_dir|quote }}/hawkular-cassandra-truststore.pwd')"
     | sed -n '7~2s/,.*$//p'
   register: hawkular_cassandra_truststore_aliases
   changed_when: false
+
 - name: check existing aliases on the hawkular-metrics truststore
   shell: >
     keytool -noprompt -list
-    -keystore {{ openshift_metrics_certs_dir }}/hawkular-metrics.truststore
+    -keystore {{ openshift_metrics_certs_dir|quote }}/hawkular-metrics.truststore
     -storepass "$(<
-    '{{ openshift_metrics_certs_dir }}/hawkular-metrics-truststore.pwd')"
+    '{{ openshift_metrics_certs_dir|quote }}/hawkular-metrics-truststore.pwd')"
     | sed -n '7~2s/,.*$//p'
   register: hawkular_metrics_truststore_aliases
   changed_when: false
+
 - name: import the hawkular metrics cert into the cassandra truststore
-  shell: >
+  command: >
     keytool -noprompt -import -v -trustcacerts
     -alias hawkular-metrics
     -file '{{ openshift_metrics_certs_dir }}/hawkular-metrics.crt'
@@ -38,8 +44,9 @@
   when: >
     'hawkular-metrics' not in
     hawkular_cassandra_truststore_aliases.stdout_lines
+
 - name: import the hawkular cassandra cert into the hawkular metrics truststore
-  shell: >
+  command: >
     keytool -noprompt -import -v -trustcacerts
     -alias hawkular-cassandra
     -file '{{ openshift_metrics_certs_dir }}/hawkular-cassandra.crt'
@@ -49,8 +56,9 @@
   when: >
     'hawkular-cassandra' not in
     hawkular_metrics_truststore_aliases.stdout_lines
+
 - name: import the hawkular cassandra cert into the cassandra truststore
-  shell: >
+  command: >
     keytool -noprompt -import -v -trustcacerts
     -alias hawkular-cassandra
     -file '{{ openshift_metrics_certs_dir }}/hawkular-cassandra.crt'
@@ -60,8 +68,9 @@
   when: >
     'hawkular-cassandra' not in
     hawkular_cassandra_truststore_aliases.stdout_lines
+
 - name: import the ca certificate into the cassandra truststore
-  shell: >
+  command: >
     keytool -noprompt -import -v -trustcacerts
     -alias '{{ item }}'
     -file '{{ openshift_metrics_certs_dir }}/ca.crt'
@@ -73,8 +82,9 @@
   - metricca
   - cassandraca
   when: item not in hawkular_cassandra_truststore_aliases.stdout_lines
+
 - name: import the ca certificate into the hawkular metrics truststore
-  shell: >
+  command: >
     keytool -noprompt -import -v -trustcacerts
     -alias '{{ item }}'
     -file '{{ openshift_metrics_certs_dir }}/ca.crt'
@@ -86,6 +96,7 @@
   - metricca
   - cassandraca
   when: item not in hawkular_metrics_truststore_aliases.stdout_lines
+
 - name: generate password for hawkular metrics and jgroups
   shell: >
     tr -dc _A-Z-a-z-0-9 < /dev/urandom | head -c15
@@ -94,6 +105,7 @@
   - hawkular-metrics
   - hawkular-jgroups-keystore
   when: not '{{ openshift_metrics_certs_dir }}/{{ item }}.pwd'|exists
+
 - name: generate htpasswd file for hawkular metrics
   shell: >
     htpasswd -ci
@@ -101,6 +113,7 @@
     < '{{ openshift_metrics_certs_dir }}/hawkular-metrics.pwd'
   when: >
     not '{{ openshift_metrics_certs_dir }}/hawkular-metrics.htpasswd'|exists
+
 - name: generate the jgroups keystore
   shell: >
     p=$(< '{{ openshift_metrics_certs_dir }}/hawkular-jgroups-keystore.pwd' )
@@ -110,6 +123,7 @@
     -keystore '{{ openshift_metrics_certs_dir }}/hawkular-jgroups.keystore'
   when: >
     not '{{ openshift_metrics_certs_dir }}/hawkular-jgroups.keystore'|exists
+
 - name: read files for the hawkular-metrics secret
   shell: >
     printf '%s: ' '{{ item }}'
@@ -133,10 +147,12 @@
   - hawkular-cassandra.truststore
   - hawkular-cassandra-truststore.pwd
   changed_when: false
+
 - set_fact:
     hawkular_secrets: |
       {{ hawkular_secrets.results|map(attribute='stdout')|join('
       ')|from_yaml }}
+
 - name: generate hawkular-metrics-secrets secret template
   template:
     src: secret.j2
@@ -163,6 +179,8 @@
         {{ hawkular_secrets['hawkular-jgroups-keystore.pwd'] }}
       hawkular-metrics.jgroups.alias: "{{ 'hawkular'|b64encode }}"
   when: name not in metrics_secrets.stdout_lines
+  changed_when: no
+
 - name: generate hawkular-metrics-certificate secret template
   template:
     src: secret.j2
@@ -177,6 +195,8 @@
       hawkular-metrics-ca.certificate: >
         {{ hawkular_secrets['ca.crt'] }}
   when: name not in metrics_secrets.stdout_lines
+  changed_when: no
+
 - name: generate hawkular-metrics-account secret template
   template:
     src: secret.j2
@@ -190,6 +210,8 @@
       hawkular-metrics.password: >
         {{ hawkular_secrets['hawkular-metrics.pwd'] }}
   when: name not in metrics_secrets.stdout_lines
+  changed_when: no
+  
 - name: generate cassandra secret template
   template:
     src: secret.j2
@@ -211,6 +233,8 @@
       cassandra.pem: >
         {{ hawkular_secrets['hawkular-cassandra.pem'] }}
   when: name not in metrics_secrets
+  changed_when: no
+
 - name: generate cassandra-certificate secret template
   template:
     src: secret.j2
@@ -225,3 +249,4 @@
       cassandra-ca.certificate: >
         {{ hawkular_secrets['hawkular-cassandra.pem'] }}
   when: name not in metrics_secrets.stdout_lines
+  changed_when: no

roles/openshift_metrics/tasks/generate_rolebindings.yaml

@@ -12,6 +12,8 @@
     subjects:
     - kind: ServiceAccount
       name: hawkular
+  changed_when: no
+
 - name: generate cluster-reader role binding for the heapster service account
   template:
     src: rolebinding.j2
@@ -28,3 +30,4 @@
     - kind: ServiceAccount
       name: heapster
       namespace: "{{ openshift_metrics_project }}"
+  changed_when: no

roles/openshift_metrics/tasks/generate_serviceaccounts.yaml

@@ -12,6 +12,7 @@
     secret: hawkular-metrics-secrets
   - name: cassandra
     secret: hawkular-cassandra-secrets
+  changed_when: no
 
 - name: Generating serviceaccount for heapster
   template: src=serviceaccount.j2 dest={{mktemp.stdout}}/templates/metrics-{{obj_name}}-sa.yaml
@@ -23,3 +24,4 @@
     - heapster-secrets
     - hawkular-metrics-certificate
     - hawkular-metrics-account
+  changed_when: no

roles/openshift_metrics/tasks/generate_services.yaml

@@ -10,6 +10,7 @@
     labels:
       metrics-infra: "{{obj_name}}"
       name: "{{obj_name}}"
+  changed_when: no
 
 - name: Generate service for hawkular-metrics
   template: src=service.j2 dest={{mktemp.stdout}}/templates/metrics-{{obj_name}}-svc.yaml
@@ -22,6 +23,7 @@
     labels:
       metrics-infra: "{{obj_name}}"
       name: "{{obj_name}}"
+  changed_when: no
 
 - name: Generate services for cassandra
   template: src=service.j2 dest={{mktemp.stdout}}/templates/metrics-{{obj_name}}-svc.yaml
@@ -41,3 +43,5 @@
   with_items:
     - cassandra
     - cassandra-nodes
+  changed_when: no
+

roles/openshift_metrics/tasks/install_hawkular.yaml

@@ -1,8 +1,8 @@
 ---
 - shell: >
-    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project }}
+    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project | quote }}
     --config={{ mktemp.stdout }}/admin.kubeconfig
-    get rc hawkular-metrics --template=\{\{.spec.replicas\}\} || echo 0
+    get rc hawkular-metrics -o jsonpath='{.spec.replicas}' || echo 0
   register: hawkular_metrics_replica_count
   changed_when: false
 
@@ -12,16 +12,17 @@
     dest: "{{ mktemp.stdout }}/templates/hawkular_metrics_rc.yaml"
   vars:
      replica_count: "{{hawkular_metrics_replica_count.stdout}}"
+  changed_when: false
 
 - shell: >
-    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project }}
+    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project | quote }}
     --config={{ mktemp.stdout }}/admin.kubeconfig
-    get rc hawkular-cassandra-{{node}} --template=\{\{.spec.replicas\}\} || echo 0
+    get rc hawkular-cassandra-{{node}} -o jsonpath='{.spec.replicas}' || echo 0
   vars:
     node: "{{ item }}"
   register: cassandra_replica_count
-  changed_when: false
   with_sequence: count={{ openshift_metrics_cassandra_replicas }}
+  changed_when: false
 
 - name: generate hawkular-cassandra replication controllers
   template:
@@ -32,6 +33,7 @@
     master: "{{ (item == '1')|string|lower }}"
     replica_count: "{{cassandra_replica_count.results[item|int - 1].stdout}}"
   with_sequence: count={{ openshift_metrics_cassandra_replicas }}
+  changed_when: false
 
 - name: generate hawkular-cassandra persistent volume claims
   template:
@@ -46,6 +48,7 @@
     size: "{{ openshift_metrics_cassandra_pv_size }}"
   with_sequence: count={{ openshift_metrics_cassandra_replicas }}
   when: openshift_metrics_cassandra_storage_type == 'pv'
+  changed_when: false
 
 - name: generate hawkular-cassandra persistent volume claims (dynamic)
   template:
@@ -62,20 +65,25 @@
     size: "{{ openshift_metrics_cassandra_pv_size }}"
   with_sequence: count={{ openshift_metrics_cassandra_replicas }}
   when: openshift_metrics_cassandra_storage_type == 'dynamic'
+  changed_when: false
 
 - name: read hawkular-metrics route destination ca certificate
   slurp: src={{ openshift_metrics_certs_dir }}/ca.crt
   register: metrics_route_dest_ca_cert
+  changed_when: false
 
 - block:
   - set_fact: hawkular_key={{ lookup('file', openshift_metrics_hawkular_key) }}
     when: openshift_metrics_hawkular_key | exists
+    changed_when: false
 
   - set_fact: hawkular_cert={{ lookup('file', openshift_metrics_hawkular_cert) }}
     when: openshift_metrics_hawkular_cert | exists
+    changed_when: false
 
   - set_fact: hawkular_ca={{ lookup('file', openshift_metrics_hawkular_ca) }}
     when: openshift_metrics_hawkular_ca | exists
+    changed_when: false
 
   - name: generate the hawkular-metrics route
     template:
@@ -95,3 +103,4 @@
         certificate: "{{ hawkular_cert | default('') }}"
         ca_certificate: "{{ hawkular_ca | default('') }}"
         destination_ca_certificate: "{{ metrics_route_dest_ca_cert.content | b64decode }}"
+    changed_when: false

roles/openshift_metrics/tasks/install_heapster.yaml

@@ -1,12 +1,13 @@
 ---
 - shell: >
-    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project }}
+    {{ openshift.common.client_binary }} -n {{ openshift_metrics_project | quote }}
     --config={{ mktemp.stdout }}/admin.kubeconfig
-    get rc heapster --template=\{\{.spec.replicas\}\} || echo 0
+    get rc heapster -o jsonpath='{.spec.replicas}' || echo 0
   register: heapster_replica_count
-  changed_when: false
+  changed_when: no
 
 - name: Generate heapster replication controller
   template: src=heapster.j2 dest={{mktemp.stdout}}/templates/metrics-heapster-rc.yaml
   vars:
     replica_count: "{{heapster_replica_count.stdout}}"
+  changed_when: no

roles/openshift_metrics/tasks/main.yaml

@@ -4,8 +4,6 @@
   register: mktemp
   changed_when: False
 
-- debug: msg="Created temp dir {{mktemp.stdout}}"
-
 - name: Create temp directory for all our templates
   file: path={{mktemp.stdout}}/templates state=directory mode=0755
   changed_when: False
@@ -17,8 +15,8 @@
   check_mode: no
   tags: metrics_init
 
-- include: "{{role_path}}/tasks/install_metrics.yaml"
+- include: install_metrics.yaml
   when: openshift_metrics_install_metrics | default(false) | bool
 
-- include: "{{role_path}}/tasks/uninstall_metrics.yaml"
+- include: uninstall_metrics.yaml
   when: not openshift_metrics_install_metrics | default(false) | bool

roles/openshift_metrics/tasks/scale.yaml

@@ -1,27 +1,30 @@
 ---
-- shell: >
+- command: >
     {{ openshift.common.client_binary }} --config={{ mktemp.stdout }}/admin.kubeconfig get {{object}}
-    --template='{{ '{{.spec.replicas}}' }}' -n {{openshift_metrics_project}}
+    -o jsonpath='{.spec.replicas}' -n {{openshift_metrics_project}}
   register: replica_count
   failed_when: "replica_count.rc == 1 and 'exists' not in replica_count.stderr"
   when: not ansible_check_mode
+  changed_when: no
 
-- shell: >
+- command: >
     {{ openshift.common.client_binary }} --config={{ mktemp.stdout }}/admin.kubeconfig scale {{object}}
     --replicas={{desired}} -n {{openshift_metrics_project}}
   register: scale_result
   failed_when: scale_result.rc == 1 and 'exists' not in scale_result.stderr
   when:
-  - replica_count.stdout != desired
+  - replica_count.stdout != (desired | string)
   - not ansible_check_mode
+  changed_when: no
 
 - name: Waiting for {{object}} to scale to {{desired}}
-  shell: >
-    {{ openshift.common.client_binary }} --config={{ mktemp.stdout }}/admin.kubeconfig describe {{object}} -n {{openshift_metrics_project}} | awk -v statusrx='Pods Status:' '$0 ~ statusrx {print $3}'
+  command: >
+    {{ openshift.common.client_binary }} --config={{ mktemp.stdout }}/admin.kubeconfig 
+    get {{object}} -n {{openshift_metrics_project|quote}} -o jsonpath='{.status.replicas}'
   register: replica_counts
   until: replica_counts.stdout.find("{{desired}}") != -1
   retries: 30
   delay: 10
   when:
-    - replica_count.stdout != desired
+    - replica_count.stdout != (desired | string)
     - not ansible_check_mode

roles/openshift_metrics/tasks/setup_certificate.yaml

@@ -10,19 +10,22 @@
     --signer-key='{{ openshift_metrics_certs_dir }}/ca.key'
     --signer-serial='{{ openshift_metrics_certs_dir }}/ca.serial.txt'
   when: not '{{ openshift_metrics_certs_dir }}/{{ component }}.key'|exists
+
 - name: generate {{ component }} certificate
   shell: >
     cat
-    '{{ openshift_metrics_certs_dir }}/{{ component|quote }}.key'
-    '{{ openshift_metrics_certs_dir }}/{{ component|quote }}.crt'
-    > '{{ openshift_metrics_certs_dir }}/{{ component|quote }}.pem'
+    '{{ openshift_metrics_certs_dir | quote }}/{{ component|quote }}.key'
+    '{{ openshift_metrics_certs_dir | quote }}/{{ component|quote }}.crt'
+    > '{{ openshift_metrics_certs_dir | quote }}/{{ component|quote }}.pem'
   when: not '{{ openshift_metrics_certs_dir }}/{{ component }}.pem'|exists
+
 - name: generate random password for the {{ component }} keystore
   shell: >
     tr -dc _A-Z-a-z-0-9 < /dev/urandom | head -c15
-    > '{{ openshift_metrics_certs_dir }}/{{ component|quote }}-keystore.pwd'
+    > '{{ openshift_metrics_certs_dir | quote }}/{{ component|quote }}-keystore.pwd'
   when: >
     not '{{ openshift_metrics_certs_dir }}/{{ component }}-keystore.pwd'|exists
+
 - name: create the {{ component }} pkcs12 from the pem file
   command: >
     openssl pkcs12 -export
@@ -32,22 +35,24 @@
     -password
     'file:{{ openshift_metrics_certs_dir }}/{{ component }}-keystore.pwd'
   when: not '{{ openshift_metrics_certs_dir }}/{{ component }}.pkcs12'|exists
+
 - name: create the {{ component }} keystore from the pkcs12 file
   shell: >
     p=$(< {{ openshift_metrics_certs_dir }}/{{ component }}-keystore.pwd)
     &&
     keytool -v -importkeystore
-    -srckeystore '{{ openshift_metrics_certs_dir }}/{{ component }}.pkcs12'
+    -srckeystore '{{ openshift_metrics_certs_dir | quote }}/{{ component | quote }}.pkcs12'
     -srcstoretype PKCS12
-    -destkeystore '{{ openshift_metrics_certs_dir }}/{{ component }}.keystore'
+    -destkeystore '{{ openshift_metrics_certs_dir | quote }}/{{ component | quote}}.keystore'
     -deststoretype JKS
     -deststorepass "$p"
     -srcstorepass "$p"
   when: not '{{ openshift_metrics_certs_dir }}/{{ component }}.keystore'|exists
+
 - name: generate random password for the {{ component }} truststore
   shell: >
     tr -dc _A-Z-a-z-0-9 < /dev/urandom | head -c15
-    > '{{ openshift_metrics_certs_dir }}/{{ component|quote }}-truststore.pwd'
+    > '{{ openshift_metrics_certs_dir | quote }}/{{ component|quote }}-truststore.pwd'
   when: >
     not
-    '{{ openshift_metrics_certs_dir }}/{{ component }}-truststore.pwd'|exists
+    '{{ openshift_metrics_certs_dir | quote }}/{{ component| quote  }}-truststore.pwd'|exists

roles/openshift_metrics/tasks/start_metrics.yaml

@@ -1,5 +1,5 @@
 ---
-- shell: >
+- command: >
     {{openshift.common.client_binary}} 
     --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc 
@@ -7,6 +7,7 @@
     -o name 
     -n {{openshift_metrics_project}}
   register: metrics_cassandra_rc
+  changed_when: no
 
 - name: Start Hawkular Cassandra
   include: scale.yaml
@@ -16,7 +17,7 @@
   loop_control:
     loop_var: object
 
-- shell: >
+- command: >
     {{openshift.common.client_binary}} 
     --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc 
@@ -24,6 +25,7 @@
     -o name 
     -n {{openshift_metrics_project}}
   register: metrics_metrics_rc
+  changed_when: no
 
 - name: Start Hawkular Metrics
   include: scale.yaml
@@ -33,7 +35,7 @@
   loop_control:
     loop_var: object
 
-- shell: >
+- command: >
     {{openshift.common.client_binary}} 
     --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc 

roles/openshift_metrics/tasks/stop_metrics.yaml

@@ -1,5 +1,5 @@
 ---
-- shell: >
+- command: >
     {{openshift.common.client_binary}} 
     --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc 
@@ -18,7 +18,7 @@
   loop_control:
     loop_var: object
 
-- shell: >
+- command: >
     {{openshift.common.client_binary}} 
     --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc
@@ -36,7 +36,7 @@
   loop_control:
     loop_var: object
 
-- shell: >
+- command: >
     {{openshift.common.client_binary}} --config={{mktemp.stdout}}/admin.kubeconfig 
     get rc
     -o name

roles/openshift_metrics/templates/hawkular_cassandra_rc.j2

@@ -75,25 +75,25 @@ spec:
    or (openshift_metrics_cassandra_requests_memory is defined and openshift_metrics_cassandra_requests_memory is not none)) 
 %}
         resources:
-{% if (openshift_metrics_cassandra_limits_cpu is not none
-   or openshift_metrics_cassandra_limits_memory is not none)
+{%      if (openshift_metrics_cassandra_limits_cpu is not none
+        or openshift_metrics_cassandra_limits_memory is not none)
 %}
           limits:
-{% if openshift_metrics_cassandra_limits_cpu is not none %}
+{%        if openshift_metrics_cassandra_limits_cpu is not none %}
             cpu: "{{openshift_metrics_cassandra_limits_cpu}}"
 {% endif %}
-{% if openshift_metrics_cassandra_limits_memory is not none %}
+{%        if openshift_metrics_cassandra_limits_memory is not none %}
             memory: "{{openshift_metrics_cassandra_limits_memory}}"
 {% endif %}
 {% endif %}
-{% if (openshift_metrics_cassandra_requests_cpu is not none 
-   or openshift_metrics_cassandra_requests_memory is not none) 
+{%        if (openshift_metrics_cassandra_requests_cpu is not none 
+          or openshift_metrics_cassandra_requests_memory is not none) 
 %}
           requests:
-{% if openshift_metrics_cassandra_requests_cpu is not none %}
+{%        if openshift_metrics_cassandra_requests_cpu is not none %}
             cpu: "{{openshift_metrics_cassandra_requests_cpu}}"
 {% endif %}
-{% if openshift_metrics_cassandra_requests_memory is not none %}
+{%        if openshift_metrics_cassandra_requests_memory is not none %}
             memory: "{{openshift_metrics_cassandra_requests_memory}}"
 {% endif %}
 {% endif %}
@@ -114,9 +114,9 @@ spec:
         terminationGracePeriodSeconds: 1800
       volumes:
       - name: cassandra-data
-{% if openshift_metrics_cassandra_storage_type == 'emptydir' %}
+{%      if openshift_metrics_cassandra_storage_type == 'emptydir' %}
         emptyDir: {}
-{% else %}
+{%      else %}
         persistentVolumeClaim:
           claimName: "{{ openshift_metrics_cassandra_pv_prefix }}-{{ node }}"
 {% endif %}