Skip to content

Handle CronJob upgrade via active Job Pod deletion#1098

Open
trvrnrth wants to merge 2 commits into
stakater:masterfrom
trvrnrth:cronjob-restart-via-pod-delete
Open

Handle CronJob upgrade via active Job Pod deletion#1098
trvrnrth wants to merge 2 commits into
stakater:masterfrom
trvrnrth:cronjob-restart-via-pod-delete

Conversation

@trvrnrth

Copy link
Copy Markdown

Instead of immediately creating new Jobs we will instead delete pods belonging to active Jobs. This will cause the job controller to re-create the pods based on the defined policies.

As such we will not violate the CronJob schedule, suspension and concurrency policies.

Resolves #822

Instead of immediately creating new Jobs we will instead delete pods
belonging to active Jobs. This will cause the job controller to re-create
the pods based on the defined policies.

As such we will not violate the CronJob schedule, suspension and concurrency
policies.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Resolves the dangerous behavior in which Reloader would create new Job instances from a (potentially suspended) CronJob when an associated ConfigMap/Secret changed. Instead, the CronJob UpdateFunc now deletes the running pods of currently active Jobs, allowing the Job controller to recreate them per the configured policies and respecting CronJob suspension/concurrency.

Changes:

  • Replace CreateJobFromCronjob with RestartRunningCronjobPods, which deletes pods of cronJob.Status.Active jobs via Pods.DeleteCollection filtered by JobNameLabel and status.phase=Running.
  • Add pods: deletecollection RBAC to both the static manifest and the Helm chart (gated by ignoreCronJobs && ignoreJobs).
  • Update tests with a fake-client reactor for delete-collection on pods, plus various doc-comment typo fixes.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/pkg/callbacks/rolling_upgrade.go Replace job-creation logic with pod deletion of active CronJob jobs; fix doc comment.
internal/pkg/callbacks/rolling_upgrade_test.go Rewrite test to verify only running pods of active jobs are deleted; add fake reactor and helpers.
internal/pkg/handler/upgrade.go Wire CronJob UpdateFunc to new RestartRunningCronjobPods; fix doc comments.
internal/pkg/testutil/kube.go Doc-comment typo fixes.
pkg/kube/client.go Doc-comment typo fix.
deployments/kubernetes/manifests/clusterrole.yaml Add pods: deletecollection permission.
deployments/kubernetes/chart/reloader/templates/clusterrole.yaml Conditionally add pods: deletecollection permission.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread deployments/kubernetes/chart/reloader/templates/clusterrole.yaml Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.

[BUG] Reloader starting jobs based on paused CronJobs

2 participants