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.

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 deployment Conditions and oc rollout status #9874

Merged
merged 5 commits into from
Oct 13, 2016
Merged

Add deployment Conditions and oc rollout status #9874

merged 5 commits into from
Oct 13, 2016

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Jul 15, 2016


const (
rolloutStatusLong = `
Watch the status of the latest rollout, until it's done.`
Copy link
Member

Choose a reason for hiding this comment

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

Meaning the command waits and blocks and only exit when the rollout is completed? In such case I'd vote for oc rollout watch or oc rollout status --watch (exits right away without the flag) for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This proposal needs to move upstream

On Wed, Jul 20, 2016 at 7:50 PM, Fabiano Franz notifications@github.com
wrote:

In pkg/cmd/cli/cmd/rollout/rollout.go
#9874 (comment):

@@ -134,3 +135,20 @@ func NewCmdRolloutUndo(fullName string, f *clientcmd.Factory, out io.Writer) *co
cmd.Example = fmt.Sprintf(rolloutUndoExample, fullName)
return cmd
}
+
+const (

  • rolloutStatusLong = +Watch the status of the latest rollout, until it's done.

Meaning the command waits and blocks and only exit when the rollout is
completed? In such case I'd vote for oc rollout watch or oc rollout
status --watch (exits right away without the flag) for consistency.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9874/files/7384f4c3e74a711cdb73ecb1d1958c1b3dc6f251#r71573627,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuFf3RedOQQ1cZeVl6Rb8Otb6xPqxmrks5qXl_3gaJpZM4JNVe5
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 13, 2016
@0xmichalis
Copy link
Contributor Author

There is an issue here: kubectl rollout status will watch for events on the provided resource.

deployment/foo: A deployment is considered complete once foo.spec.replicas == foo.status.updatedReplicas

dc/foo: A deployment is considered complete once openshift.io/deployment-phase=Complete is added in the underlying replication controller.

This issue screams for Conditions in the deployment config. The DC controller should update a DC that was successfully deployed with a Condition, oc rollout status (and every other client for that matter) should expect such a Condition before exiting.

@smarterclayton
Copy link
Contributor

Yes.

On Tue, Aug 16, 2016 at 8:44 AM, Michail Kargakis notifications@github.com
wrote:

There is an issue here: kubectl rollout status will watch for events on
the provided resource.

deployment/foo: A deployment is considered complete once foo.spec.replicas
== foo.status.updatedReplicas

dc/foo: A deployment is considered complete once
openshift.io/deployment-phase=Complete is added in the underlying
replication controller.

This issue screams for Conditions in the deployment config. The DC
controller should update a DC that was successfully deployed with a
Condition, oc rollout status (and every other client for that matter)
should expect such a Condition before exiting.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9874 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p5zL3KJOw-cY2ih5CROcz-YB6qoTks5qgbCZgaJpZM4JNVe5
.

@0xmichalis
Copy link
Contributor Author

Blocked for now. Will handle as part of https://trello.com/c/OIggCmzo/699-5-deployments-downstream-conditions

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2016
@0xmichalis 0xmichalis removed the blocked label Oct 3, 2016
@0xmichalis 0xmichalis changed the title oc: add rollout status Add deployment Conditions and oc rollout status Oct 3, 2016
@0xmichalis 0xmichalis added this to the 1.4.0 milestone Oct 3, 2016
@0xmichalis
Copy link
Contributor Author

Added conditions alongside rollout status, needs reviews

rolloutStatusLong = `
Watch the status of the latest rollout, until it's done.`

rolloutStatusExample = ` # Watch the status of the latest rollout
Copy link
Contributor

Choose a reason for hiding this comment

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

this wont watch when the rollout is complete, just print the status, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It watches until the end of the rollout. @fabianofranz asked for a --watch flag that we will default to true to make it clearer: kubernetes/kubernetes#30627

case deployutil.HasImageChangeTrigger(config):
return fmt.Sprintf("Deployment config %q waiting on image update", name), false, nil
case len(config.Spec.Triggers) == 0:
return fmt.Sprintf("Deployment config %q waiting on manual update (use 'oc deploy %s --latest')", name, name), true, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

this will be oc rollout latest right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will update

// Return as an error.
return fmt.Sprintf("%s\n", cond.Message), true, errors.New(cond.Message)
case cond != nil && cond.Reason == deployutil.PausedDeployReason:
return fmt.Sprintf("Deployment config %q is paused. Unpause to continue watching the status of the rollout.\n", config.Name), true, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Unpause/Resume/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 5, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2016
@mfojtik
Copy link
Contributor

mfojtik commented Oct 10, 2016

LGTM [merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@0xmichalis
Copy link
Contributor Author

#11292 and #11240 [merge]

@0xmichalis
Copy link
Contributor Author

#10773 [test]

@0xmichalis
Copy link
Contributor Author

#11240 [test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3417ff0

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9867/)

@0xmichalis
Copy link
Contributor Author

#11353 [merge]

@0xmichalis
Copy link
Contributor Author

#11074 [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3417ff0

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 13, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10009/) (Base Commit: 50cc896) (Image: devenv-rhel7_5171)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants