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

Allow {web_,task_,}replicas to be 0 and split out molecule tests #1468

Merged
merged 5 commits into from Jul 18, 2023

Conversation

relrod
Copy link
Member

@relrod relrod commented Jun 26, 2023

SUMMARY

(Replaces #1464 because I think there is a 👻 CI check there that is in a weird state.)

There is quite a bit going on here.

On the user-facing side:

  • First off allow replicas, web_replicas, and task_replicas to be 0. Previously this was interpreted as a falsy value and messed up the templating logic. Now we compare it to the default (empty string) instead. Fixes AWX doesn't scale in to 0 replicas #1454.
  • The above uncovered another issue: If we scale replicas to 0, then the operator reconciliation loop starts failing since it tries to find some pod that doesn't exist in that case. Only run those tasks if we are able.

On the testing side:

  • Add tests for the above.
  • However this made the (single) test file really big, so I broke it into some separately-runnable files. For example, one file to update the AWX instance we are testing with. You just import that with include_tasks.
    • I also made it so that when you do that, you can pass in an additional_fields var so that you can actually customize the AWX definition to test how changes to fields interact.
  • Lowered CPU request specs, because it was failing with testing 3 replicas on GHA.
  • All these new tests made CI really slow (~13 minutes up to ~25 minutes). So to fix this, I made use of ansible tags and broke a test matrix in GHA. It's also easy to expand this (just tag the new tasks and edit/add two lines in ci.yaml) so that if we add chunks of other tests later, they can each run in their own separate instance, in parallel.
    • To do that, I had to tag the tasks that should always run (like the tasks to set up kind and tear it down, and to do the image build). I think this is a fair trade-off for the flexibility we get with doing matrix runs now.
  • Also deleted the branch filter from ci.yaml so that people can run the tests in the own forks on their own feature branches.
ISSUE TYPE
  • Bug, Docs Fix or other nominal change

Signed-off-by: Rick Elrod <rick@elrod.me>
Signed-off-by: Rick Elrod <rick@elrod.me>
…ous fields

Signed-off-by: Rick Elrod <rick@elrod.me>
Signed-off-by: Rick Elrod <rick@elrod.me>
Signed-off-by: Rick Elrod <rick@elrod.me>
@relrod relrod force-pushed the zero-replicas-are-replicas-too branch from 7d033ca to e46a1de Compare July 13, 2023 23:19
@relrod relrod changed the title Allow {web_,task_,}replicas to be 0 Allow {web_,task_,}replicas to be 0 and split out molecule tests Jul 13, 2023
@@ -285,5 +285,7 @@

- name: Verify the resource pod name is populated.
assert:
that: awx_task_pod_name != ''
that:
- awx_task_pod_name != ''
fail_msg: "Could not find the tower pod's name."
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to think about moving away from the tower naming scheme in errors since we changed the pod_names? I will leave this as a single comment since it isn't directly applied to the PR, more just for others to see and think about potentially changing it if there are others.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but it doesn't have to prevent this from merging.

Copy link
Member

@thedoubl3j thedoubl3j left a comment

Choose a reason for hiding this comment

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

big plus 1 for the test changes around tags to help with the slowness and fixes for replicas

@rooftopcellist
Copy link
Member

I like the way you split out the replicas check to _test_case_replicas.yml. Nicely done all around.

@rooftopcellist rooftopcellist enabled auto-merge (squash) July 18, 2023 20:32
@rooftopcellist rooftopcellist merged commit c9ab993 into ansible:devel Jul 18, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWX doesn't scale in to 0 replicas
3 participants