Fix cronjob status reconciliation when job template labels change

[d-honeybadger]

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

In CronJobController V2, the controller only considers jobs with labels that match current cronjob's job template labels. This leads to the controller losing track of jobs owned by a given cronjob when those template labels change. For example, if job template labels were updated while the cronjob was in the active status, that cronjob will be stuck as active, since the controller will never find the completed job whose labels now don't match those of the job template.

Which issue(s) this PR fixes:

Fixes #106496

Special notes for your reviewer:

I see that in #93370 filtering by label was added for performance. If anyone has ideas on how to keep that performance improvement while maintaining correct behavior, I'd be happy to close this and work on another solution.

Does this PR introduce a user-facing change?

Fixes regression in 1.21 default configurations where a bug in CronJob Controller V2 where it would lose track of jobs upon job template labels change.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Welcome @d-honeybadger!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @d-honeybadger. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[d-honeybadger]

/assign @soltysh

[d-honeybadger]

/sig apps

[alculquicondor]

/ok-to-test

This definitely requires a release note.

[alculquicondor]

For comparison, in the job controller we list all the pods for a similar reason:

kubernetes/pkg/controller/job/job_controller.go

Lines 558 to 560 in 3866cb9

	// List all pods to include those that don't match the selector anymore
	// but have a ControllerRef pointing to this controller.
	pods, err := jm.podStore.Pods(j.Namespace).List(labels.Everything())

So I don't see why we shouldn't do the same for the cronjob controller.

I'll have to defer to @soltysh as I don't have the historical background.

[d-honeybadger]

This definitely requires a release note.

Ah yes, added.
Thank you for taking a look!

[alculquicondor]

You should probably add a test too :)
But maybe wait for Maciej to confirm if the change is reasonable.

[alculquicondor]

@janetkuo @kow3ns do you have any thoughts on #107997 (comment) ?

[janetkuo]

For comparison, in the job controller we list all the pods for a similar reason:

kubernetes/pkg/controller/job/job_controller.go

Lines 558 to 560 in 3866cb9

	// List all pods to include those that don't match the selector anymore
	// but have a ControllerRef pointing to this controller.
	pods, err := jm.podStore.Pods(j.Namespace).List(labels.Everything())

So I don't see why we shouldn't do the same for the cronjob controller.

I'll have to defer to @soltysh as I don't have the historical background.

The reason that we didn't do this for the original CronJob controller is that it doesn't use watches, see the ControllerRef design doc for more details. Given that CronJob v2 now uses watches, it makes sense to make the changes accordingly.

[alculquicondor]

Makes sense.

@d-honeybadger could you add a unit test for the change?

[soltysh]

Good catch, I'll dig through perf data and see how good/bad this is, alternatively we could add a separate indexer for jobs, if we needed, but that can be done separately from this PR.
But I definitely would like to see unit test added like @alculquicondor mentioned
/approve
/triage accepted
/priority important-longterm
/assign @alculquicondor
he has the final lgtm on it

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: d-honeybadger, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/controller/cronjob/OWNERS [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[d-honeybadger]

@alculquicondor 👍 Added a test and a comment for good measure.

[alculquicondor]

Add one that has a different owner reference.

[alculquicondor]

/lgtm

Thanks!

[simonjpartridge]

Thanks for fixing this!

I'm curious if this bug will get backported to any older versions of k8s? Or just released in the 1.24 release?

[alculquicondor]

This qualifies for backporting as it's a regression.
Feel free to create a cherry-pick https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md

[simonjpartridge]

@alculquicondor not done this before so let me know if I got something wrong. But I've created 3 cherrypick PRs, one each for 1.21, 1.22, 1.23. I hope that's helpful :)