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

Implement Native Cronjob Removal #1178

Merged
merged 40 commits into from Sep 19, 2019

Conversation

@cdchris12
Copy link
Contributor

commented Aug 13, 2019

Checklist

Currently, native cronjobs are never removed from the system. This PR implements the logic necessary to detect and remove cronjobs which are no longer defined in .lagoon.yml as well as cronjobs which have been modified so as to no longer be ran natively.

Changelog Entry

Improvement - Implement Native Cronjob Removal

@Schnitzel

This comment has been minimized.

NATIVE_CRONJOB_CLEANUP_ARRAY will include ALL cronjobs, not only the Native ones, see:

NATIVE_CRONJOB_CLEANUP_ARRAY+=("cronjob-${SERVICE_NAME}-${CRONJOB_NAME}")

NATIVE_CRONJOB_CLEANUP_ARRAY is actually a leftover from an earlier cleanup system, which did not get completely removed.
so if you want to use it, we need to move the filling of this array into here:
. /oc-build-deploy/scripts/exec-openshift-resources-with-images.sh

This comment has been minimized.

Copy link
Member

replied Aug 8, 2019

please call CRONJOB something that makes more sense. Like SINGLE_NATIVE_CRONJOB

This comment has been minimized.

Copy link
Contributor Author

replied Aug 8, 2019

This array will list ALL the cronjobs, and that's okay. For us to remove cronjobs which are no longer defined, we simply iterate through the list of cronjobs returned from the oc command. If a cronjob appears in the oc get cronjobs output, but does NOT appear in our list of all cron jobs, we know that it has been removed from the .lagoon.yaml

This comment has been minimized.

Copy link
Member

replied Aug 8, 2019

but what in the case if somebody moves the cronjob from the native cronjob type to the in-pod type? then the native cronjob is not removed as it will still exist in the list of all cronjobs.

@Schnitzel

This comment has been minimized.

why do we need to remove SCHEDULE from here?

This comment has been minimized.

Copy link
Contributor Author

replied Aug 8, 2019

This is designed to strip the header row returned by the oc get cronjobs command so we can pipe only the lines containing actual cronjob definitions on to the cut command, which will further strip those lines down to just the name of said cronjob.

This comment has been minimized.

Copy link
Member

replied Aug 8, 2019

ok, then please use oc --no-headers because if somebody defines a cronjob that contains the words SCHEDULE (which is quite likely) your code will ignore it.

@cdchris12

This comment has been minimized.

Copy link
Contributor Author

commented on images/oc-build-deploy-dind/build-deploy-docker-compose.sh in 5f229cf Aug 8, 2019

By moving this defunct array to only be appended to in the case where this cronjob actually should have a native cronjob pod, we can effectively cover the edge case where a cronjob is amended from a runtime >15 minutes to a runtime <15 minutes.

@cdchris12 cdchris12 self-assigned this Aug 13, 2019
tests/files/features/.lagoon.yml.test Outdated Show resolved Hide resolved
tests/files/features/.lagoon.yml Outdated Show resolved Hide resolved
tests/files/features/.lagoon.yml.test Outdated Show resolved Hide resolved
tests/files/features/.lagoon.yml.test Outdated Show resolved Hide resolved
tests/tests/features/cronjobs.yaml Show resolved Hide resolved
cdchris12 added 21 commits Aug 14, 2019
… returns its timestamp.
@cdchris12 cdchris12 requested a review from Schnitzel Aug 20, 2019
cdchris12 and others added 15 commits Aug 20, 2019
tests/tasks/git-add-commit-push.yaml Outdated Show resolved Hide resolved
@cdchris12 cdchris12 requested a review from Schnitzel Sep 10, 2019
@Schnitzel Schnitzel merged commit f8d2adb into master Sep 19, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@Schnitzel Schnitzel added this to the v1.1.0 milestone Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.