-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
CronJob: Respect ControllerRef #42177
CronJob: Respect ControllerRef #42177
Conversation
4973031
to
d471655
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/approve |
/lgtm |
Reviewed 9 of 9 files at r1. pkg/controller/cronjob/cronjob_controller.go, line 58 at r1 (raw file):
Probably should not be exported from package Comments from Reviewable |
d471655
to
1083d91
Compare
9586055
to
3e166bd
Compare
3e166bd
to
7505b1f
Compare
@soltysh Please take a look. I believe all comments are addressed. |
/lgtm |
@ethernetdan @calebamiles is this ok to merge for 1.6 release? |
// List children (Jobs) before parents (CronJob). | ||
// This guarantees that if we see any Job that got orphaned by the GC orphan finalizer, | ||
// we must also see that the parent CronJob has non-nil DeletionTimestamp (see #42639). | ||
// Note that this only works because we are NOT using any caches here. | ||
jl, err := jm.kubeClient.BatchV1().Jobs(metav1.NamespaceAll).List(metav1.ListOptions{}) | ||
if err != nil { | ||
glog.Errorf("Error listing jobs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is not printed here
@@ -117,6 +116,14 @@ func (jm *CronJobController) syncAll() { | |||
js := jl.Items | |||
glog.V(4).Infof("Found %d jobs", len(js)) | |||
|
|||
sjl, err := jm.kubeClient.BatchV2alpha1().CronJobs(metav1.NamespaceAll).List(metav1.ListOptions{}) | |||
if err != nil { | |||
glog.Errorf("Error listing cronjobs: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change glog.Errorf calls to utilruntime.HandleError (the latter is ratelimited among other things)
@@ -237,6 +244,18 @@ func syncOne(sj *batchv2alpha1.CronJob, js []batchv1.Job, now time.Time, jc jobC | |||
} | |||
*sj = *updatedSJ | |||
|
|||
if sj.DeletionTimestamp != nil { | |||
// The CronJob is being deleted. | |||
// Don't do anything other than updating status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no status update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh well it seems it has already happened above
|
||
// Adopt Jobs it owns that don't have ControllerRef yet. | ||
// That is, the Jobs were created by a pre-v1.6.0 master. | ||
It("should adopt Jobs it owns that don't have ControllerRef yet", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be an integration test, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was mainly added because we've run into problems with RBAC before. Do integration tests cover that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect so, @deads2k ?
Now that CronJob adds ControllerRef to Jobs it creates, we need to set this default so legacy behavior is maintained.
This should only happen if the Jobs were created by an older version of the CronJob controller, since from now on we add ControllerRef upon creation. CronJob doesn't do actual adoption because it doesn't use label selectors to find its Jobs. However, we should apply ControllerRef for potential server-side cascading deletion, and to advise other controllers we own these objects.
bb3305d
to
c168dad
Compare
pkg/controller/cronjob/utils.go
Outdated
continue | ||
} | ||
job.OwnerReferences = append(job.OwnerReferences, sjControllerRef) | ||
updatedJob, err := jc.UpdateJob(job.Namespace, job) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have more time, shall we switch to patch
?
test/e2e/generated_clientset.go
Outdated
@@ -324,7 +324,8 @@ var _ = framework.KubeDescribe("Generated release_1_5 clientset", func() { | |||
observeCreation(w) | |||
|
|||
By("deleting the cronJob") | |||
if err := cronJobClient.Delete(cronJob.Name, nil); err != nil { | |||
// Use OrphanDependents=false so the delete is synchronous. | |||
if err := cronJobClient.Delete(cronJob.Name, &metav1.DeleteOptions{OrphanDependents: new(bool)}); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrphanDependents
is being deprecated in 1.7. Should we switch to use deleteOptions.propagationPolicy
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// List children (Jobs) before parents (CronJob). | ||
// This guarantees that if we see any Job that got orphaned by the GC orphan finalizer, | ||
// we must also see that the parent CronJob has non-nil DeletionTimestamp (see #42639). | ||
// Note that this only works because we are NOT using any caches here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soltysh we'll need to take care of this when we change cronjob controller to use informer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we've talked about it during KubeCon. If I have a reasonable solution in place in openshift I'll make sure to fix cronjob controller.
Do we need to test that SJ won't adopt Jobs it doesn't own? |
This is unit tested via The e2e test is only to ensure surprises like RBAC don't prevent adopt calls from working. In this case, the correct action is to do nothing, so we don't need an e2e test to make sure other components don't interfere. There is no "release/orphan" case since CronJob does not use a label selector to find its children. |
c168dad
to
01aded9
Compare
This is needed now that the default is OrphanDependents.
This prevents a race with the GC while it orphans dependents.
Currently, an e2e test is the only way to ensure we have the proper RBAC permissions to adopt Jobs.
01aded9
to
be1fe95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Automatic merge from submit-queue (batch tested with PRs 42177, 42176, 44721) |
Does this mean that whenever I delete a cronjob from a k8s cluster, all its jobs, and associated pods would get deleted too? after deleting their associated cronjob. See my comment on #42627 |
That depends how you remove the object, it is still possible to remove an object and orphan its dependents. |
I delete the object using helm. |
Can you provide me the actual requests |
What this PR does / why we need it:
This is part of the completion of the ControllerRef proposal. It brings CronJob into compliance with ControllerRef. See the individual commit messages for details.
Which issue this PR fixes:
This ensures that other controllers do not fight over control of objects that a CronJob owns.
Special notes for your reviewer:
Release note:
cc @erictune @kubernetes/sig-apps-pr-reviews