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

k8s_scale fixes #100

Merged
merged 3 commits into from
May 29, 2020

Conversation

willthames
Copy link
Collaborator

SUMMARY

Fix scale wait and add tests

Move wait logic out of raw and into common and use that
logic in scale

Fix a few broken wait condition cases highlighted by scaling
up and down

Move scale-related tests into dedicated test task file

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

k8s_scale

ADDITIONAL INFORMATION

@willthames
Copy link
Collaborator Author

Based on #35 - merge that one first

@codecov
Copy link

codecov bot commented May 16, 2020

Codecov Report

Merging #100 into master will decrease coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #100      +/-   ##
==========================================
- Coverage   42.98%   42.88%   -0.11%     
==========================================
  Files           3        3              
  Lines         542      541       -1     
  Branches      110      110              
==========================================
- Hits          233      232       -1     
  Misses        266      266              
  Partials       43       43              
Impacted Files Coverage Δ
...ommunity/kubernetes/plugins/module_utils/common.py 37.93% <0.00%> (-12.76%) ⬇️
...s/community/kubernetes/plugins/module_utils/raw.py 44.51% <0.00%> (+5.65%) ⬆️

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 3bdfb47...aa70416. Read the comment docs.

@willthames willthames force-pushed the scale_improvements branch 7 times, most recently from f3f534f to 441397b Compare May 21, 2020 02:40
@geerlingguy
Copy link
Collaborator

Latest test is giving:

Failed to apply object: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure",
"message":"Service "apply-svc" is invalid: spec.ports[1].nodePort: Forbidden: may not be used when `type` is 'ClusterIP'"

...which is weird, because it seems like type is set to NodePort...?

Move wait logic out of raw and into common and use that
logic in scale

Fix a few broken wait condition cases highlighted by scaling
up and down

Move scale-related tests into dedicated test task file

Additional service related tests
@willthames willthames force-pushed the scale_improvements branch from 441397b to beebe98 Compare May 22, 2020 02:39
@willthames
Copy link
Collaborator Author

@geerlingguy the bit where it was failing was in adding a port to an existing service, and also setting NodePort at the same time (I suspect I mis-merged two separate changes at some point) - just need to be consistently all NodePorts or all ClusterIPs. I've changed it to the latter.

@geerlingguy
Copy link
Collaborator

Now this is failing with the same thing mentioned in #104

    TASK [Check that deployment wait worked] ***************************************
    task path: /home/runner/work/community.kubernetes/community.kubernetes/ansible_collections/community/kubernetes/molecule/default/tasks/waiter.yml:247
fatal: [localhost]: FAILED! => {
    "assertion": "updated_deploy_pods.resources[0].spec.containers[0].image.endswith(\":2\")",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}
    Read vars_file 'vars/main.yml'

@willthames
Copy link
Collaborator Author

Yeah, there's definitely a race condition, not 100% sure on the best course of action - Kubernetes is doing the right thing, the tests are doing the right thing, just perhaps too soon.

I might just have to improve the filters to ignore the unready pod where its container has terminated but the pod is still not in terminating state (that bit is weird)

Move wait logic out of raw and into common and use that
logic in scale

Fix a few broken wait condition cases highlighted by scaling
up and down

Move scale-related tests into dedicated test task file

Additional service related tests
Copy link
Collaborator

@geerlingguy geerlingguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on initial review—I like that we're removing net 93 LoC in the python modules and fixing bugs at the same time!

@geerlingguy geerlingguy requested a review from fabianvf May 29, 2020 14:29
Copy link
Collaborator

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks great, removes a lot of cruft and moves some common logic from raw -> common.

@geerlingguy
Copy link
Collaborator

Merging—it also seems to fix the instability with our CI tests, so I can stop getting nightly fail notifications from the scheduled CI run :)

@geerlingguy geerlingguy merged commit 1ed3b65 into ansible-collections:master May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants