Improve oc cancel-build command #8509

mfojtik · 2016-04-14T10:20:22Z

This is rework of the oc cancel-build command, following the 'options' style @smarterclayton introduced for start-build and other commands already.

It also adds more functions and control over cancelling builds, following this PR: #8453

More specifically:

You can now cancel multiple builds:

oc cancel-build ruby-build-1 ruby-build-2 ...

You can now cancel all (new, pending, running) builds created from build config:

oc cancel-build bc/ruby-build

And you can also select what builds you want to cancel:

oc cancel-build bc/ruby-build --state=new

The last example is basically a command that will cleanup the build queue, allowing the next created build to be first executed (@bparees).

@smarterclayton PTAL.

mfojtik · 2016-04-14T10:22:11Z

[test]

mfojtik · 2016-04-14T10:22:43Z

pkg/build/util/util.go

+// BuildConfigBuilds return a list of builds for the given build config.
+// Optionally you can specify a filter function to select only builds that
+// matches your criteria.
+func BuildConfigBuilds(c client.BuildInterface, name string, filterFunc buildFilter) (*buildapi.BuildList, error) {


this is borrowed from serial builds PR and will be re-used there :-)

jhadvig · 2016-04-14T10:31:52Z

@mfojtik 👍
@jwforres might by worth to think about batch build cancellation in the /builds page, with some checkbox logic

jwforres · 2016-04-14T12:03:45Z

Yeah we have been thinking about batch delete as well.
On Apr 14, 2016 6:32 AM, "Jakub Hadvig" [email protected] wrote:

@mfojtik https://github.com/mfojtik [image: 👍]
@jwforres https://github.com/jwforres might by worth to think about
batch build cancellation in the /builds page, with some checkbox logic

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

deads2k · 2016-04-14T12:40:55Z

Why would I have a --from instead of accepting the resource/name format like every other command making my choice based on that information?

@Kargakis

mfojtik · 2016-04-14T12:45:47Z

@deads2k I think it will make it harder for users to support that format, think of:

oc cancel-build bc/foo build/bar-1 foobar-1 --state=new

What will this do? Cancel all builds created from bc/foo, then cancel build/bar-1 and then cancel foobar-1 that might be BC or build?...

Also buildConfig is not a build and the command is cancel-build, not cancel build config.

bparees · 2016-04-14T12:46:29Z

This is related to #5193

Note that this is proposing slightly different behavior than described in the issue (this PR would cancel all builds for a BC, whereas in the issue we discussed only canceling if there's just a single build running for the BC). I think what's proposed here is ok/better though (behavior wise. i agree with @deads2k that the syntax should be more like cancel-build bc/foo)

liggitt · 2016-04-14T12:48:02Z

Agree on cancel-build bc/foo syntax. We do similar things for rsh dc/foo where we find the pod connected to the thing you asked to rsh into

bparees · 2016-04-14T12:48:15Z

What will this do? Cancel all builds created from bc/foo, then cancel build/bar-1 and then cancel foobar-1 that might be BC or build?...

for backwards compatibility, if no resource type is specified, you must assume it's a build. just like "oc logs" assumes you provided a pod name.

mfojtik · 2016-04-14T12:48:44Z

@liggitt @bparees does oc cancel-**build** buildconfig/foo sounds right to you?

bparees · 2016-04-14T12:49:17Z

@mfojtik it's not ideal, but it doesn't sound totally wrong. after all, "oc start-build" takes a BC....

mfojtik · 2016-04-14T12:49:58Z

@bparees I see, actually I looked on start-build and it is accepting both as well :-) OK, I'm surrendering :)

liggitt · 2016-04-14T12:50:28Z

Take the thing they gave you and find a build logically connected to it you can cancel. Makes sense to me.

mfojtik · 2016-04-14T13:14:37Z

@liggitt @bparees @deads2k updated, I like it more now :)

0xmichalis · 2016-04-14T19:31:37Z

We do similar things for rsh dc/foo where we find the pod connected to the thing you asked to rsh into

Not yet, but in progress. #8279

mfojtik · 2016-04-15T12:53:52Z

[test] seems like a flake in integration (failed to pull image)

mfojtik · 2016-04-15T12:55:56Z

@Kargakis @bparees can I have review pls? :-)

bparees · 2016-04-15T19:13:03Z

pkg/cmd/cli/cmd/cancelbuild.go

 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 )

 const (
 	cancelBuildLong = `
-Cancels a pending or running build
+Cancels a pending, running or new builds


s/builds/build/

0xmichalis · 2016-04-17T16:25:14Z

What will this do? Cancel all builds created from bc/foo,

I thought we would support canceling the latest but cancelling all of the builds for that bc also makes sense.

then cancel build/bar-1 and then cancel foobar-1 that might be BC or build?...

cancel-build supports builds currently so we would continue to resolve single arguments to builds.

0xmichalis · 2016-04-17T16:29:20Z

pkg/cmd/cli/cmd/cancelbuild.go

-	if err != nil {
-		return err
+	if len(o.State) > 0 && !isStateCancellable(o.State) {
+		return kcmdutil.UsageError(cmd, "The '--state' flag has invalid value. Must be 'new', 'pending' or 'running'")


oxford comma

smarterclayton · 2016-04-26T21:32:00Z

I'm provisionally ok with the experience you describe, but given deployments has a bunch of stuff recently just want to be sure.

0xmichalis · 2016-04-27T08:07:41Z

Cancelling all cancelable builds for a bc makes sense to me. We cannot have the same experience in deployments because there should always be one cancelable deployment. But even in the case there are more than one active deployments (can happen in the upstream controller), we should be able to cancel them all at once. Either way, multiple deployments means all but the latest are rolling out their pods to the latest.

mfojtik · 2016-04-27T08:50:40Z

@bparees @smarterclayton @Kargakis fixed test, I think we are good to merge after tests pass.

mfojtik · 2016-04-27T09:10:39Z

@smarterclayton is there anything we can do with travis flakes on import ordering? maybe disable the check for now till we fix it? :)

mfojtik · 2016-04-27T10:28:00Z

[test]

smarterclayton · 2016-04-27T15:29:31Z

We probably need to just fix it.

On Wed, Apr 27, 2016 at 5:10 AM, Michal Fojtik [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton is there anything we
can do with travis flakes on import ordering? maybe disable the check for
now till we fix it? :)

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

mfojtik · 2016-04-28T10:21:31Z

seems like jenkins hate me today... [test]

mfojtik · 2016-04-28T12:51:58Z

[test]

(Error: image cache/mysql:pullthrough not found flake)

mfojtik · 2016-04-28T13:11:13Z

[test] ?

mfojtik · 2016-04-29T11:52:10Z

[test]

(integration flake)

mfojtik · 2016-05-02T07:58:13Z

@smarterclayton @bparees any final comments?

mfojtik · 2016-05-02T10:31:11Z

(actually I would prefer to merge the run policy PR first)

openshift-bot · 2016-05-02T18:30:23Z

Evaluated for origin test up to a37693a

bparees · 2016-05-02T19:16:47Z

@mfojtik still lgtm.

openshift-bot · 2016-05-02T19:50:11Z

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

mfojtik · 2016-05-02T19:54:46Z

[merge]

openshift-bot · 2016-05-02T19:55:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5784/) (Image: devenv-rhel7_4078)

openshift-bot · 2016-05-02T19:55:21Z

Evaluated for origin merge up to a37693a

mfojtik reviewed Apr 14, 2016
View reviewed changes

mfojtik force-pushed the cancel-build branch 2 times, most recently from 62cd88d to 474a27e Compare April 14, 2016 10:29

mfojtik force-pushed the cancel-build branch from 474a27e to 3fc14c5 Compare April 14, 2016 12:33

mfojtik force-pushed the cancel-build branch 2 times, most recently from ec20874 to 81dbcf5 Compare April 14, 2016 13:14

mfojtik force-pushed the cancel-build branch 3 times, most recently from d4bff9d to 35e2947 Compare April 15, 2016 09:46

bparees reviewed Apr 15, 2016
View reviewed changes

0xmichalis reviewed Apr 17, 2016
View reviewed changes

mfojtik force-pushed the cancel-build branch from 5d4c5ff to e187fad Compare April 27, 2016 08:48

mfojtik force-pushed the cancel-build branch from e187fad to 81f1bce Compare April 27, 2016 13:43

mfojtik force-pushed the cancel-build branch from 81f1bce to 885b8f5 Compare April 28, 2016 08:27

mfojtik force-pushed the cancel-build branch from ddf7809 to 7fa3f12 Compare April 28, 2016 11:02

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016

Improve oc cancel-build command

a37693a

mfojtik force-pushed the cancel-build branch from 7fa3f12 to a37693a Compare May 2, 2016 18:25

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2016

openshift-bot merged commit cd62e58 into openshift:master May 2, 2016

0xmichalis mentioned this pull request Jun 12, 2016

cancel-build should take a buildconfig #5193

Closed

adellape mentioned this pull request Jul 5, 2016

Bug 1352387, documented the ability to cancel multiple builds openshift/openshift-docs#2456

Merged

mfojtik deleted the cancel-build branch September 5, 2018 21:07

Improve oc cancel-build command #8509

Improve oc cancel-build command #8509

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mfojtik commented Apr 14, 2016

Uh oh!

mfojtik Apr 14, 2016

Choose a reason for hiding this comment

Uh oh!

jhadvig commented Apr 14, 2016

Uh oh!

jwforres commented Apr 14, 2016

Uh oh!

deads2k commented Apr 14, 2016

Uh oh!

mfojtik commented Apr 14, 2016

Uh oh!

bparees commented Apr 14, 2016

Uh oh!

liggitt commented Apr 14, 2016

Uh oh!

bparees commented Apr 14, 2016

Uh oh!

mfojtik commented Apr 14, 2016

Uh oh!

bparees commented Apr 14, 2016

Uh oh!

mfojtik commented Apr 14, 2016

Uh oh!

liggitt commented Apr 14, 2016

Uh oh!

mfojtik commented Apr 14, 2016

Uh oh!

0xmichalis commented Apr 14, 2016

Uh oh!

mfojtik commented Apr 15, 2016

Uh oh!

mfojtik commented Apr 15, 2016

Uh oh!

bparees Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

0xmichalis commented Apr 17, 2016

Uh oh!

0xmichalis Apr 17, 2016

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Apr 26, 2016

Uh oh!

0xmichalis commented Apr 27, 2016

Uh oh!

mfojtik commented Apr 27, 2016

Uh oh!

mfojtik commented Apr 27, 2016

Uh oh!

mfojtik commented Apr 27, 2016

Uh oh!

smarterclayton commented Apr 27, 2016

Uh oh!

mfojtik commented Apr 28, 2016

Uh oh!

mfojtik commented Apr 28, 2016

Uh oh!

mfojtik commented Apr 28, 2016

Uh oh!

mfojtik commented Apr 29, 2016

Uh oh!

mfojtik commented May 2, 2016

Uh oh!

mfojtik commented May 2, 2016

Uh oh!

openshift-bot commented May 2, 2016

Uh oh!

bparees commented May 2, 2016

Uh oh!

openshift-bot commented May 2, 2016

Uh oh!

mfojtik commented Apr 14, 2016 •

edited

Loading

openshift-bot commented May 2, 2016 •

edited

Loading