Fix dry runs (--check) erroneously reporting changes

[shaunsmiley-xevo]

SUMMARY

Fix --check mode reporting changes when there are none.

These issues are possibly related:
Previously -

• k8s check mode does not work very well ansible/ansible#66870
• k8s module: bad diff in check mode when creating a service with apply:yes ansible/ansible#67653

Now -
#189
#190

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

raw.py

ADDITIONAL INFORMATION

We know that we can run an ansible playbook using the k8s module over and over and see no changes (i.e. reports OK, not CHANGED). However, we decided to add an additional step to validate which k8s tasks would change and what the changes were before executing with --check mode.

We found that when we turned --check mode on from the cli, the k8s module reported changes every time, even when there would be no changes. We confirmed this by running again without check mode seeing everything show OK (for 20+ different k8s manifests) then seeing everything show CHANGED with --check mode enabled.

This fix also required a fix in the openshift lib. That PR is here: openshift/openshift-restclient-python#360

We are using both patches locally, and have tested internally that it is now working as expected.

[codecov]

Codecov Report

Merging #99 into main will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   42.50%   42.37%   -0.14%     
==========================================
  Files           4        4              
  Lines         614      616       +2     
  Branches      122      123       +1     
==========================================
  Hits          261      261              
- Misses        306      307       +1     
- Partials       47       48       +1     

Impacted Files	Coverage Δ
...s/community/kubernetes/plugins/module_utils/raw.py	43.30% <0.00%> (-0.28%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51cadb7...608734a. Read the comment docs.

[geerlingguy]

@shaunsmiley-xevo - Would #84 resolve this issue as well?

[fabianvf]

Since this will require an updated version of the openshift client to work properly, would it be possible to fall back to the old behavior unless you have openshift >= 0.11.1?

edit: Also I believe dryRun was added in kubernetes 1.18, so we should probably fall back to the old behavior for kubernetes versions < 1.18 as well

[willthames]

@fabianvf given that this passes all test cases, we should probably ensure we have more test cases for check mode

[mgogoi]

@shaunsmiley-xevo - Would #84 resolve this issue as well?

We tested this and #84 didn't fix the dry run issue.

[mgogoi]

Since this will require an updated version of the openshift client to work properly, would it be possible to fall back to the old behavior unless you have openshift >= 0.11.1?

edit: Also I believe dryRun was added in kubernetes 1.18, so we should probably fall back to the old behavior for kubernetes versions < 1.18 as well

We are working on a fix to revert back to the old logic if the openshift version < 0.11.1.
The dry run feature in kubernetes has been there for a while now (since v1.13). So, I feel its probably ok to pass dry_run to openshift,

[fabianvf]

@mgogoi makes sense, thanks a ton!

[willthames]

My thoughts on this are that this is usually a bug in our implementation of apply in the openshift library and all such bugs should be eliminated there (or here where it's not a bug in the openshift library).

If there are outstanding test cases, then I'd be interested to see them (the test suite for apply is getting more and more extensive as I eliminate such bugs as I find them).

I'm already running ansible-playbook -CDvvv kubernetes-playbook.yml for a suitably large set of resources as a validation of this approach and finding very few false positive changes these days.

[shaunsmiley-xevo]

@mgogoi's PR on our fork to revert behavior if openshift version is < 0.11.1 has been tested on our end and merged. It is now included in this PR.

[fabianvf]

@willthames I think if we can use the dry run API it makes sense to, it can only be more accurate than whatever we do client side

[willthames]

Can we have some tests that exercise this? Not really sure how best to test the matrix of old and new openshift versions and old and new kubernetes clusters automatically but the tests would be a start, can always run the variations manually.

[mgogoi]

@willthames - I am a bit unsure how to execute those tests. Could you please help me run those before I could add some new ones?

[willthames]

@mgogoi sure, happy to help. Basically you need any kubernetes cluster (e.g. docker desktop, minikube, KinD) up and running. Install molecule however best suits your python setup (mine is in a virtualenv called ansible-kubernetes-testing). Then just run molecule test - it will pick up the kubernetes cluster from your current kubeconfig context