Moving replica logic to filter_plugin to fix skipped task variable behavior. #3503
Conversation
kwoodson
force-pushed
the
openshift_hosted_replicas
branch
from
e53dbc4 to
3b87782
Compare
|
Fixes issue from #3423 where the replica value, when set, caused the router step to fail. |
|
aos-ci-test |
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| - name: set_fact replicas | |||
| set_fact: | |||
| replicas: "{{ openshift.hosted.router.replicas | default(router_nodes.results.results[0]['items'] | length) }}" | |||
| replicas: "{{ openshift.hosted.router.replicas|default(None) | get_router_replicas(router_nodes) }}" | |||
There was a problem hiding this comment.
👍 on moving to a filter plugin.
I think it might be good to switch the order of the parameters, though. It just seems odd to filter on an integer and provide a list parameter instead of filtering on a list and providing an int param to override.
- set_fact:
replicas: "{{ router_nodes | get_router_replicas(openshift.hosted.router.replicas | default(None) }}"
There was a problem hiding this comment.
@detiber, I think that since the first parameter takes precedence I'd prefer it as the one being piped. The integer you refer to isn't always set.
There was a problem hiding this comment.
I'm just thinking that filters are meant to translate data, and translating from a value that is either an int or None to an int seems a bit odd and could be confusing for someone not already familiar with the code.
| 'results' in router_nodes['results'] and | ||
| 'items' in router_nodes['results']['results']): | ||
|
|
||
| return len(router_nodes['results']['results'][0]['items']) |
There was a problem hiding this comment.
Would it be better to not be so tightly coupled to the module result implementation here? It looks like if the module result format changes, then we could return 1 instead of the number of nodes here.
There was a problem hiding this comment.
Using router_nodes.results.results[0]['items'] in the playbook is ugly, but it would at least throw an error if the return format changed.
There was a problem hiding this comment.
Th discussion around normalizing return output is on the table post 3.5. It currently would require a significant refactor to openshift-ansible to all the places that call oc_obj. I'm fine with changing it but it won't get done until post 3.5.
I won't argue with the ugliness of it but again, this is for 3.5. I would want to know if the format changed because the filter_plugin is specific. I want those tied together tightly.
There was a problem hiding this comment.
I would argue that we shouldn't tightly couple the filter_plugins to implementation details of the modules.
If the team decides it is ok to create tightly coupled filters to match the modules, then I would suggest that they should live together and be tested together. I would also suggest that they fail if the expected data structure changes, rather than returning an incorrect value. With the current implementation, if openshift.hosted.router.replicas is unset and the return value of oc_obj changes, then the filter would return 1.
There was a problem hiding this comment.
@detiber, I think this is a single purpose item for the filter_plugin and tightly coupling is exactly what we want since the value is very specific.
I see your point that if the structure changed you wouldn't receive what you thought. That is not a good solution. The question enters in, how do we know when it doesn't have the key that we want? That's the difficult question. If we run them together and test them together then we'll at least have some confidence in them. The issue is that when the previous task is skipped the variable from the register gets written in with data that's not the same as the structure would have returned with the oc_obj query.
Maybe a better solution would be to check to see if the skip occurred and then return the data:
if 'skipped' not in router_nodes: return router_nodes['results']['results'][0]['items']
This is all subject to change in the next 3.6 release as the openshift_hosted role will be completely refactored.
If we did it this way we guarantee the output to be tied with the module output.
|
3b87782 - 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-1012/3b877826bc214c7e1e952b1968f933c40e477f50.txt |
|
The previous version of this passed for me. |
|
(the existing PR, that is) |
|
3b87782 - 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-1016/3b877826bc214c7e1e952b1968f933c40e477f50.txt |
|
3b87782 - 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-1016/3b877826bc214c7e1e952b1968f933c40e477f50.txt |
|
3b87782 - 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-1016/3b877826bc214c7e1e952b1968f933c40e477f50.txt |
|
3b87782 - 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-1016/3b877826bc214c7e1e952b1968f933c40e477f50.txt |
|
@kwoodson can you open either a followup PR or issue to address feedback raised, i've merged this as the fix as is was verified to unblock other merge queues downstream of openshift-ansible. |
No description provided.