Fix RollingUpdateDaemonSet godoc regarding rounding

[Miciah]

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]

@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]

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]

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]

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]

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]

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.

[smarterclayton]

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]

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

[smarterclayton]

. 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]

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

/lgtm
/approve

[pacoxu]

/remove-kind api-change
/retest

[pacoxu]

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

[k8s-ci-robot]

[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

[pacoxu]

/ok-to-test

[pacoxu]

/lgtm