oc: add rollout cancel by mfojtik · Pull Request #10731 · openshift/origin

mfojtik · 2016-08-31T12:45:28Z

This moves the code responsible for cancellation to cmd/cli/rollback/cancel.go and add new sub-command for oc rollout. It also provides compatibility helper for oc deploy --cancel:

$ oc deploy --latest dc/demo-app
Started deployment #6
Use 'oc logs -f dc/demo-app' to track its progress.

$ oc rollout cancel dc/demo-app
Cancelled deployment #6

// To verify the old cancel works
$ oc deploy --latest dc/demo-app
Started deployment #7
Use 'oc logs -f dc/demo-app' to track its progress.

$ oc deploy --cancel dc/demo-app
Cancelled deployment #7

Fixes: #11225

@Kargakis @fabianofranz PTAL

0xmichalis · 2016-08-31T12:47:35Z

+			allErrs := []error{}
+			err := opts.Complete(f, args, out)
+			if err != nil {
+				allErrs = append(allErrs, err)


We shouldn't do this. That's the problem you need to fix for rollout pause and rollout resume upstream in kubernetes/kubernetes#31234

fixed (here).

0xmichalis · 2016-09-05T09:41:57Z

+
+func (o *RolloutCancelOptions) Complete(f *clientcmd.Factory, args []string, out io.Writer) error {
+	if len(args) > 1 {
+		return errors.New("only one deployment config name is supported as argument.")


Don't the rest of the rollout commands support multiple arguments?

yeah, I think I want to rework this to follow the upstream pattern.

0xmichalis · 2016-09-05T09:43:53Z

@openshift/cli-review this is part of #9298

0xmichalis · 2016-09-05T09:44:41Z

Needs a test in hack/test-cmd.sh

fabianofranz · 2016-09-08T18:15:27Z

 }

 // cancel cancels any deployment process in progress for config.
+// TODO: this code will be deprecated


Are we already going to cmd.Flags().MarkDeprecated in --cancel, or not yet?

@fabianofranz I think we can do that now.

fabianofranz · 2016-09-09T18:16:05Z

Might need to review the help info in oc deploy (remove references to "cancel" and possibly point to oc rollout, etc).

mfojtik · 2016-10-05T11:34:54Z

@fabianofranz right, if we mark the --cancel as deprecated, we need to replace all references to it everywhere.

mfojtik · 2016-10-05T11:57:05Z

@Kargakis @fabianofranz marked --cancel as deprecated (i hope everywhere). PTAL

I will follow up with documentation PR when this is merged.

0xmichalis · 2016-10-05T11:59:34Z

 					e2e.Logf("%02d: cancelling deployment", i)
-					if out, err := oc.Run("deploy").Args("dc/deployment-simple", "--cancel").Output(); err != nil {
+					if out, err := oc.Run("rollout").Args("cancel", "dc/deployment-simple").Output(); err != nil {
 						// TODO: we should fix this


A different issue but do we want to retry cancel on update conflicts? Better yet should we use PATCH for cancel?

@Kargakis i think I like the PATCH approach... we should get it merged in upstream... i will spawn an issue and do it as follow up.

upstream does not have rollout cancel

mfojtik · 2016-10-05T12:05:43Z

[test]

mfojtik · 2016-10-05T14:11:57Z

@openshift/cli-review PTAL

0xmichalis · 2016-12-07T13:01:58Z

+			}
+			rc.Annotations[deployapi.DeploymentCancelledAnnotation] = deployapi.DeploymentCancelledAnnotationValue
+			rc.Annotations[deployapi.DeploymentStatusReasonAnnotation] = deployapi.DeploymentCancelledByUser
+			if _, err := o.Client.ReplicationControllers(rc.Namespace).Update(rc); err != nil {


Maybe we should retry on conflicts until we switch to patch?

i can wrap it with RetryOnConflict and add TODO

scratch that... switching this command to client set and patch \o/

mfojtik · 2016-12-07T13:52:43Z

@Kargakis updated to use PATCH (and addressed your comments)

0xmichalis · 2016-12-07T14:07:16Z

+			}
+
+			patches := set.CalculatePatches([]*resource.Info{&resource.Info{Object: rc, Mapping: mapping}}, o.Encoder, func(rcInfo *resource.Info) (bool, error) {
+				deployment, ok := rcInfo.Object.(*kapi.ReplicationController)


Can you not use "deployment" for a rc?

i see what you mean now

0xmichalis · 2016-12-07T14:07:35Z

 	cmd.Flags().MarkDeprecated("latest", fmt.Sprintf("use '%s rollout latest' instead", fullName))
 	cmd.Flags().BoolVar(&options.retryDeploy, "retry", false, "Retry the latest failed deployment.")
 	cmd.Flags().BoolVar(&options.cancelDeploy, "cancel", false, "Cancel the in-progress deployment.")
+	cmd.Flags().MarkDeprecated("cancel", fmt.Sprintf("use %s rollout cancel instead", fullName))


add single quotes around the command

0xmichalis · 2016-12-07T14:07:42Z

 	cmd.Flags().BoolVar(&options.cancelDeploy, "cancel", false, "Cancel the in-progress deployment.")
+	cmd.Flags().MarkDeprecated("cancel", fmt.Sprintf("use %s rollout cancel instead", fullName))
 	cmd.Flags().BoolVar(&options.enableTriggers, "enable-triggers", false, "Enables all image triggers for the deployment config.")
+	cmd.Flags().MarkDeprecated("enable-triggers", fmt.Sprintf("use %s set triggers instead", fullName))


add single quotes around the command

0xmichalis · 2016-12-07T14:08:50Z

+		return nil, err
+	}
+	if len(deployments.Items) == 0 {
+		return nil, fmt.Errorf("there have been no deployments for %s/%s\n", namespace, name)


s/deployments/replication controllers/

0xmichalis · 2016-12-07T14:11:11Z

+				maybeCancelling = " (cancelling)"
+			}
+			timeAt := strings.ToLower(units.HumanDuration(time.Now().Sub(latest.CreationTimestamp.Time)))
+			fmt.Fprintf(o.Out, "No deployments are in progress (latest deployment #%d %s%s %s ago)\n",


No rollouts are in progress (latest rollout ... )

0xmichalis · 2016-12-07T14:12:52Z

+
+	for _, deployment := range deployments.Items {
+		status := deployutil.DeploymentStatusFor(&deployment)
+		switch status {


Should we print something in case a rollout for a rc is terminated?

we did not for oc deploy --cancel

Actually we do

$ oc deploy --cancel dc/router No deployments are in progress (latest deployment #1 complete 4 minutes ago)

but it's already covered in your code.

0xmichalis · 2016-12-07T14:14:47Z

+				return
+			}
+			kcmdutil.PrintSuccess(o.Mapper, false, o.Out, info.Mapping.Resource, info.Name, false, "cancelling")
+			cancelled = true


This will change to true if the latest rollout for a DC is cancelled but what if you have an additional DC with the latest rollout still ongoing?

2 parallel rollouts for 1 dc?

@Kargakis we reset this to false for each DC

Now I see, ok

i reworked this to be more clean and do not shadow the cancelled variable in the function

0xmichalis · 2016-12-07T14:25:24Z

LGTM [merge]

mfojtik · 2016-12-07T14:28:35Z

[test]

openshift-bot · 2016-12-08T10:53:23Z

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

mfojtik · 2016-12-08T12:13:47Z

flakes: #11024 && yum

[merge]

openshift-bot · 2016-12-08T12:21:20Z

Evaluated for origin merge up to ba3450c

mfojtik · 2016-12-09T09:54:47Z

[test]

openshift-bot · 2016-12-09T10:05:23Z

Evaluated for origin test up to ba3450c

openshift-bot · 2016-12-09T11:37:32Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12180/) (Base Commit: f2427b8)

mfojtik added this to the 1.3.1 milestone Aug 31, 2016

0xmichalis reviewed Aug 31, 2016
View reviewed changes

mfojtik force-pushed the rollout-cancel branch 2 times, most recently from c432680 to 0635840 Compare September 5, 2016 09:11

0xmichalis reviewed Sep 5, 2016
View reviewed changes

mfojtik force-pushed the rollout-cancel branch 2 times, most recently from f191bbe to 68e784e Compare September 5, 2016 15:14

fabianofranz reviewed Sep 8, 2016
View reviewed changes

smarterclayton modified the milestones: 1.3.1, 1.4.0 Sep 19, 2016

mfojtik force-pushed the rollout-cancel branch 2 times, most recently from a27e506 to 114e5fb Compare October 5, 2016 11:55

0xmichalis reviewed Oct 5, 2016

View reviewed changes

mfojtik mentioned this pull request Oct 5, 2016

Use PATCH for cancelling deployments #11225

Closed

mfojtik force-pushed the rollout-cancel branch 4 times, most recently from 90d3a2c to b93a2be Compare October 10, 2016 11:00

0xmichalis reviewed Dec 7, 2016

View reviewed changes

mfojtik force-pushed the rollout-cancel branch from 49794cd to 6e3284f Compare December 7, 2016 13:52

0xmichalis reviewed Dec 7, 2016

View reviewed changes

mfojtik force-pushed the rollout-cancel branch 2 times, most recently from 011d784 to 74bcc22 Compare December 7, 2016 14:17

mfojtik force-pushed the rollout-cancel branch from 74bcc22 to 682df97 Compare December 7, 2016 14:26

mfojtik force-pushed the rollout-cancel branch from 682df97 to fbf199e Compare December 7, 2016 15:31

mfojtik added 3 commits December 7, 2016 18:43

deploy: add rollout cancel command and deprecate deploy --cancel

96efb37

deploy: deprecate --enable-triggers in favor of set triggers

030b1ed

deploy: use rollout cancel in extended test

125dabb

mfojtik force-pushed the rollout-cancel branch from fbf199e to 800451f Compare December 7, 2016 17:43

generate man and bash completion

ba3450c

mfojtik force-pushed the rollout-cancel branch from 800451f to ba3450c Compare December 7, 2016 19:26

openshift-bot merged commit 5e81dae into openshift:master Dec 9, 2016

mfojtik deleted the rollout-cancel branch September 5, 2018 21:08

Conversation

mfojtik commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Sep 5, 2016

Uh oh!

0xmichalis commented Sep 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianofranz commented Sep 9, 2016

Uh oh!

mfojtik commented Oct 5, 2016

Uh oh!

mfojtik commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Oct 5, 2016

Uh oh!

mfojtik commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Dec 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmichalis Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Aug 31, 2016 •

edited

Loading

0xmichalis Dec 7, 2016 •

edited

Loading

openshift-bot commented Dec 8, 2016 •

edited

Loading