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.

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

Add `kubectl rollout status` #19946

Merged
merged 1 commit into from May 12, 2016

Conversation

Projects
None yet
8 participants
@janetkuo
Member

janetkuo commented Jan 21, 2016

Pull Request Guidelines

  1. Please read our contributor guidelines.
  2. See our developer guide.
  3. Follow the instructions for labeling and writing a release note for this PR in the block below.
Implement `kubectl rollout status` that can be used to watch a deployment's rollout status

Addresses #17168; depends on #19882 (the "Add kubectl rollout" commit).
See proposal.

Fixes #25235

cc @bgrant0607 @nikhiljindal @ironcladlou @kargakis @kubernetes/sig-config @kubernetes/kubectl @madhusudancs


This change is Reviewable

@googlebot googlebot added the cla: yes label Jan 21, 2016

@k8s-merge-robot k8s-merge-robot added size/L size/XL and removed size/L labels Jan 21, 2016

@janetkuo janetkuo changed the title from WIP: Add kubectl rollout status to RFC: Add kubectl rollout status Jan 21, 2016

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo Jan 21, 2016

Member

This is blocked by #19939 since it depends on deployments watch.

Member

janetkuo commented Jan 21, 2016

This is blocked by #19939 since it depends on deployments watch.

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Jan 21, 2016

Member

Will this command be able to be made equally useful if the deployment and its rc's do not have any of the annotations set by kubectl deploy? I ask because all our deployment object changes will be done with kubectl apply, and it would be great to still have the rollout status command be just as useful.

Member

ghodss commented Jan 21, 2016

Will this command be able to be made equally useful if the deployment and its rc's do not have any of the annotations set by kubectl deploy? I ask because all our deployment object changes will be done with kubectl apply, and it would be great to still have the rollout status command be just as useful.

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo Jan 21, 2016

Member

@ghodss: Based on implementation in #19581 (not merged yet), version annotations will be appended by deployment controller which updates version annotations of a deployment and its RCs when syncing the deployment.

So yes, kubectl rollout status will still be useful if the deployment object changes are done with kubectl apply.

Member

janetkuo commented Jan 21, 2016

@ghodss: Based on implementation in #19581 (not merged yet), version annotations will be appended by deployment controller which updates version annotations of a deployment and its RCs when syncing the deployment.

So yes, kubectl rollout status will still be useful if the deployment object changes are done with kubectl apply.

@bgrant0607 bgrant0607 assigned kargakis and unassigned jlowdermilk Jan 22, 2016

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jan 22, 2016

Member

Is this any different than kubectl get -w deployment/mydep?

If not, we could wait and add something more customized in 1.3.

Member

bgrant0607 commented Jan 22, 2016

Is this any different than kubectl get -w deployment/mydep?

If not, we could wait and add something more customized in 1.3.

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo Jan 22, 2016

Member

@bgrant0607 yes, this is similar to kubectl get -w deployments. I'll add available replicas to get in another PR and we can deal with kubectl rollout status after 1.2 then.

Member

janetkuo commented Jan 22, 2016

@bgrant0607 yes, this is similar to kubectl get -w deployments. I'll add available replicas to get in another PR and we can deal with kubectl rollout status after 1.2 then.

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo Jan 22, 2016

Member

PR #20009 adds availableReplicas to kubectl get deployments.

Member

janetkuo commented Jan 22, 2016

PR #20009 adds availableReplicas to kubectl get deployments.

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Jan 30, 2016

Member

I'm going to close this to get it off my radar. It can be reopened after we decided what we want to do here.

Member

bgrant0607 commented Jan 30, 2016

I'm going to close this to get it off my radar. It can be reopened after we decided what we want to do here.

@janetkuo janetkuo changed the title from RFC: Add kubectl rollout status to Add `kubectl rollout status` May 7, 2016

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo
Member

janetkuo commented May 7, 2016

@kargakis PTAL

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo May 7, 2016

Member

Example output:

$ kubectl edit deployment/nginx
deployment "nginx" edited

$ kubectl rollout status deployment/nginx
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
deployment nginx successfully rolled out
Member

janetkuo commented May 7, 2016

Example output:

$ kubectl edit deployment/nginx
deployment "nginx" edited

$ kubectl rollout status deployment/nginx
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 2 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 3 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 4 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 5 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 6 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 7 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 8 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
Waiting for rollout to finish: 9 out of 10 new replicas has been updated...
deployment nginx successfully rolled out
fmt.Fprintf(out, "%s", status)
// Quit waiting if the rollout is done
if done {
w.Stop()

This comment has been minimized.

@kargakis

kargakis May 9, 2016

Member

defer this immediately after using Watch

@kargakis

kargakis May 9, 2016

Member

defer this immediately after using Watch

This comment has been minimized.

@kargakis

kargakis May 10, 2016

Member

You should remove this though since now it's globally deferred. Also the defer above should be after the error check.

@kargakis

kargakis May 10, 2016

Member

You should remove this though since now it's globally deferred. Also the defer above should be after the error check.

This comment has been minimized.

@janetkuo

janetkuo May 10, 2016

Member

Because we're in a watch loop, without this line users will be stuck in this loop even when the rollout is done. I'd like to stop watching after it's done.

I have another question, if we're doing w.Stop() when the rollout is done, and in WatchLoop w.Stop() is called either when error happens or the interrupt signal is received. Do we still need the defer w.Stop()?

@janetkuo

janetkuo May 10, 2016

Member

Because we're in a watch loop, without this line users will be stuck in this loop even when the rollout is done. I'd like to stop watching after it's done.

I have another question, if we're doing w.Stop() when the rollout is done, and in WatchLoop w.Stop() is called either when error happens or the interrupt signal is received. Do we still need the defer w.Stop()?

This comment has been minimized.

@kargakis

kargakis May 10, 2016

Member

Oh I see. No, you don't need the defer w.Stop above even though in the unlikely event we error out before the watchloop the watch will be leaked. So I would suggest you to move w, err := r.Watch(rv) just before the watchloop since you dont use it before and you are good to go.

@kargakis

kargakis May 10, 2016

Member

Oh I see. No, you don't need the defer w.Stop above even though in the unlikely event we error out before the watchloop the watch will be leaked. So I would suggest you to move w, err := r.Watch(rv) just before the watchloop since you dont use it before and you are good to go.

This comment has been minimized.

@janetkuo

janetkuo May 10, 2016

Member

Oh, right! It's fixed now. Thanks!

@janetkuo

janetkuo May 10, 2016

Member

Oh, right! It's fixed now. Thanks!

Show outdated Hide outdated pkg/kubectl/rollout_status.go Outdated
return "", false, err
}
if deployment.Generation <= deployment.Status.ObservedGeneration {
if deployment.Status.UpdatedReplicas == deployment.Spec.Replicas {

This comment has been minimized.

@kargakis

kargakis May 9, 2016

Member

I am going to create a helper for this in the perma-failed PR, for now, it's ok. The perma-failed PR will be merged later so I will refactor this.

@kargakis

kargakis May 9, 2016

Member

I am going to create a helper for this in the perma-failed PR, for now, it's ok. The perma-failed PR will be merged later so I will refactor this.

Show outdated Hide outdated pkg/kubectl/rollout_status.go Outdated
@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo May 10, 2016

Member

All comments addressed. PTAL

Member

janetkuo commented May 10, 2016

All comments addressed. PTAL

@kargakis

This comment has been minimized.

Show comment
Hide comment
@kargakis

kargakis May 10, 2016

Member

Left some comments, feel free to apply lgtm after you address them.

Member

kargakis commented May 10, 2016

Left some comments, feel free to apply lgtm after you address them.

@janetkuo janetkuo added the lgtm label May 10, 2016

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo
Member

janetkuo commented May 10, 2016

@kargakis thanks!

@kargakis

This comment has been minimized.

Show comment
Hide comment
@kargakis
Member

kargakis commented May 10, 2016

@janetkuo janetkuo removed the lgtm label May 10, 2016

@janetkuo

This comment has been minimized.

Show comment
Hide comment
@janetkuo

janetkuo May 10, 2016

Member

Sorry, missed that. Add some questions in the comment.

Member

janetkuo commented May 10, 2016

Sorry, missed that. Add some questions in the comment.

@janetkuo janetkuo added the lgtm label May 10, 2016

@k8s-merge-robot k8s-merge-robot removed the lgtm label May 10, 2016

@kargakis kargakis added the lgtm label May 10, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented May 10, 2016

GCE e2e build/test passed for commit 67beccc.

@janetkuo janetkuo added this to the v1.3 milestone May 11, 2016

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot May 12, 2016

Contributor

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

Contributor

k8s-merge-robot commented May 12, 2016

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

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented May 12, 2016

GCE e2e build/test passed for commit 67beccc.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot May 12, 2016

Contributor

Automatic merge from submit-queue

Contributor

k8s-merge-robot commented May 12, 2016

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit 0c2641d into kubernetes:master May 12, 2016

3 of 5 checks passed

Jenkins GCE Node e2e Build started sha1 is merged.
Details
Submit Queue Github CI tests are not green.
Details
Jenkins GCE e2e 292 tests run, 122 skipped, 0 failed.
Details
Jenkins unit/integration 6180 tests run, 24 skipped, 0 failed.
Details
cla/google All necessary CLAs are signed

@janetkuo janetkuo referenced this pull request May 18, 2016

Closed

Update deployment user guide when new commands are merged #498

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment