job controller: set job conditions to false explicitly #100701

adtac · 2021-03-31T11:03:19Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

From the guide to writing controllers in the [API conventions doc]:

Controllers should apply their conditions to a resource the first time they visit the resource, even if the status is Unknown. This allows other components in the system to know that the condition exists ...

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is a user-facing change. If external tools were incorrectly relying on the presence/absence of a particular condition instead of its status, such tools will need to correct their behaviour. Whether or not this is acceptable, I'll leave it to the API reviewers.

However, I'll note that other controllers also make this mistake of removing conditions instead of setting the status to false. Non-exhaustive list of examples:

kubernetes/pkg/controller/deployment/progress.go

Line 42 in a651804

util.RemoveDeploymentCondition(&newStatus, apps.DeploymentProgressing)

kubernetes/pkg/controller/replicaset/replica_set_utils.go

Line 120 in a651804

RemoveCondition(&newStatus, apps.ReplicaSetReplicaFailure)

Does this PR introduce a user-facing change?

The Job controller now sets Job conditions with a false status value when it previously omitted them

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

cc @alculquicondor @lavalamp @soltysh
/sig apps

k8s-ci-robot · 2021-03-31T11:03:27Z

@adtac: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>

k8s-ci-robot · 2021-03-31T11:56:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adtac
To complete the pull request process, please assign smarterclayton after the PR has been reviewed.
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/controller/job/OWNERS
test/OWNERS

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

alculquicondor · 2021-03-31T14:15:42Z

pkg/controller/job/job_controller.go

+			job.Status.Conditions, isUpdated = ensureJobConditionStatus(job.Status.Conditions, batch.JobComplete, v1.ConditionFalse, "", "")
+			jobConditionsChanged = jobConditionsChanged || isUpdated
+		}
+		if utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) && manageJobCalled {


this is a subtle change from the previous implementation:

Before, it looked like we would only suspend a job if didn't already completed. However, now we can theoretically mark a job as suspended even if it's also marked as completed.

For a moment, I thought this wouldn't be possible, but it is, in the coincidence where all pods succeed and a Job is marked as suspended at the same time.

Maybe we can just do:

if !complete && job.Spec.Suspend != nil && *job.Spec.Suspend

also, we should set the condition even if manageJobCalled is false. And even remove the condition if the feature gate is disabled.

alculquicondor · 2021-03-31T14:16:09Z

pkg/controller/job/job_controller.go

@@ -986,13 +996,21 @@ func ensureJobConditionStatus(list []batch.JobCondition, cType batch.JobConditio
 				list[i].Reason = reason


don't forget to update the Probe timestamp

So I did begin to change LastProbeTime, but there's a problem with that. By definition, LastProbeTime must be updated every time ensureJobConditionStatus is called regardless of whether the status/reason/message matches (because it's just a probe). That means that jobConditionsChanged is always true every time syncJob is called. That means that we'll do a Jobs.UpdatedStatus() every time syncJob is called and that forget is always true. These are significantly more far-reaching consequences than I had originally anticipated.

The performance impact of syncing the Job with the API server every time syncJob is called could be disastrous for long running jobs that see very infrequent updates.

So I'd like to hold off from making LastProbeTime change in this PR. It also sends the wrong message to update it only when job condition status value changes (i.e. the if part of this if-else). In fact, AFAICT there are also no other k8s controllers that do anything like LastProbeTime, so I think removing that field might even be the most consistent approach. There is some value in knowing LastProbeTime from the user's perspective, but idk if it's possible to accurately update that without performance concerns.

I was thinking that if the block in line 643 is reached, you can update all LastProbeTimes

Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>

alculquicondor · 2021-04-01T14:22:08Z

pkg/controller/job/job_controller.go

@@ -986,13 +996,21 @@ func ensureJobConditionStatus(list []batch.JobCondition, cType batch.JobConditio
 				list[i].Reason = reason


I was thinking that if the block in line 643 is reached, you can update all LastProbeTimes

alculquicondor · 2021-04-01T14:24:56Z

pkg/controller/job/job_controller.go

-					job.Status.StartTime = &now
-				}
+		} else {
+			job.Status.Conditions, isUpdated = ensureJobConditionStatus(job.Status.Conditions, batch.JobComplete, v1.ConditionFalse, "", "")


why is this necessary?
Wouldn't it be overridden below?

this is JobComplete, not JobSuspended

alculquicondor · 2021-04-01T14:40:10Z

pkg/controller/job/job_controller.go

+			jobConditionsChanged = jobConditionsChanged || isUpdated
+		}
+
+		if !utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) {


Consider putting all this logic in a function.

For example, you could just return after removing the condition if the gate is disabled, simplifying the conditionals below.

alculquicondor · 2021-04-01T14:40:20Z

pkg/controller/job/job_controller.go

+			jobConditionsChanged = jobConditionsChanged || isUpdated
+		} else if !hasCondition(job.Status.Conditions, batch.JobSuspended) {
+			// The feature gate is enabled, but the condition doesn't exist. This
+			// should never happen, insert the default placeholder with status false.


wouldn't this happen in the first sync?

s/This should never happen/This should not be the case/ :)

Yes, this will happen in the first sync.

alculquicondor · 2021-04-01T14:48:04Z

pkg/controller/job/job_controller_test.go

@@ -654,9 +643,14 @@ func TestControllerSyncJob(t *testing.T) {
 			} else if tc.fakeExpectationAtCreation > 0 {
 				manager.expectations.ExpectCreations(key, int(tc.fakeExpectationAtCreation))
 			}
+			originalConditions := []batch.JobCondition{
+				newCondition(batch.JobComplete, v1.ConditionFalse, "", ""),
+				newCondition(batch.JobFailed, v1.ConditionFalse, "", ""),


also newCondition(batch.JobSuspended, v1.ConditionFalse, "", ""), if feature is enabled.

But, I would rather prefer we have wantConditions []JobCondition in the test case.

alculquicondor · 2021-04-01T14:49:41Z

pkg/controller/job/job_controller_test.go

-				t.Errorf("Unexpected conditions %v", actual.Status.Conditions)
+			for _, jc := range originalConditions {
+				if !getCondition(actual, jc.Type, jc.Status, jc.Reason) {
+					copyIrrelevantJobConditionFields(actual.Status.Conditions, originalConditions)


I would sort the conditions by name and then do a direct comparison of actual.Status.Condition and original. Then call cmp.Diff with cmpopts.IgnoreFields

k8s-ci-robot · 2021-05-08T15:26:51Z

@adtac: PR needs rebase.

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.

k8s-triage-robot · 2021-08-06T16:17:21Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

adtac · 2021-08-06T19:16:31Z

is someone else able to take this over? this is an important change but I don't have the time to see it through

k8s-triage-robot · 2021-09-05T19:18:56Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2021-10-05T19:41:49Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2021-10-05T19:42:06Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 31, 2021

k8s-ci-robot requested review from kow3ns and tnozicka March 31, 2021 11:04

job controller: set job conditions to false explicitly

64d694d

Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>

adtac force-pushed the jcstatus branch from ea86a7c to 64d694d Compare March 31, 2021 11:55

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 31, 2021

alculquicondor reviewed Mar 31, 2021

View reviewed changes

review 1

6a7b0e9

Signed-off-by: Adhityaa Chandrasekar <adtac@google.com>

adtac force-pushed the jcstatus branch from a2dc135 to 6a7b0e9 Compare April 1, 2021 07:40

alculquicondor reviewed Apr 1, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 6, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 5, 2021

k8s-ci-robot closed this Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

job controller: set job conditions to false explicitly #100701

job controller: set job conditions to false explicitly #100701

adtac commented Mar 31, 2021

k8s-ci-robot commented Mar 31, 2021

k8s-ci-robot commented Mar 31, 2021

alculquicondor Mar 31, 2021

adtac Apr 1, 2021

alculquicondor Mar 31, 2021

adtac Apr 1, 2021 •

edited

alculquicondor Apr 1, 2021

alculquicondor Apr 1, 2021

alculquicondor Apr 1, 2021

adtac Apr 1, 2021

alculquicondor Apr 1, 2021

alculquicondor Apr 1, 2021

adtac Apr 1, 2021

alculquicondor Apr 1, 2021

alculquicondor Apr 1, 2021

k8s-ci-robot commented May 8, 2021

k8s-triage-robot commented Aug 6, 2021

adtac commented Aug 6, 2021

k8s-triage-robot commented Sep 5, 2021

k8s-triage-robot commented Oct 5, 2021

k8s-ci-robot commented Oct 5, 2021

		@@ -986,13 +996,21 @@ func ensureJobConditionStatus(list []batch.JobCondition, cType batch.JobConditio
		list[i].Reason = reason

job controller: set job conditions to false explicitly #100701

job controller: set job conditions to false explicitly #100701

Conversation

adtac commented Mar 31, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Mar 31, 2021

k8s-ci-robot commented Mar 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adtac Apr 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 8, 2021

k8s-triage-robot commented Aug 6, 2021

adtac commented Aug 6, 2021

k8s-triage-robot commented Sep 5, 2021

k8s-triage-robot commented Oct 5, 2021

k8s-ci-robot commented Oct 5, 2021

adtac Apr 1, 2021 •

edited