Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update deployment and daemonset completeness checks #44672

Merged
merged 3 commits into from
Apr 24, 2017
Merged

Update deployment and daemonset completeness checks #44672

merged 3 commits into from
Apr 24, 2017

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Apr 19, 2017 •
edited
Loading

maxUnavailable being taken into account for deployment completeness has caused a lot of confusion (#44395, #44657, #40496, others as well I am sure) so I am willing to just stop using it and require all of the new Pods for a Deployment to be available for the Deployment to be considered complete (hence both rollout status and ProgressDeadlineSeconds will not be successful in cases where a 1-pod Deployment never becomes successful because its Pod never transitions to ready).

@kubernetes/sig-apps-api-reviews thoughts?

Deployments and DaemonSets are now considered complete once all of the new pods are up and running - affects `kubectl rollout status` (and ProgressDeadlineSeconds for Deployments)

Fixes #44395

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 19, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2017
janetkuo
janetkuo reviewed Apr 19, 2017
View reviewed changes
pkg/controller/deployment/util/deployment_util.go Outdated
func DeploymentComplete(deployment *extensions.Deployment, newStatus *extensions.DeploymentStatus) bool {
return newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) &&
newStatus.Replicas == *(deployment.Spec.Replicas) &&
newStatus.AvailableReplicas >= *(deployment.Spec.Replicas)-MaxUnavailable(*deployment) &&
newStatus.AvailableReplicas >= *(deployment.Spec.Replicas) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

==

@janetkuo
Copy link
Member

+1, having "min available pods" doesn't mean the rollout is complete

@janetkuo
Copy link
Member

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 19, 2017
@0xmichalis 0xmichalis added this to the v1.7 milestone Apr 19, 2017
janetkuo
janetkuo reviewed Apr 19, 2017
View reviewed changes
Copy link
Member

@janetkuo janetkuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update PR title since this changes DaemonSet also.

pkg/controller/deployment/util/deployment_util.go Outdated
@@ -410,25 +410,14 @@ func SetReplicasAnnotations(rs *extensions.ReplicaSet, desiredReplicas, maxRepli

// MaxUnavailable returns the maximum unavailable pods a rolling deployment can take.
func MaxUnavailable(deployment extensions.Deployment) int32 {
if !IsRollingUpdate(&deployment) || *(deployment.Spec.Replicas) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this, right?

pkg/controller/deployment/util/deployment_util_test.go Outdated
@@ -960,19 +960,19 @@ func TestDeploymentComplete(t *testing.T) {
expected bool
}{
{
name: "complete",
name: "not complete #0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest making it more descriptive, something like "not complete: min but not all pods become available"

pkg/controller/deployment/util/deployment_util_test.go
@@ -960,19 +960,19 @@ func TestDeploymentComplete(t *testing.T) {
expected bool
}{
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hence both rollout status and ProgressDeadlineSeconds will not be successful in cases where a 1-pod Deployment never becomes successful because its Pod is never run

Add a test case for this

@lukaszo
Copy link
Contributor

lukaszo commented Apr 19, 2017

Why labeling with relase-note-none? It changes the kubectl behavior.

@0xmichalis
Copy link
Contributor Author

Yeah, we need a release note for the change

/release-note

@k8s-ci-robot 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 Apr 19, 2017
@janetkuo
Copy link
Member

Then add a release note block in the PR description as well

@0xmichalis 0xmichalis mentioned this pull request Apr 20, 2017
Merged
@0xmichalis
Copy link
Contributor Author

@janetkuo comments addressed

@0xmichalis 0xmichalis mentioned this pull request Apr 21, 2017
Closed
@janetkuo
Copy link
Member

Thanks! LGTM

Please update PR title and release note since this also changes DaemonSets. Please also squash commits.

@0xmichalis 0xmichalis changed the title Update deployment completeness Update deployment and daemonset completeness checks Apr 22, 2017
@0xmichalis
Copy link
Contributor Author

Please update PR title and release note since this also changes DaemonSets. Please also squash commits.

Updated PR title and release note. How do you want me to squash the commits? The first one is a straight revert (and it better stay like that), the second one is another revert, albeit not straight. Squashing the other two makes sense.

@lukaszo
Copy link
Contributor

lukaszo commented Apr 24, 2017

/approve

@janetkuo
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, kargakis, lukaszo

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43575, 44672)

@k8s-github-robot k8s-github-robot merged commit f0ce5bd into kubernetes:master Apr 24, 2017
@0xmichalis 0xmichalis deleted the update-deployment-completeness branch April 24, 2017 17:59
@0xmichalis 0xmichalis mentioned this pull request May 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

Assignees

@smarterclayton smarterclayton

@janetkuo janetkuo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
9 participants
@0xmichalis @k8s-reviewable @janetkuo @lukaszo @k8s-github-robot @fabianofranz @sttts @smarterclayton @k8s-ci-robot