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

Add -ness checks and refactor migrations #1674

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

dhageman
Copy link
Contributor

SUMMARY

This PR implements liveness & readiness probes supporting the split web/task container configuration. It also adjusts database migrations to be done in the init container of the task pod.

Addresses #414 & #926

ISSUE TYPE
  • New or Enhanced Feature
ADDITIONAL INFORMATION

It should be acknowledged that this is not the first work on implementing these features.

@dhageman dhageman force-pushed the inesscheck branch 5 times, most recently from e49c674 to 2f1f75b Compare February 23, 2024 02:35
@rooftopcellist
Copy link
Member

I just ran through the code one more time. The k8s job get created as expected.

image

Then runs to completion. The logs for the migration container show the migrations have all been completed, as can be seen by the output of:

$ oc logs awx-migration-23.9.0-6vwzm

@@ -0,0 +1,57 @@
---

- name: Check for pending migrations
Copy link
Member

Choose a reason for hiding this comment

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

👍
This task ensures that the k8s job is only run if there are new schema changes to be migrated.

And the unique name based on the version and the random hash ensure that there are no conflicts with other k8s jobs from previous migrations.

{{ lookup("template", "../common/templates/labels/version.yaml.j2") | indent(width=4) | trim }}
spec:
template:
spec:
Copy link
Member

Choose a reason for hiding this comment

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

@dhageman One thought I had is that we might want to be mindful of cleaning up these migration jobs in case a deployment gets in a bad state and winds up creating them in a loop after the timeout is reached. The job containers are automatically cleaned up after 3600 seconds, which is pretty reasonable.

If we need to, we can adjust this via .spec.ttlSecondsAfterFinished. Though that is probably a good default to start with since it strikes a nice balance between "not cluttering things" and having logs stick around long enough for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You make an excellent point. There is definitely a balance there.

@rooftopcellist rooftopcellist merged commit ffba1b4 into ansible:devel Mar 6, 2024
6 checks passed
@rooftopcellist
Copy link
Member

@dhageman thank you for your time and effort to develop this solution. I think it is robust and will be a good pattern.

@erz4
Copy link
Contributor

erz4 commented Mar 6, 2024

The table in container-probes.md is missing table format, fixed that here #1748
@dhageman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants