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

Add MinReadySeconds to rolling updater #28111

Merged
merged 1 commit into from
Jul 2, 2016
Merged

Add MinReadySeconds to rolling updater #28111

merged 1 commit into from
Jul 2, 2016
+81 −26

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Jun 27, 2016

Add MinReadySeconds support to RollingUpdater that allows to specify the number of seconds to wait on top of the pod is "ready" because its readiness probe passed.

@mfojtik
Copy link
Contributor Author

mfojtik commented Jun 27, 2016

@smarterclayton @Kargakis FYI

@mfojtik mfojtik mentioned this pull request Jun 27, 2016
Closed
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 27, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 27, 2016

GCE e2e build/test failed for commit b25f4a49ac40eb91aeb036fbdf07a3f8b2b2aa99.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@mfojtik mfojtik force-pushed the min-ready-seconds branch from b25f4a4 to c3ee0eb Compare June 27, 2016 13:34
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 27, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 27, 2016

GCE e2e build/test failed for commit c3ee0ebc2bb13255b7a7c195e2e27505e5eac91b.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@0xmichalis
Copy link
Contributor

@kubernetes/kubectl

@0xmichalis 0xmichalis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 27, 2016
0xmichalis
0xmichalis reviewed Jun 27, 2016
View reviewed changes
pkg/kubectl/rolling_updater.go
if !api.IsPodReady(&pod) {
continue
}
// Compute the time where the Pod should be ready, adding the MinReadySeconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/where/when/

@0xmichalis
Copy link
Contributor

Needs a unit test.

@j3ffml
Copy link
Contributor

j3ffml commented Jun 27, 2016

Is the plan to expose this for deployments with an annotation, or something else?

@0xmichalis
Copy link
Contributor

Is the plan to expose this for deployments with an annotation, or something else?

Are you talking about the Deployment object or kubectl rolling-update? It's already in the former as an API field, as far as the later is concerned, we use just the rolling updater and want to expose it downstream. It's your call for exposing it in kubectl.

@mfojtik mfojtik force-pushed the min-ready-seconds branch from c3ee0eb to 124f990 Compare June 28, 2016 12:12
@mfojtik
Copy link
Contributor Author

mfojtik commented Jun 28, 2016

@Kargakis test added

@mfojtik mfojtik force-pushed the min-ready-seconds branch from 124f990 to 6b34073 Compare June 28, 2016 12:15
@mfojtik mfojtik changed the title WIP: Add MinReadySeconds to rolling updater Add MinReadySeconds to rolling updater Jun 28, 2016
@mfojtik mfojtik force-pushed the min-ready-seconds branch from 6b34073 to 39782fa Compare June 28, 2016 12:21
@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 Jun 28, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 28, 2016

GCE e2e build/test failed for commit 124f9905b93ab9373cc5591d4e43bc0351d34a1f.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Jun 28, 2016

GCE e2e build/test passed for commit 6b340733fbe53c99dd689df3e2761c11423aeabb.

@k8s-bot
Copy link

k8s-bot commented Jun 28, 2016

GCE e2e build/test passed for commit 39782fabd0232291b7324cd439f1ed34dfabad5c.

@mfojtik
Copy link
Contributor Author

mfojtik commented Jun 30, 2016

@Kargakis done.

0xmichalis
0xmichalis reviewed Jun 30, 2016
View reviewed changes
pkg/kubectl/rolling_updater.go
case newRc.Name:
newReady++
}
if r.nowFn != nil && !deployment.IsPodAvailable(&pod, minReadySeconds, r.nowFn().Time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If nowFn is nil default to Now(). Otherwise, you will consider every pod as ready, right?

if r.nowFn == nil {
    r.nowFn = unversioned.Now()
}
if !deployment.IsPodAvailable(&pod, minReadySeconds, r.nowFn().Time) {
    continue
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis ready in no time :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis the nowFn is defaulted in initializer:

    updater.nowFn = func() unversioned.Time { return unversioned.Now() }

but yes, in case you don't use initializer, we should default there (tests)

@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 8f395e8eb020f54c6c67aaa96140d23135b3f9d9.

0xmichalis
0xmichalis reviewed Jun 30, 2016
View reviewed changes
pkg/kubectl/rolling_updater_test.go
Status: status,
Type: api.PodReady,
Status: status,
LastTransitionTime: now,
Copy link
Contributor

Choose a reason for hiding this comment

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

Have this time being fixed and pass two different times (so you will need two different test cases below) - one that is after this time and one that is before.

@mfojtik mfojtik force-pushed the min-ready-seconds branch 2 times, most recently from b783c17 to 464d136 Compare June 30, 2016 10:05
@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit b783c17f972b284fd387da1f6bc5c05e6761fbc1.

0xmichalis
0xmichalis reviewed Jun 30, 2016
View reviewed changes
pkg/kubectl/rolling_updater_test.go
oldReady, newReady int32
err error
)
oldReady, newReady, err = updater.readyPods(test.oldRc, test.newRc, test.minReadySeconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

oldReady, newReady, err :=

Copy link
Contributor

Choose a reason for hiding this comment

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

The point of initializing was to remove the variables above too:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ETOOMUCHMULTITASKING

@mfojtik mfojtik force-pushed the min-ready-seconds branch from 464d136 to 2921784 Compare June 30, 2016 10:37
@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 464d136005a367bc576f4d204388c2c03270f4f9.

@0xmichalis
Copy link
Contributor

One more comment, then LGTM

@mfojtik mfojtik force-pushed the min-ready-seconds branch from 2921784 to 7ea28e4 Compare June 30, 2016 11:13
@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 2921784e9c8766196265bf5e6c44a0c5a9522bf4.

@0xmichalis 0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 7ea28e4.

@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 7ea28e4.

@0xmichalis
Copy link
Contributor

@k8s-bot test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 7ea28e4.

@j3ffml
Copy link
Contributor

j3ffml commented Jul 1, 2016

@k8s-bot test this issue: #IGNORE (jenkins plugin crash)

@k8s-bot
Copy link

k8s-bot commented Jul 1, 2016

GCE e2e build/test passed for commit 7ea28e4.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 2, 2016

GCE e2e build/test passed for commit 7ea28e4.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3166f93 into kubernetes:master Jul 2, 2016
@0xmichalis 0xmichalis mentioned this pull request Jul 13, 2016
Closed
@pmorie pmorie mentioned this pull request Jul 30, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Aug 1, 2016
Merge pull request #29826 from pmorie/desimplify-volume-controller
31da82d 
Automatic merge from submit-queue

Document space shuttle style in controller/volume

Reverts #28813 and #28111.

@xiang90, I really appreciate the effort that went into your PRs (and think, in general, code simplification is a worthwhile effort), but the style in this controller was intentional to ensure that every branch is covered.  The verbosity and branchyness of this controller stores a lot of context and knowledge about how this subsystem is meant to function, so we need to put them back in.

@kubernetes/sig-storage 
cc @jsafrane @saad-ali @matchstick @thockin @childsb @rootfs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@j3ffml j3ffml

@0xmichalis 0xmichalis

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mfojtik @k8s-bot @0xmichalis @j3ffml @k8s-github-robot @googlebot