Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 #66581

janetkuo · 2018-07-24T20:50:10Z

What this PR does / why we need it: Default values should be set in all API versions, because defaulting happens whenever a serialized version is read. When we switched to apps/v1 as the storage version in 1.10 (#58854), extensions/v1beta1 DeploymentSpec.ProgressDeadlineSeconds gets apps/v1 default value (600) instead of being unset.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #66135

Special notes for your reviewer: We need to cherrypick this fix to 1.10 and 1.11. Note that this fix will only help people who haven't upgraded to 1.10 or 1.11 when the storage version is changed.

@kubernetes/sig-apps-bugs

Release note:

extensions/v1beta1.Deployment .spec.progressDeadlineSeconds should be defaulted to "no deadline" instead of 600.

enisoc · 2018-07-24T22:22:17Z

pkg/apis/extensions/validation/validation.go

@@ -289,7 +289,8 @@ func ValidateDeploymentSpec(spec *extensions.DeploymentSpec, fldPath *field.Path
 	}
 	if spec.ProgressDeadlineSeconds != nil {
 		allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.ProgressDeadlineSeconds), fldPath.Child("progressDeadlineSeconds"))...)
-		if *spec.ProgressDeadlineSeconds <= spec.MinReadySeconds {
+		if *spec.ProgressDeadlineSeconds > 0 && *spec.ProgressDeadlineSeconds <= spec.MinReadySeconds {


Is it safe to relax validation to allow 0? The API guidelines say, "Validation rules on spec fields can neither be relaxed nor strengthened."

In this case, I'm not too worried about clients being broken by 0, but what happens on the server if, for example, you roll back to a previous patch release after some objects have been persisted with a 0 value? Will the object become inaccessible? Will a "list all" fail? Or does the server handle this gracefully somehow?

Good point. Even if the object is still accessible with 0 value after downgrade, the old controller will treat deadline == 0 as "immediately pass deadline" instead of "no deadline".

It's safer to use max int instead, then.

Updated to use max int32 instead.

smarterclayton · 2018-07-25T02:05:04Z

Doesn't this mean that anytime we have an effective default on a pointer field, where we want to leave the field unset, we can't change the default across versions? Pretty sure we've introduced more of these as we've become quite pointer happy these days.

…

On Tue, Jul 24, 2018 at 9:12 PM k8s-ci-robot ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *NOT APPROVED* This pull-request has been approved by: *janetkuo <#66581#>* To fully approve this pull request, please assign additional approvers. We suggest the following additional approver: *lavalamp* If they are not already assigned, you can assign the PR to them by writing /assign @lavalamp in a comment when ready. The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands>. The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - *api/OWNERS <https://github.com/kubernetes/kubernetes/blob/master/api/OWNERS>* - *pkg/apis/OWNERS <https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/OWNERS>* - pkg/controller/deployment/OWNERS <https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/OWNERS> [janetkuo] - *staging/src/k8s.io/api/OWNERS <https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/OWNERS>* Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#66581 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_px-Isf0NgpgiMyDuz2lAHyBlD8jxks5uJ8XkgaJpZM4Veh72> .

janetkuo · 2018-07-25T17:49:39Z

Doesn't this mean that anytime we have an effective default on a pointer field, where we want to leave the field unset, we can't change the default across versions?

@smarterclayton Yes. We cannot leave a field unset in one version but defaulted in another version, because defaulting are not just applied when an object is created, but every time a serialized version is read.

Pretty sure we've introduced more of these as we've become quite pointer happy these days.

I opened an umbrella issue #66582 to track other violations.

janetkuo · 2018-07-25T21:34:33Z

/test pull-kubernetes-e2e-gce

enisoc

LGTM other than nits.

enisoc · 2018-07-26T18:04:13Z

pkg/controller/deployment/progress.go

@@ -47,7 +47,7 @@ func (dc *DeploymentController) syncRolloutStatus(allRSs []*apps.ReplicaSet, new
 	isCompleteDeployment := newStatus.Replicas == newStatus.UpdatedReplicas && currentCond != nil && currentCond.Reason == util.NewRSAvailableReason
 	// Check for progress only if there is a progress deadline set and the latest rollout
 	// hasn't completed yet.
-	if d.Spec.ProgressDeadlineSeconds != nil && !isCompleteDeployment {
+	if !util.NoProgressDeadline(d) && !isCompleteDeployment {


Nit: could we invert the function to HasProgressDeadline to avoid the double negative?

Good point. Done.

enisoc · 2018-07-26T18:09:25Z

pkg/apis/extensions/types.go

-	// not be estimated during the time a deployment is paused. This is not set
-	// by default.
+	// not be estimated during the time a deployment is paused. This is set to
+	// the max value of int32 (1<<31 - 1) by default, which means "no deadline".


Nit: Since this may end up in generated user docs, it might be better to give the explicit value instead (2147483647) so it looks the same way people will see it printed in objects, and so people don't need to be familiar with bit shift syntax.

…xInt32. 1. MaxInt32 has the same meaning as unset, for compatibility 2. Deployment controller treats MaxInt32 the same as unset (nil)

1. hack/update-generated-protobuf.sh 2. hack/update-generated-swagger-docs.sh 3. hack/update-swagger-spec.sh 4. hack/update-openapi-spec.sh 5. hack/update-api-reference-docs.sh

janetkuo · 2018-07-27T17:34:53Z

@enisoc PTAL

enisoc

/lgtm

janetkuo · 2018-07-30T20:46:51Z

/assign @smarterclayton

Would you approve it? Thanks!

tnozicka

nit; lgtm otherwise

tnozicka · 2018-07-31T11:13:51Z

pkg/controller/deployment/progress_test.go

@@ -330,7 +338,7 @@ func TestSyncRolloutStatus(t *testing.T) {
 			newCond := util.GetDeploymentCondition(test.d.Status, test.conditionType)
 			switch {
 			case newCond == nil:
-				if test.d.Spec.ProgressDeadlineSeconds != nil {
+				if test.d.Spec.ProgressDeadlineSeconds != nil && *test.d.Spec.ProgressDeadlineSeconds != math.MaxInt32 {


nit: deploymentutil.HasProgressDeadline

Intentionally write it explicitly in the test, so that if we break deploymentutil.HasProgressDeadline() in the future, this test will still catch that.

smarterclayton · 2018-07-31T14:47:52Z

/approve

k8s-ci-robot · 2018-07-31T14:48:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enisoc, janetkuo, smarterclayton

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:

~~api/OWNERS~~ [smarterclayton]
~~docs/api-reference/OWNERS~~ [smarterclayton]
~~pkg/apis/OWNERS~~ [smarterclayton]
~~pkg/controller/deployment/OWNERS~~ [janetkuo,smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [smarterclayton]

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

k8s-github-robot · 2018-07-31T15:05:38Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-07-31T16:30:58Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

nikhita · 2018-09-12T16:12:18Z

This should include a release note.

@janetkuo Can you update the main PR body to include a release note? Thanks!

janetkuo · 2018-10-01T22:32:51Z

Added a release note. Thanks!

…81-upstream-release-1.11 Automated cherry pick of #66581 upstream release 1.11

janetkuo assigned enisoc and kow3ns Jul 24, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 24, 2018

k8s-ci-robot requested review from david-mcmahon and gmarek July 24, 2018 20:50

janetkuo mentioned this pull request Jul 24, 2018

Any field with a default value must have default values in all API versions #66582

Closed

5 tasks

janetkuo force-pushed the deploy-progress branch 2 times, most recently from 2b950f9 to 0fa3593 Compare July 24, 2018 22:11

enisoc reviewed Jul 24, 2018

View reviewed changes

janetkuo mentioned this pull request Jul 25, 2018

1.10.0 ProgressDeadlineSeconds default added to v1beta1 deployment #66135

Closed

janetkuo force-pushed the deploy-progress branch from 0fa3593 to e46e270 Compare July 25, 2018 01:11

janetkuo changed the title ~~Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to 0~~ Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 Jul 25, 2018

janetkuo force-pushed the deploy-progress branch from e46e270 to 1abd732 Compare July 25, 2018 01:15

janetkuo force-pushed the deploy-progress branch from 1abd732 to 729e6de Compare July 25, 2018 17:53

enisoc approved these changes Jul 26, 2018

View reviewed changes

janetkuo added 3 commits July 27, 2018 10:10

Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to Ma…

a4f85c8

…xInt32. 1. MaxInt32 has the same meaning as unset, for compatibility 2. Deployment controller treats MaxInt32 the same as unset (nil)

Update API doc of ProgressDeadlineSeconds

849c08d

Autogen

4dadbb5

1. hack/update-generated-protobuf.sh 2. hack/update-generated-swagger-docs.sh 3. hack/update-swagger-spec.sh 4. hack/update-openapi-spec.sh 5. hack/update-api-reference-docs.sh

janetkuo force-pushed the deploy-progress branch from 729e6de to 4dadbb5 Compare July 27, 2018 17:10

david-mcmahon removed their request for review July 27, 2018 17:37

enisoc approved these changes Jul 27, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2018

janetkuo removed the request for review from gmarek July 27, 2018 18:06

k8s-ci-robot assigned smarterclayton Jul 30, 2018

tnozicka reviewed Jul 31, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2018

k8s-github-robot merged commit 259e074 into kubernetes:master Jul 31, 2018

This was referenced Aug 9, 2018

Automated cherry pick of #66581 upstream release 1.11 #67210

Merged

Automated cherry pick of #66581 upstream release 1.10 #67213

Closed

roycaihw mentioned this pull request Sep 28, 2018

Generate 8.0.0 snapshot client on master branch kubernetes-client/python#631

Merged

2 tasks

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 1, 2018

k8s-ci-robot added a commit that referenced this pull request Jan 18, 2019

Merge pull request #67210 from janetkuo/automated-cherry-pick-of-#665…

640dd3c

…81-upstream-release-1.11 Automated cherry pick of #66581 upstream release 1.11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 #66581

Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 #66581

janetkuo commented Jul 24, 2018 •

edited

enisoc Jul 24, 2018

janetkuo Jul 25, 2018 •

edited

janetkuo Jul 25, 2018

smarterclayton commented Jul 25, 2018 via email

janetkuo commented Jul 25, 2018

janetkuo commented Jul 25, 2018

enisoc left a comment

enisoc Jul 26, 2018

janetkuo Jul 27, 2018

enisoc Jul 26, 2018

janetkuo Jul 27, 2018

janetkuo commented Jul 27, 2018

enisoc left a comment

janetkuo commented Jul 30, 2018

tnozicka left a comment

tnozicka Jul 31, 2018

janetkuo Jul 31, 2018

smarterclayton commented Jul 31, 2018

k8s-ci-robot commented Jul 31, 2018

k8s-github-robot commented Jul 31, 2018

k8s-github-robot commented Jul 31, 2018

nikhita commented Sep 12, 2018

janetkuo commented Oct 1, 2018

Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 #66581

Default extensions/v1beta1 Deployment's ProgressDeadlineSeconds to MaxInt32 #66581

Conversation

janetkuo commented Jul 24, 2018 • edited

Choose a reason for hiding this comment

janetkuo Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 25, 2018 via email

janetkuo commented Jul 25, 2018

janetkuo commented Jul 25, 2018

enisoc left a 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

janetkuo commented Jul 27, 2018

enisoc left a comment

Choose a reason for hiding this comment

janetkuo commented Jul 30, 2018

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 31, 2018

k8s-ci-robot commented Jul 31, 2018

k8s-github-robot commented Jul 31, 2018

k8s-github-robot commented Jul 31, 2018

nikhita commented Sep 12, 2018

janetkuo commented Oct 1, 2018

janetkuo commented Jul 24, 2018 •

edited

janetkuo Jul 25, 2018 •

edited