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
Graduate JobTrackingWithFinalizers to stable #113510
Graduate JobTrackingWithFinalizers to stable #113510
Conversation
0fcd992
to
72269e3
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
72269e3
to
a2743f2
Compare
/assign @soltysh |
a2743f2
to
379dc13
Compare
@@ -689,7 +689,9 @@ func testResourceUpdate(c *testContext) { | |||
if err != nil { | |||
return err | |||
} | |||
obj.SetAnnotations(map[string]string{"update": "true"}) | |||
annotations := obj.GetAnnotations() |
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.
If somebody had a buggy mutating webhook that did something like this, it will start failing on upgrade to 1.26.
I would consider this WAI: we don't want users to disable tracking with finalizers.
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.
Changed the behavior to always add the annotation back
/retest |
932e894
to
b17dae2
Compare
/label api-review |
/triage accepted |
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
@@ -546,8 +546,8 @@ func ValidateJobTemplateSpec(spec *batch.JobTemplateSpec, fldPath *field.Path, o | |||
|
|||
type JobValidationOptions struct { | |||
apivalidation.PodValidationOptions | |||
// Allow Job to have the annotation batch.kubernetes.io/job-tracking | |||
AllowTrackingAnnotation bool | |||
// Allow Job to not have the annotation batch.kubernetes.io/job-tracking |
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.
// Allow Job to not have the annotation batch.kubernetes.io/job-tracking | |
// Allow Job to not to have the annotation batch.kubernetes.io/job-tracking |
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.
ok, I didn't like either option, so I changed to logic to be RequireTrackingAnnotation=true
by default, and false if the old job didn't have it.
b17dae2
to
3cbde55
Compare
if the goal is to make sure new jobs have the tracking annotation and updates to jobs don't remove it, is there a reason not to add/preserve the annotation in PrepareForCreate/PrepareForUpdate, rather than rejecting previously working API requests? |
19bb0d1
to
cdeb40c
Compare
I just didn't think of that, thanks! That's much simpler. |
Change-Id: Ifc749a85b1270c0155ac511b91d4681d53236820
cdeb40c
to
4948918
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.
Reverted the validation to disallow adding the tracking annotation
@@ -418,17 +423,6 @@ func testJobStrategy(t *testing.T) { | |||
t.Errorf("Expected a validation error") | |||
} | |||
|
|||
// Ensure going from legacy tracking Job to tracking with finalizers is |
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 is now tested in TestJobStrategyValidateUpdate
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.
only comment was on PrepareForUpdate
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, liggitt, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
/kind api-change
/sig apps
What this PR does / why we need it:
Graduate JobTrackingWithFinalizers to stable:
Which issue(s) this PR fixes:
Refs kubernetes/enhancements#2307
Special notes for your reviewer:
The other graduation criteria are completed in #113478 and #113176.
Legacy tracking is still supported. However, now it can only be tested in unit tests and not integration tests, because apiserver disallows creating a job without job tracking annotation.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: