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

oc rollout support #8927

Merged
merged 8 commits into from
Jul 1, 2016
Merged

oc rollout support #8927

merged 8 commits into from
Jul 1, 2016

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented May 18, 2016

TODO:

  • oc rollout
  • oc rollout pause
  • oc rollout resume
  • oc rollout history
  • oc rollout undo
  • tests

Fixes #6896
Part of https://trello.com/c/Mljldox7/643-5-deployments-downstream-support-for-upstream-features

@fabianofranz @ironcladlou @mfojtik @smarterclayton

@0xmichalis
Copy link
Contributor Author

@smarterclayton @fabianofranz thoughts on:
oc rollout instantiate maps to oc deploy --latest
oc rollout undo maps to oc rollback
I don't want to have oc rollout retry before we decide upstream what do do with it. For me, it makes sense to have just oc rollout instantiate for both actions.
Still thinking about oc rollout latest deploying the latest existing image but I am not sure how this should happen atm or if it should happen at all (without an ICT)

@smarterclayton
Copy link
Contributor

The one thing I'm not clear on is triggers. Do we want to implement Pause
first, so that rollout undo can correctly pause the deployment?

On Wed, May 18, 2016 at 6:37 PM, Michail Kargakis [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton @fabianofranz
https://github.com/fabianofranz thoughts on:
oc rollout instantiate maps to oc deploy --latest
oc rollout undo maps to oc rollback
I don't want to have oc rollout retry before we decide upstream what do
do with it. For me, it makes sense to have just oc rollout instantiate
for both actions.
Still thinking about oc rollout latest deploying the latest existing
image but I am not sure how this should happen atm or if it should happen
at all (without an ICT)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8927 (comment)

@0xmichalis
Copy link
Contributor Author

@smarterclayton what is the difference between Pause and a config change trigger?

@smarterclayton
Copy link
Contributor

Pause stops all processing (all triggers, and probably scaling behavior). Config change trigger just stops config change.

@0xmichalis
Copy link
Contributor Author

probably scaling behavior

We are working upstream on enabling this with proportional scaling (scaling isn't supported in the first version of paused). A paused deployment is still a running application and autoscalers should be able to continue working.

@0xmichalis 0xmichalis changed the title [WIP] oc rollout support oc rollout support Jun 10, 2016
@0xmichalis
Copy link
Contributor Author

cc: @fabianofranz @mfojtik @ironcladlou @smarterclayton

oc rollout pause, resume, and history are ready for review.

Sample output for oc rollout histroy:

$ oc rollout history dc/frontend
deploymentconfigs "frontend" history viewed
REVISION    STATUS      CAUSE
1       Complete    caused by a config change
2       Complete    caused by an image change
3       Complete    caused by an image change
4       Failed      cancelled by the user
5       Failed      cancelled as a newer deployment was found running
6       Complete    caused by a config change

Outstanding issue with history: we need to surface deployments triggered manually into the deployment status. The best way to do this is a /instantiate endpoint. Currently oc deploy --latest will return the same cause as the one the deployment config had before the redeployment.

Separate pulls:

  • I will add oc rollout undo either in Rollback rework #9135 if the present pull merges first or in a follow up to this if Rollback rework #9135 merges first.
  • oc rollout status once the rebase lands
  • Move oc deploy to oc rollout in a separate pull, will open issue for this.

@0xmichalis
Copy link
Contributor Author

[test]

@0xmichalis
Copy link
Contributor Author

Outstanding issue with history: we need to surface deployments triggered manually into the deployment status. The best way to do this is a /instantiate endpoint. Currently oc deploy --latest will return the same cause as the one the deployment config had before the redeployment.

Related: #4359

@0xmichalis
Copy link
Contributor Author

Move oc deploy to oc rollout in a separate pull, will open issue for this.

#9298

@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 Jun 16, 2016
@0xmichalis 0xmichalis mentioned this pull request Jun 20, 2016
10 tasks
@0xmichalis
Copy link
Contributor Author

[test]

case deployapi.DeploymentTriggerOnConfigChange:
config.Status.Details.Message = "caused by a config change"
case deployapi.DeploymentTriggerOnImageChange:
config.Status.Details.Message = "caused by an image change"
Copy link
Contributor

Choose a reason for hiding this comment

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

Messages probably should be sentences (or be capitalized). It's fine for now but we should revisit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am merely consistent with our pre-existing messages surfaced by our annotations:

DeploymentCancelledByUser = "cancelled by the user"
DeploymentCancelledNewerDeploymentExists = "cancelled as a newer deployment was found running"
DeploymentFailedUnrelatedDeploymentExists = "unrelated pod with the same name as this deployment is already running"
DeploymentFailedDeployerPodNoLongerExists = "deployer pod no longer exists"

@0xmichalis 0xmichalis added this to the 1.3.0 milestone Jun 25, 2016
@0xmichalis
Copy link
Contributor Author

Somebody needs to start reviewing this @mfojtik @fabianofranz @ironcladlou @smarterclayton

}
t.Spec.Paused = true
_, err := oc.DeploymentConfigs(t.Namespace).Update(t)
// TODO: Pause the deployer containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there issue created to track this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #9629


var _ kubectl.Rollbacker = &DeploymentConfigRollbacker{}

func (r *DeploymentConfigRollbacker) Rollback(namespace, name string, updatedAnnotations map[string]string, toRevision int64, obj runtime.Object) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add godoc

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

@mfojtik
Copy link
Contributor

mfojtik commented Jun 29, 2016

LGTM

@0xmichalis
Copy link
Contributor Author

Opened #9632 that will be needed once this is merged.

@0xmichalis
Copy link
Contributor Author

[merge]

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2016
ChangeCauseAnnotation is hardcoded in PrintRolloutHistory and it needs
to be overriden since other resources that may need to be added in
`kubectl rollout history` may not use it. Instead of adding one more
method in the factory, refactor the existing HistoryViewer interface
to accomodate the change.
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d362664

@openshift-bot
Copy link
Contributor

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

@0xmichalis
Copy link
Contributor Author

conformance never run, [merge]

@0xmichalis
Copy link
Contributor Author

Flaked on #9364. [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 1, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5669/) (Image: devenv-rhel7_4500)

@0xmichalis
Copy link
Contributor Author

#9512, [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d362664

@openshift-bot openshift-bot merged commit 0acacec into openshift:master Jul 1, 2016
@0xmichalis 0xmichalis deleted the oc-rollout branch July 1, 2016 16:31
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.

4 participants