Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Miciah · 2021-04-21T01:22:24Z

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Fix the godoc for RollingUpdateDaemonSet to state that
spec.updateStrategy.rollingUpdate.maxUnavailable is rounded up.

A recent commit changed the godoc to say that the value of this field was rounded down, but the actual implementation rounds up and always has rounded up. (This is in contrast to Deployments, where spec.strategy.rollingUpdate.maxUnavailable is rounded down.)

Follow-up to 5aa53f8.

api/openapi-spec/swagger.json:
staging/src/k8s.io/api/apps/v1/generated.proto:
pkg/apis/apps/types.go:
staging/src/k8s.io/api/apps/v1/types.go:
staging/src/k8s.io/api/apps/v1/types_swagger_doc_generated.go:
staging/src/k8s.io/api/apps/v1beta2/generated.proto:
staging/src/k8s.io/api/apps/v1beta2/types.go:
staging/src/k8s.io/api/apps/v1beta2/types_swagger_doc_generated.go:
staging/src/k8s.io/api/extensions/v1beta1/generated.proto:
staging/src/k8s.io/api/extensions/v1beta1/types.go:
staging/src/k8s.io/api/extensions/v1beta1/types_swagger_doc_generated.go:
staging/src/k8s.io/cli-runtime/artifacts/openapi/swagger.json:
staging/src/k8s.io/kubectl/testdata/openapi/swagger.json: Change "rounding down" to "rounding up".

Which issue(s) this PR fixes:

N/A.

Special notes for your reviewer:

N/A.

Does this PR introduce a user-facing change?

The API documentation for the DaemonSet's spec.updateStrategy.rollingUpdate.maxUnavailable field was corrected to state that the value is rounded up.

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

The godoc was changed during the implementation of the following kep, but this kep does not mention any changes with respect to rounding:

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/1591-daemonset-surge

k8s-ci-robot · 2021-04-21T01:22:31Z

@Miciah: 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.

k8s-ci-robot · 2021-04-21T01:22:32Z

Hi @Miciah. 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.

smarterclayton · 2021-04-21T01:39:03Z

Is there a reason it rounds up? we already have fence post progress guarantees for maxUnavailable in deployment that i would expect daemonset to have (we never allow less than one unavailable). why round up?

fejta-bot · 2021-04-21T02:02:21Z

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.

Miciah · 2021-04-21T02:49:03Z

Is there a reason it rounds up? we already have fence post progress guarantees for maxUnavailable in deployment that i would expect daemonset to have (we never allow less than one unavailable). why round up?

I don't know the original reasoning, but it seems like a breaking change to modify the rounding logic. As far as I can tell, 5aa53f8 and 18f43e4 changed the godoc without changing the implementation, and I figured it was better to revert the godoc change rather than change long-standing behavior. Such a change could cause mild weirdness if people are trying to duplicate the max unavailable calculation for status reporting, for example, and it could cause some surprise when a rolling update of 6 pods with max unavailable set to 33% now takes twice as long. Maybe I am overly paranoid though regarding what constitutes a "breaking change".

wking · 2021-04-21T03:19:45Z

pkg/apis/apps/types.go

@@ -532,7 +532,7 @@ type RollingUpdateDaemonSet struct {
 	// The maximum number of DaemonSet pods that can be unavailable during the
 	// update. Value can be an absolute number (ex: 5) or a percentage of total
 	// number of DaemonSet pods at the start of the update (ex: 10%). Absolute
-	// number is calculated from percentage by rounding down to a minimum of one.
+	// number is calculated from percentage by rounding up with a minimum of one.


I think "rounded up." is sufficient. The "minimum of one" is part of rounding up for every case except maxUnavailable: 0, and the zero case is discussed explicitly in the line below this.

Ironically this line is in here because someone asked. I would leave it for now, since I'm probably going to go start the process to change the behavior.

wking · 2021-04-21T03:24:08Z

Specific lines where the godocs changed in 5aa53f8 (#96375). The backing implementation has been round-up since it landed in b27308c (#41116).

smarterclayton · 2021-04-21T13:52:24Z

. Such a change could cause mild weirdness if people are trying to duplicate the max unavailable calculation for status reporting, for example, and it could cause some surprise when a rolling update of 6 pods with max unavailable set to 33% now takes twice as long. Maybe I am overly paranoid though regarding what constitutes a "breaking change".

The backing implementation has been round-up since it landed in b27308c (#41116).

So in general from an overall Kube perspective changing the rounding behavior for daemonsets with max unavailable cannot make the application more unavailable (it would make it less unavailable). A user who expected a certain percentage would potentially have slower rollouts of daemonsets (at the extreme, it would go from 2 to 1 pods per unit). Conversely, an admin who is familiar with deployments (almost all app authors) might be surprised with daemonsets and have lower availability. The argument would have to be that the consistency between statefulset (future), deployment, and daemonset was more important than the impact to those users who expected 2 and would now get 1 (potentially 2x their upgrade). Typically consistency in API is hugely important in the review process (we historically leaned towards not making apis "better" if they could be consistent with apis we cannot change).

smarterclayton · 2021-04-21T13:56:06Z

However, for the purposes of the issue at hand, the documentation should be correct for the implemented behavior.

/lgtm
/approve

pacoxu · 2021-04-23T08:29:15Z

/remove-kind api-change
/retest

pacoxu · 2021-04-23T08:36:37Z

Please run hack/update-openapi-spec.sh to make the ci green.
/lgtm cancel

k8s-ci-robot · 2021-05-05T00:02:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, 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]
~~pkg/apis/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/kubectl/OWNERS~~ [smarterclayton]

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

Fix the godoc for RollingUpdateDaemonSet to state that spec.updateStrategy.rollingUpdate.maxUnavailable is rounded up. A recent commit changed the godoc to say that the value of this field was rounded down, but the actual implementation rounds up and always has rounded up. (This is in contrast to Deployments, where spec.strategy.rollingUpdate.maxUnavailable is rounded down.) Follow-up to commit 5aa53f8. * api/openapi-spec/swagger.json: * staging/src/k8s.io/api/apps/v1/generated.proto: * pkg/apis/apps/types.go: * staging/src/k8s.io/api/apps/v1/types.go: * staging/src/k8s.io/api/apps/v1/types_swagger_doc_generated.go: * staging/src/k8s.io/api/apps/v1beta2/generated.proto: * staging/src/k8s.io/api/apps/v1beta2/types.go: * staging/src/k8s.io/api/apps/v1beta2/types_swagger_doc_generated.go: * staging/src/k8s.io/api/extensions/v1beta1/generated.proto: * staging/src/k8s.io/api/extensions/v1beta1/types.go: * staging/src/k8s.io/api/extensions/v1beta1/types_swagger_doc_generated.go: * staging/src/k8s.io/cli-runtime/artifacts/openapi/swagger.json: * staging/src/k8s.io/kubectl/testdata/openapi/swagger.json: Change "rounding down" to "rounding up".

pacoxu · 2021-06-11T06:09:26Z

/ok-to-test

pacoxu · 2021-07-05T08:05:32Z

/lgtm

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 21, 2021

k8s-ci-robot requested review from caesarxuchao and errordeveloper April 21, 2021 01:22

wking reviewed Apr 21, 2021

View reviewed changes

k8s-ci-robot assigned smarterclayton Apr 21, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 21, 2021

k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 23, 2021

k8s-ci-robot assigned pacoxu Apr 23, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2021

Miciah force-pushed the fix-RollingUpdateDaemonSet-godoc-regarding-rounding branch from 18fc519 to 108d1d4 Compare May 5, 2021 00:01

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

Miciah force-pushed the fix-RollingUpdateDaemonSet-godoc-regarding-rounding branch from 108d1d4 to 452e2ea Compare June 1, 2021 14:11

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

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2021

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

k8s-ci-robot merged commit 687f0aa into kubernetes:master Jul 5, 2021

k8s-ci-robot added this to the v1.22 milestone Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Miciah commented Apr 21, 2021 •

edited

Loading

k8s-ci-robot commented Apr 21, 2021

k8s-ci-robot commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

fejta-bot commented Apr 21, 2021

Miciah commented Apr 21, 2021

wking Apr 21, 2021

smarterclayton Apr 21, 2021

wking commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

pacoxu commented Apr 23, 2021

pacoxu commented Apr 23, 2021

k8s-ci-robot commented May 5, 2021

pacoxu commented Jun 11, 2021

pacoxu commented Jul 5, 2021

Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Fix RollingUpdateDaemonSet godoc regarding rounding #101296

Conversation

Miciah commented Apr 21, 2021 • edited Loading

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 Apr 21, 2021

k8s-ci-robot commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

fejta-bot commented Apr 21, 2021

Miciah commented Apr 21, 2021

wking Apr 21, 2021

Choose a reason for hiding this comment

smarterclayton Apr 21, 2021

Choose a reason for hiding this comment

wking commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

smarterclayton commented Apr 21, 2021

pacoxu commented Apr 23, 2021

pacoxu commented Apr 23, 2021

k8s-ci-robot commented May 5, 2021

pacoxu commented Jun 11, 2021

pacoxu commented Jul 5, 2021

Miciah commented Apr 21, 2021 •

edited

Loading