Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

k8s: Fix .to_dict not needed #56147

Merged
merged 2 commits into from
Jun 1, 2019
Merged

k8s: Fix .to_dict not needed #56147

merged 2 commits into from
Jun 1, 2019

Conversation

jfrabaute
Copy link
Contributor

@jfrabaute jfrabaute commented May 6, 2019

SUMMARY

Fixes small bug in k8s module

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

k8s

ADDITIONAL INFORMATION

Trying to deploy a simple ConfigMap with the "k8s" module and "force" set to true should reproduce the error.

jfrabaute referenced this pull request May 6, 2019
Provide wait and wait_timeout parameters and wait for certain
resource kinds to become available.
@ansibot
Copy link
Contributor

ansibot commented May 6, 2019

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. clustering Clustering category community_review In order to be merged, this PR must follow the community review workflow. k8s needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch support:community This issue/PR relates to code supported by the Ansible community. labels May 6, 2019
@willthames
Copy link
Contributor

There should be a test case for this under test/integration/targets/k8s - could you add one please?

I'm not 100% convinced this is correct in all cases (e.g. when wait is not set, result['result'] will be k8s_obj, which I'd think needs .to_dict), but a new test case along with the existing test suite should confirm if more changes are required (it may well be that the test suite doesn't check force)

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label May 7, 2019
@jfrabaute
Copy link
Contributor Author

There should be a test case for this under test/integration/targets/k8s - could you add one please?

Yes, I'll work on this. One question.
The README in this folder says:

cd test/integration/targets/k8s
./runme.sh -vv

But there is no "runme.sh" in this particular folder.
https://github.com/ansible/ansible/tree/devel/test/integration/targets/k8s

I'm not 100% convinced this is correct in all cases (e.g. when wait is not set, result['result'] will be k8s_obj, which I'd think needs .to_dict),

This is what was happening for me. I don't have "wait" set and it was raising an exception. I checked and "k8s_obj" is a dict when not set.
It is also a dict when set.

but a new test case along with the existing test suite should confirm if more changes are required (it may well be that the test suite doesn't check force)

I'll try to add a test on the existing code to reproduce the problem I was having.

@willthames
Copy link
Contributor

Yeah, that's a little out of date (I wrote the original README and then runme.sh got removed by someone else).

I think

cd test/integration/targets
ansible -m include_role -a name=k8s localhost

might do the trick but not 100% sure - I'll take a look later if not.

@willthames
Copy link
Contributor

The tests really aren't working very well at the moment, I'm disappointed that the result of the refactor wasn't properly tested.

I'll have a PR up for that shortly which will be a better basis for the tests needed for this PR

@willthames
Copy link
Contributor

willthames commented May 7, 2019

I had to create #56163 for ansible -m include_role being broken

In the mean time I've been using the following playbook:

- hosts: localhost
  connection: local
  gather_facts: no
  vars:
    ansible_python_interpreter: "{{ ansible_playbook_python }}"
    k8s_openshift: no

  roles:
    - test/integration/targets/k8s

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 16, 2019
@geerlingguy
Copy link
Contributor

I believe we may be running into this in some circumstances even when we are not using force; we're ending up with an error like:

TASK [Update the secret in the namespace] **************************************
task path: /var/jenkins_home/workspace/job-name/src/k8s-update-secret/main.yml:51
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: jenkins
<127.0.0.1> EXEC /bin/sh -c 'echo ~jenkins && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585 `" && echo ansible-tmp-1558125808.4-215671049649585="` echo /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585 `" ) && sleep 0'
<127.0.0.1> Attempting python interpreter discovery
<127.0.0.1> EXEC /bin/sh -c 'echo PLATFORM; uname; echo FOUND; command -v '"'"'/usr/bin/python'"'"'; command -v '"'"'python3.7'"'"'; command -v '"'"'python3.6'"'"'; command -v '"'"'python3.5'"'"'; command -v '"'"'python2.7'"'"'; command -v '"'"'python2.6'"'"'; command -v '"'"'/usr/libexec/platform-python'"'"'; command -v '"'"'/usr/bin/python3'"'"'; command -v '"'"'python'"'"'; echo ENDFOUND && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python && sleep 0'
<127.0.0.1> Python interpreter discovery fallback (unsupported Linux distribution: debian)
Using module file /usr/local/lib/python2.7/dist-packages/ansible/modules/clustering/k8s/k8s.py
<127.0.0.1> PUT /var/jenkins_home/.ansible/tmp/ansible-local-10668DHEDp3/tmpWKS8g0 TO /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/ /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
Traceback (most recent call last):
  File "/var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py", line 114, in <module>
    _ansiballz_main()
  File "/var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py", line 106, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File "/var/jenkins_home/.ansible/tmp/ansible-tmp-1558125808.4-215671049649585/AnsiballZ_k8s.py", line 49, in invoke_module
    imp.load_module('__main__', mod, module, MOD_DESC)
  File "/tmp/ansible_k8s_payload_UTeKfv/__main__.py", line 258, in <module>
  File "/tmp/ansible_k8s_payload_UTeKfv/__main__.py", line 254, in main
  File "/tmp/ansible_k8s_payload_UTeKfv/ansible_k8s_payload.zip/ansible/module_utils/k8s/raw.py", line 174, in execute_module
  File "/tmp/ansible_k8s_payload_UTeKfv/ansible_k8s_payload.zip/ansible/module_utils/k8s/raw.py", line 321, in perform_action
AttributeError: 'dict' object has no attribute 'to_dict'

@willthames
Copy link
Contributor

@geerlingguy if you're hitting it without force, that's a different issue and should be raised as such.

That said, I might have fixed your issue with #56168

@fabianvf
Copy link
Contributor

@jfrabaute let me know if you have any difficulties with that test case

@willthames
Copy link
Contributor

Rebased and added a changelog fragment so we can backport this to 2.8 - I do have a working test for this but I'll raise that separately so that the backport doesn't need to include the test suite changes

@ansibot ansibot removed small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jun 1, 2019
@willthames willthames merged commit 7364e79 into ansible:devel Jun 1, 2019
@willthames
Copy link
Contributor

Thanks @jfrabaute!

willthames pushed a commit to willthames/ansible that referenced this pull request Jun 1, 2019
* k8s: Fix .to_dict not needed

* Add changelog fragment

(cherry picked from commit 7364e79)
willthames added a commit to willthames/ansible that referenced this pull request Jun 1, 2019
Tests are separate for easier backporting
@jfrabaute
Copy link
Contributor Author

Sorry, I haven't taken the time to work on the test after sending the PR.
Thank you for the help and merging it!

abadger pushed a commit that referenced this pull request Jun 3, 2019
* k8s: Fix .to_dict not needed

* Add changelog fragment

(cherry picked from commit 7364e79)
pull bot pushed a commit to madonius/ansible that referenced this pull request Jun 6, 2019
Tests are separate for easier backporting
nijinashok pushed a commit to nijinashok/ansible that referenced this pull request Jun 6, 2019
Tests are separate for easier backporting
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. clustering Clustering category community_review In order to be merged, this PR must follow the community review workflow. k8s new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants