Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Add k8s_exec module to execute command through the API #14

Merged
merged 12 commits into from
Feb 14, 2020

Conversation

TristanCacqueray
Copy link
Contributor

This change adds a new module to execute command in a container
through the API:
https://docs.okd.io/latest/dev_guide/executing_remote_commands.html#protocol

Related: ansible/ansible#55029
Closes: #7

@geerlingguy
Copy link
Collaborator

(Just noting that the test failures here are due to upstream issues in the ansible-base repository; the issues should hopefully be cleared up by end of week, and then one of the maintainers can re-run the GitHub Actions workflow to see if it passes.)

Also, I would like to have a test or two using k8s_exec added to the integration tests in this repo (just to make sure it's working and stays working with future changes).

@fabianvf
Copy link
Collaborator

fabianvf commented Feb 6, 2020

@geerlingguy how are we doing the tests? If it's the same way as ansible/ansible then it won't work because it's just spinning up an API server, no containers are actually being run. We should probably rewrite the tests to make use of kind or something like that so we can do full integration tests

@geerlingguy
Copy link
Collaborator

geerlingguy commented Feb 6, 2020

@fabianvf - Ah, in that case yes. And I'd say #10 is super-necessary in that case, to make our test coverage way nicer.

I know you had ansible/ansible#61466 lined up in ansible/ansible... time to pull the trigger? (Though we'd still need the change for ansible-test to work—but I might be able to pummel that through).

(We can also consider using Molecule, so we drop the dependency on ansible-test / ansible/ansible.)

@fabianvf
Copy link
Collaborator

fabianvf commented Feb 6, 2020

yeah, that could do it, we could also just move to using the official kind tool. Basically our test pipeline would then be

install kind -> kind create cluster -> invoke tests, I'd hazard a guess that a lot of what ansible-test gets you doesn't help us a ton since we have pretty simple host management and mostly just need to interact with a kubernetes API. In the long run I plan to make a kind molecule driver, but for now we could pretty easily just stand it up manually and either invoke molecule with the delegated driver or just run the test playbook by hand

edit: We should probably move the discussion to #10, k8s_exec module looks great 👍

@geerlingguy
Copy link
Collaborator

@fabianvf - I like that approach (using official CNCF kind). Do you have time to work on that? Otherwise I may be able to work on it next week. Just don't want to duplicate efforts!

@fabianvf
Copy link
Collaborator

fabianvf commented Feb 6, 2020

@geerlingguy I'll be out on PTO next week so go for it! I'm happy to help out when I get back though. The new cluster scenario in the operator-sdk scaffolding uses this pattern (we currently use it with kind in our CI)

@fabianvf
Copy link
Collaborator

fabianvf commented Feb 7, 2020

@TristanCacqueray you could still do something like what I've done for the k8s_log module, where I added tests and verified locally but didn't import them, so they won't be run in CI: https://github.com/ansible-collections/kubernetes/pull/16/files#diff-31a24fedeb58002db40bc88400eec0feR1

@TristanCacqueray
Copy link
Contributor Author

@fabianvf alright, b4f2e88 does that. i haven't tested the task though, i'd like to wait for ci to validate the tasks.

@geerlingguy
Copy link
Collaborator

@TristanCacqueray - If you get a chance, can you rebase, just to make sure tests pass? I'm still working on the new kind-cluster-based tests, but at least the tests should pass since #13 is resolved.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   43.28%   43.28%           
=======================================
  Files           3        3           
  Lines         536      536           
  Branches      109      109           
=======================================
  Hits          232      232           
  Misses        261      261           
  Partials       43       43
Flag Coverage Δ
#_ 43.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 f0ecc4c...5f1b88b. Read the comment docs.

@TristanCacqueray
Copy link
Contributor Author

Is there an easy way to get to the failed task in those check results?

@TristanCacqueray
Copy link
Contributor Author

@geerlingguy @fabianvf the pod creation (needed to test k8s_exec) is failing because of no nodes available to schedule pods. Is this something I can fix or should I remove the integration test from the full_test.yml playbook?

@geerlingguy
Copy link
Collaborator

@TristanCacqueray - Go ahead and remove from full_test.yml, but leave in the test tasks file. I'm going to try to get a cluster setup via molecule where these integration tests can go when they need a fully functional cluster running.

@geerlingguy
Copy link
Collaborator

@TristanCacqueray - After #22 was merged, there's now a KinD cluster environment managed by Molecule—can you add your integration test there (add in tasks, with a reference from playbook.yml to it)?

@TristanCacqueray
Copy link
Contributor Author

@geerlingguy ok, ea0326e adds the integration test to molecule.

@TristanCacqueray
Copy link
Contributor Author

molecule/default/tasks/exec.yml Outdated Show resolved Hide resolved
molecule/default/tasks/exec.yml Outdated Show resolved Hide resolved
molecule/default/tasks/exec.yml Outdated Show resolved Hide resolved
plugins/modules/k8s_exec.py Outdated Show resolved Hide resolved
@geerlingguy
Copy link
Collaborator

LGTM!

@geerlingguy geerlingguy merged commit 7067700 into ansible-collections:master Feb 14, 2020
@TristanCacqueray
Copy link
Contributor Author

Finally, thanks! I haven't follow what is a collection, would you mind sharing the instructions how to use that module now? e.g. a playbook or ansible.cfg...

@geerlingguy
Copy link
Collaborator

@TristanCacqueray - This project's README file has instructions for using the collection in your playbook—in Ansible 2.10, the k8s modules will be moving out of the ansible/ansible project and instead will come from this collection (though there will be a distribution of Ansible that includes them, copied over from this collection).

But basically, you can do the following to use the collection (and your shiny new module):

ansible-galaxy collection install community.kubernetes

Then in your playbook, add the section:

- hosts: [hosts here]

  collections:
    - community.kubernetes

  tasks:
    - k8s_exec:
      ...

Alternatively you can drop the collections section and specify the 'FQCN' (Fully Qualified Collection Namespace) for the task reference, so instead of k8s_exec, it would be community.kubernetes.k8s_exec.

@geerlingguy
Copy link
Collaborator

(Note that the above requires at least Ansible 2.8.)

@TristanCacqueray
Copy link
Contributor Author

@geerlingguy alright, that works, thanks again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add k8s_exec module to collection
4 participants