Adding changed_whens for role, rolebinding, and scc reconciliation ba… #3517

ewolinetz · 2017-02-27T22:00:20Z

…sed on output from oadm policy command

Encompasses https://trello.com/c/5Yb3cOk1/372-2-3-idempotency-reconcile-role-s-bindings
@tbielawa @detiber

tbielawa

These changes are required to meet the defined acceptance criteria

tbielawa · 2017-02-28T16:08:14Z

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

-      policy reconcile-cluster-roles --additive-only=true --confirm
+      policy reconcile-cluster-roles --additive-only=true --confirm -o name
+    register: reconcile_cluster_role_result
+    changed_when: reconcile_cluster_role_result.stdout.length > 0


Missing part of the acceptance criteria

The changed_when parameter is added to each task such that it is only true when there is output from the task and the return code is 0

For example:

- name: Reconcile Cluster Roles command: > {{ openshift.common.client_binary }} adm --config={{ openshift.common.config_base }}/master/admin.kubeconfig policy reconcile-cluster-roles --additive-only=true --confirm policy reconcile-cluster-roles --additive-only=true --confirm -o name register: reconcile_cluster_role_result changed_when: - reconcile_cluster_role_result.stdout.length > 0 - reconcile_cluster_role_result.rc == 0

would be more like what we're looking for

I must have missed that in the AC, will update

Shouldn't the return value impact failed_when not changed_when? I guess I'm not seeing the reasoning for comparing the rc for changed_when in this case.

We want to also check the rc for the changed_when because -o name will tell us what objects need a change, not whether or not they did change (per IRC discussion with liggit).

I would agree that we would also want to have a failed_when that examined the rc != 0

Ah, that makes sense.

tbielawa · 2017-02-28T16:08:48Z

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

    when: origin_reconcile_bindings | bool or ent_reconcile_bindings | bool
+    register: reconcile_bindings_result
+    change_when: reconcile_bindings_result.stdout.length > 0


same as above

tbielawa · 2017-02-28T16:08:53Z

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

    run_once: true
+    register: reconcile_jenkens_role_binding_result
+    changed_when: reconcile_jenkins_role_binding_result.stdout.length > 0


tbielawa · 2017-02-28T16:08:59Z

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

-      {{ openshift.common.client_binary }} adm policy reconcile-sccs --confirm --additive-only=true
+      {{ openshift.common.client_binary }} adm policy reconcile-sccs --confirm --additive-only=true -o name
+    register: reconcile_scc_result
+    changed_when: reconcile_scc_result.stdout.length > 0


ewolinetz · 2017-02-28T16:21:09Z

@tbielawa updated changed_when to also verify that the rc == 0

tbielawa · 2017-02-28T16:30:05Z

aos-ci-test

openshift-bot · 2017-02-28T16:53:41Z

6e8a061 - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-1024/6e8a061e0fd9d597af4830f6bd97dd06e7b9cf62.txt

openshift-bot · 2017-02-28T17:17:38Z

6e8a061 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1028/6e8a061e0fd9d597af4830f6bd97dd06e7b9cf62.txt

openshift-bot · 2017-02-28T17:19:47Z

6e8a061 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1028/6e8a061e0fd9d597af4830f6bd97dd06e7b9cf62.txt

openshift-bot · 2017-02-28T17:23:11Z

6e8a061 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1028/6e8a061e0fd9d597af4830f6bd97dd06e7b9cf62.txt

openshift-bot · 2017-02-28T17:24:59Z

6e8a061 - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1028/6e8a061e0fd9d597af4830f6bd97dd06e7b9cf62.txt

ewolinetz · 2017-02-28T18:40:14Z

reverted failed_when commit

…sed on output from oadm policy command

ewolinetz · 2017-03-01T21:48:17Z

aos-ci-test

openshift-bot · 2017-03-01T22:01:40Z

6a0c52a - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-1031/6a0c52a0642b1e962246633bf6bb8a0cde3930ba.txt

openshift-bot · 2017-03-01T22:25:03Z

6a0c52a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1035/6a0c52a0642b1e962246633bf6bb8a0cde3930ba.txt

openshift-bot · 2017-03-01T22:27:26Z

6a0c52a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1035/6a0c52a0642b1e962246633bf6bb8a0cde3930ba.txt

openshift-bot · 2017-03-01T22:30:28Z

6a0c52a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1035/6a0c52a0642b1e962246633bf6bb8a0cde3930ba.txt

openshift-bot · 2017-03-01T22:33:09Z

6a0c52a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1035/6a0c52a0642b1e962246633bf6bb8a0cde3930ba.txt

ewolinetz · 2017-03-01T23:08:57Z

aos-ci-test

openshift-bot · 2017-03-01T23:16:35Z

f7c801f - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-1032/f7c801f77284da83ccc18aee771e11ce17f59dd2.txt

openshift-bot · 2017-03-01T23:40:05Z

f7c801f - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1036/f7c801f77284da83ccc18aee771e11ce17f59dd2.txt

openshift-bot · 2017-03-01T23:42:29Z

f7c801f - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1036/f7c801f77284da83ccc18aee771e11ce17f59dd2.txt

openshift-bot · 2017-03-01T23:46:07Z

f7c801f - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1036/f7c801f77284da83ccc18aee771e11ce17f59dd2.txt

openshift-bot · 2017-03-01T23:47:59Z

f7c801f - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1036/f7c801f77284da83ccc18aee771e11ce17f59dd2.txt

tbielawa · 2017-03-02T17:22:04Z

I'm running two tests from this branch. A first test to get a base-line for the timing and number of changes. A second test to verify the results from the first. Hopefully should be done in about 25-30 minutes once I examine the ARA report.

tbielawa · 2017-03-02T17:33:00Z

Test run fails for me @ewolinetz

TASK [openshift_cli : Install bash completion for oc tools] ********************
ok: [m01.example.com] => {"changed": false, "msg": "", "rc": 0, "results": ["bash-completion-1:2.1-6.el7.noarch providing bash-completion is already installed"]}

TASK [Reconcile Cluster Roles] *************************************************
fatal: [m01.example.com]: FAILED! => {"failed": true, "msg": "The conditional check 'reconcile_cluster_role_result.stdout.length > 0' failed. The error was: error while evaluating conditional (reconcile_cluster_role_result.stdout.length > 0): 'ansible.vars.unsafe_proxy.AnsibleUnsafeText object' has no attribute 'length'"}

NO MORE HOSTS LEFT *************************************************************

PLAY RECAP *********************************************************************
localhost                  : ok=34   changed=0    unreachable=0    failed=0
m01.example.com            : ok=245  changed=10   unreachable=0    failed=1
n01.example.com            : ok=100  changed=1    unreachable=0    failed=0
n02.example.com            : ok=100  changed=1    unreachable=0    failed=0

Maybe try with reconcile_cluster_role_result.stdout != '' ?

EDIT removed .length from my suggestion

tbielawa

Upgrade test fails on invalid when test statements.

detiber · 2017-03-02T17:59:35Z

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

@@ -192,7 +192,7 @@
    when: origin_reconcile_bindings | bool or ent_reconcile_bindings | bool
    register: reconcile_bindings_result
    changed_when:
-    - reconcile_bindings_result.stdout.length > 0
+    - reconcile_bindings_result.stdout != ''


Just a bit, but I think the default filter might be better here.

Or maybe even just dropping the == ''...

we would need some way to verify whether or not the output is empty which means that there weren't any roles to update. I'm not sure using default or dropping the comparison would allow us to still accomplish this...

@ewolinetz I meant something like this:

changed_when: - reconcile_bindings_results.stdout

Then empty string would evaluate to False

or if it is possible that stdout is not present:

changed_when: - reconcile_bindings_result.stdout | default(false, boolean=true)

ack, that's pretty cool; i didn't realize that we could do that. So it would be something like

changed_when: - not reconcile_bindings_results.stdout

However I think this makes the check non-obvious for people that don't realize you can do that. It would probably be better if we had a way to test like

changed_when: - not reconcile_bindings_result.stdout | empty

There are many different ways to ~~shave this yack~~ write these when conditions

I don't care which one we use but we need to make a decision

I personally prefer the last one @detiber posted, it handles a lot of edge cases

changed_when: - reconcile_bindings_result.stdout | default(false, boolean=true)

If stdout is empty for some reason then the default is false. Otherwise the result is true.

stdout == '' → Nothing changed → default filter says no input value, so this is false

stdout != '' → Something changed changed → default filter says input value present, so this is true

@detiber @ewolinetz if that translation is correct ^, let's just do that and move on with this the way @detiber wrote it up.

tbielawa · 2017-03-02T18:05:42Z

We talked about this on IRC, just noting it here for transparency:

TASK [Reconcile Jenkins Pipeline Role Bindings] ********************************
fatal: [m01.example.com]: FAILED! => {"failed": true, "msg": "The conditional check 'reconcile_jenkins_role_binding_result.stdout != ''' failed. The error was: error while evaluating conditional (reconcile_jenkins
_role_binding_result.stdout != ''): 'reconcile_jenkins_role_binding_result' is undefined"}

tbielawa · 2017-03-02T18:08:35Z

Restesting w/ updates

ewolinetz · 2017-03-02T21:43:57Z

aos-ci-test

tbielawa · 2017-03-02T21:48:05Z

Tests are quite satisfying.

Those 9 result rows were (in order) ran against the following branches

This PR
This PR
Master Branch
Master Branch
This PR
This PR
Master Branch
Master Branch
This PR

Examining the changed tasks in the PR vs. Master branch results side-by-side we can see that the tasks we fixed in this PR no longer appear as changing in the PR branch

Confirming this PR reduces changed by 5 vs the master branch @ openshift-ansible-3.5.3-1-466-g68aa6f2.

👍

openshift-bot · 2017-03-02T21:52:24Z

352917a - State: success - All Test Contexts: aos-ci-jenkins/OS_unit_tests - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-2-unit-tests-1046/352917ae21424ba518d0fe4513dcc540c7698ae4.txt

openshift-bot · 2017-03-02T22:15:36Z

352917a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_NOT_containerized, aos-ci-jenkins/OS_3.4_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1050/352917ae21424ba518d0fe4513dcc540c7698ae4.txt

openshift-bot · 2017-03-02T22:18:18Z

352917a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.4_containerized, aos-ci-jenkins/OS_3.4_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.4,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1050/352917ae21424ba518d0fe4513dcc540c7698ae4.txt

openshift-bot · 2017-03-02T22:20:55Z

352917a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_NOT_containerized, aos-ci-jenkins/OS_3.5_NOT_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_NOT_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster,TargetBranch=master,nodes=openshift-ansible-slave-1050/352917ae21424ba518d0fe4513dcc540c7698ae4.txt

openshift-bot · 2017-03-02T22:23:12Z

352917a - State: success - All Test Contexts: "aos-ci-jenkins/OS_3.5_containerized, aos-ci-jenkins/OS_3.5_containerized_e2e_tests" - Logs: https://aos-ci.s3.amazonaws.com/openshift/openshift-ansible/jenkins-openshift-ansible-3-test-matrix-CONTAINERIZED=_containerized,OSE_VER=3.5,PYTHON=System-CPython-2.7,TOPOLOGY=openshift-cluster-containerized,TargetBranch=master,nodes=openshift-ansible-slave-1050/352917ae21424ba518d0fe4513dcc540c7698ae4.txt

ewolinetz · 2017-03-06T22:59:35Z

I didn't get a chance to work on a test plugin for checking std{out,err}; so we can merge this now and i'll take it on as tech debt/a nice to have

detiber approved these changes Feb 27, 2017

View reviewed changes

tbielawa suggested changes Feb 28, 2017

View reviewed changes

ewolinetz force-pushed the idempotency_role_bindings branch from 218f299 to 6e8a061 Compare February 28, 2017 16:19

tbielawa approved these changes Feb 28, 2017

View reviewed changes

ewolinetz force-pushed the idempotency_role_bindings branch from de87f91 to 6e8a061 Compare February 28, 2017 18:35

Adding changed_whens for role, rolebinding, and scc reconciliation ba…

6a0c52a

…sed on output from oadm policy command

ewolinetz force-pushed the idempotency_role_bindings branch from 6e8a061 to 6a0c52a Compare February 28, 2017 18:46

Resolving yammlint errors

f7c801f

ewolinetz added the area/idempotency label Mar 2, 2017

tbielawa suggested changes Mar 2, 2017

View reviewed changes

detiber reviewed Mar 2, 2017

View reviewed changes

Updating stdout check for changed_when

352917a

ewolinetz force-pushed the idempotency_role_bindings branch from af2dec0 to 352917a Compare March 2, 2017 18:05

tbielawa approved these changes Mar 2, 2017

View reviewed changes

ewolinetz merged commit f017f5a into openshift:master Mar 6, 2017

ewolinetz deleted the idempotency_role_bindings branch July 26, 2017 15:07

Adding changed_whens for role, rolebinding, and scc reconciliation ba… #3517

Adding changed_whens for role, rolebinding, and scc reconciliation ba… #3517

Conversation

ewolinetz commented Feb 27, 2017

tbielawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewolinetz commented Feb 28, 2017

tbielawa commented Feb 28, 2017

openshift-bot commented Feb 28, 2017

openshift-bot commented Feb 28, 2017

openshift-bot commented Feb 28, 2017

openshift-bot commented Feb 28, 2017

openshift-bot commented Feb 28, 2017

ewolinetz commented Feb 28, 2017

ewolinetz commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

ewolinetz commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

openshift-bot commented Mar 1, 2017

tbielawa commented Mar 2, 2017

tbielawa commented Mar 2, 2017 • edited

tbielawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbielawa commented Mar 2, 2017

tbielawa commented Mar 2, 2017

ewolinetz commented Mar 2, 2017

tbielawa commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

openshift-bot commented Mar 2, 2017

ewolinetz commented Mar 6, 2017 • edited

tbielawa commented Mar 2, 2017 •

edited

ewolinetz commented Mar 6, 2017 •

edited