Allow to specify runPolicy for BuildConfig #8453

mfojtik · 2016-04-11T11:56:58Z

Add runPolicy=Parallel | Serial | SerialLatestOnly into the BuildConfig.
- The Parallel represents current behavior.
- The Serial means the builds will be execute sequentially.
- The SerialLatestOnly means that only the latest build will be executed and previous non-running builds will be cancelled.

The Serial will be default when this PR gets merged. This will be documented for users.

mfojtik · 2016-04-11T12:01:59Z

@bparees @smarterclayton this is an very early draft of the API additions needed for https://trello.com/c/Wz8FFwko/849-13-single-threaded-builds. Would appreciate quick review before I jump into actual implementation.

mfojtik · 2016-04-11T12:02:17Z

pkg/build/api/types.go

+	// make the build created from this build requests be the only build in the
+	// queue. This option is valid only for build configurations with "recent"
+	// build scheduling policy.
+	SkipBuildsInQueue *bool


@smarterclayton @bparees maybe this deserves a better name ;-)

What does Jenkins call this flag? How many flags do they have? What about jenkins pipeline and concourse?

@jstrachan && @jimmidyson to the rescue :-)

There isn't a direct equivalent afaik. There's also accelerated build which will run the new build immediately, but not cancel the other builds. It's all possible of course :) @jstrachan Do you know any different?

not seen anything else equivalent I'm afraid

@jimmidyson @jstrachan thanks guys, we are opting out this option and have one parallel and serial builds for now.

not sure this is the way we want to expose that behavior anyway. i think the use case is more like:

i started a build

i look at the build queue and see it's full of garbage

i run "oc skip-builds" (name tbd) which skips all the queued builds except the most recent one (mine).

@bparees iow.

oc skip-build --to ruby-10

--to is fancy :) but sure.

bparees · 2016-04-11T19:05:35Z

pkg/build/api/types.go

@@ -86,6 +86,10 @@ type BuildStatus struct {
 	// Cancelled describes if a cancel event was triggered for the build.
 	Cancelled bool

+	// Position represents the build position in the build queue. Only applies to
+	// build configurations with "serial" or "recent" scheduling policy.
+	Position *int64


going to use the build number annotation instead?

yes, already updated this pull.

mfojtik · 2016-04-12T09:06:01Z

@bparees @smarterclayton PTAL. I did some initial implementation for serial builds.

It works, but it is not ideal.. The builds are executed sequentially based on the build number we store in the annotation.
The problem is that the time for retrying the new build handling seems very long. When one build finishes, it took about ~30 seconds when the next build is scheduled. I dunno if this is something we can tweak (build completion force trigger handing builds in New state).

Also the BuildController has 30 minutes retry timeout... does that mean if I have big queue for builds and they will not finish in 30 minutes, they will be marked as failed? Another solution would be to have separate controller that handle only "serial" builds.

mfojtik · 2016-04-12T09:15:40Z

@smarterclayton @bparees little demo: https://www.youtube.com/watch?v=AEEOgmjnNHQ

bparees · 2016-04-12T13:24:05Z

When one build finishes, it took about ~30 seconds when the next build is scheduled. I dunno if this is something we can tweak (build completion force trigger handing builds in New state).

that should definitely be done. I guess i didn't put it in the card but i was assuming that controller should, on seeing a build enter a terminal state, revisit the queue and pick the next build.

doesn't solve the "stuck build that is waiting for an IST" scenario though.

mfojtik · 2016-04-12T13:30:29Z

@bparees it basically means that an event of "complete" build should kick HandleBuild() for all queued builds....

smarterclayton · 2016-04-12T13:39:46Z

api/swagger-spec/oapi-v1.json

@@ -17354,6 +17354,10 @@
      },
      "description": "Triggers determine how new Builds can be launched from a BuildConfig. If no triggers are defined, a new build can only occur as a result of an explicit client build creation."
     },
+     "schedulePolicy": {
+      "$ref": "v1.BuildSchedulePolicy",
+      "description": "SchedulePolicy describes how the new build created from this build configuration will be scheduled for execution. This is optional, if not specified we default to \"parallel\"."


I think we would default to serial?

@smarterclayton right. I was thinking about that and I think it make sense. Serial is also default in Jenkins.

i was thinking we'd default to parallel for backwards compatibility reasons.

i'm ok w/ changing it, but we need to acknowledge we're changing the default behavior for users.

@smarterclayton i found the schedulePolicy long to type ;-) WDYT about changing that just to schedule: parallel ?

@smarterclayton should we still default to Serial or SerialLatestOnly? I think Serial will be "safe" default.

bparees · 2016-04-12T14:13:23Z

@bparees it basically means that an event of "complete" build should kick HandleBuild() for all queued builds....

not just complete. also failed, canceled, etc.

mfojtik · 2016-04-12T19:44:09Z

@bparees made some improvements in the last commit, https://www.youtube.com/watch?v=0MpmnYebdHw

smarterclayton · 2016-04-12T19:57:38Z

I think you guys need to define what skipped is before we argue about how
it's represented in the API :)

On Tue, Apr 12, 2016 at 3:44 PM, Michal Fojtik [email protected]
wrote:

@bparees https://github.com/bparees made some improvements in the last
commit, https://www.youtube.com/watch?v=0MpmnYebdHw

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

bparees · 2016-04-12T19:59:26Z

pkg/build/controller/controller.go

+		if nextBuild != nil {
+			glog.Infof("Queue: The next build is %s", nextBuild.Name)
+			switch build.Status.Phase {
+			case buildapi.BuildPhaseError, buildapi.BuildPhaseFailed, buildapi.BuildPhaseComplete, buildapi.BuildPhaseCancelled:


pkg/build/util/util.go:IsBuildComplete

bparees · 2016-04-12T20:03:13Z

I think you guys need to define what skipped is before we argue about how it's represented in the API :)

to me, skipped is a reason. canceled is the state.

canceled - because the user canceled it
canceled - because it was auto-skipped due to the presence of a newer build (function we are not implementing yet)
canceled - because the user requested we skip queued builds and run the most recent one instead

mfojtik · 2016-04-13T08:38:50Z

@bparees +1 we don't want to introduce new phase, so we have to live with existing phases. The best fit for "skipped" is cancelled with a good reason (Ben described the reasons).

mfojtik · 2016-04-13T10:51:30Z

pkg/build/api/types.go

+	// SchedulePolicy describes how the new build created from this build
+	// configuration will be scheduled for execution.
+	// This is optional, if not specified we default to "parallel".
+	SchedulePolicy *BuildSchedulePolicy


@smarterclayton API question... do you think this field should be here or in BuildSpec? I think it should be higher level (same as Triggers), WDYT?

Since this doesn't apply to Builds, it makes sense to me that it is not part of BuildSpec.

right. I think it should live on BuildConfig as it is now.

but renamed i guess per the type renaming below.

@bparees yes as soon as we decide the name :-)

mfojtik · 2016-04-28T11:32:58Z

[test]

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

[test]

(integration flake)

mfojtik · 2016-04-29T12:54:39Z

@smarterclayton @bparees merge while it is green :-)

jimmidyson · 2016-04-29T13:03:09Z

@mfojtik Don't you make me rebase #7949 again!

bparees · 2016-04-29T15:21:23Z

@mfojtik still needs api approved label from @smarterclayton

smarterclayton · 2016-04-29T17:20:22Z

Approved, thanks for following process :)

bparees · 2016-04-29T17:26:05Z

lgtm but we'll let @jimmidyson's PR merge first :) (it just got tagged)

mfojtik · 2016-05-02T08:13:46Z

@bparees rebased... will wait for tests to pass :-)

mfojtik · 2016-05-02T10:12:48Z

[test]

openshift-bot · 2016-05-02T12:00:16Z

Evaluated for origin testonlyextended up to bd47008

openshift-bot · 2016-05-02T12:27:44Z

continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/132/) (Extended Tests: core(using build configuration))

mfojtik · 2016-05-02T14:27:31Z

ok jenkins, [test] again.

openshift-bot · 2016-05-02T14:30:22Z

Evaluated for origin test up to bd47008

openshift-bot · 2016-05-02T15:30:21Z

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

mfojtik · 2016-05-02T15:31:56Z

[merge]

(tests are flaking but nothing related to this PR)

openshift-bot · 2016-05-02T15:35:22Z

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

openshift-bot · 2016-05-02T15:35:23Z

Evaluated for origin merge up to bd47008

smarterclayton · 2016-05-02T16:03:57Z

Are all the flakes known?

On May 2, 2016, at 11:32 AM, Michal Fojtik [email protected] wrote:

[merge]

(tests are flaking but nothing related to this PR)

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

mfojtik · 2016-05-02T16:32:59Z

Yes as far I know (s2i quota, mysql passtrough)
On May 2, 2016 18:04, "Clayton Coleman" [email protected] wrote:

Are all the flakes known?

On May 2, 2016, at 11:32 AM, Michal Fojtik [email protected]
wrote:

[merge]

(tests are flaking but nothing related to this PR)

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

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

mfojtik force-pushed the serial-builds branch from 3cfad6d to 0377afa Compare April 11, 2016 12:00

mfojtik reviewed Apr 11, 2016
View reviewed changes

mfojtik force-pushed the serial-builds branch 3 times, most recently from 8fa4671 to e22db83 Compare April 11, 2016 17:30

bparees reviewed Apr 11, 2016
View reviewed changes

mfojtik changed the title ~~WIP: Initial API draft for serial builds~~ WIP: Initial implementation of serial builds Apr 12, 2016

mfojtik force-pushed the serial-builds branch 2 times, most recently from 978f35b to bacdafb Compare April 12, 2016 10:12

mfojtik force-pushed the serial-builds branch from bacdafb to fae2e18 Compare April 12, 2016 13:34

smarterclayton reviewed Apr 12, 2016
View reviewed changes

bparees reviewed Apr 12, 2016
View reviewed changes

mfojtik reviewed Apr 13, 2016
View reviewed changes

mfojtik added this to the 1.2.x milestone Apr 13, 2016

mfojtik changed the title ~~WIP: Initial implementation of serial builds~~ Initial implementation of serial builds Apr 13, 2016

mfojtik force-pushed the serial-builds branch from be72d42 to 30b679c Compare April 13, 2016 14:15

mfojtik force-pushed the serial-builds branch from 0737352 to 539694d Compare April 28, 2016 10:57

mfojtik force-pushed the serial-builds branch 2 times, most recently from 181125a to d3c2897 Compare April 28, 2016 13:00

smarterclayton added api-approved and removed needs-api-review labels Apr 29, 2016

mfojtik force-pushed the serial-builds branch from d3c2897 to fe2ba14 Compare May 2, 2016 08:13

mfojtik force-pushed the serial-builds branch from fe2ba14 to 160e9cf Compare May 2, 2016 10:29

Allow to specify the run policy for builds

bd47008

mfojtik force-pushed the serial-builds branch from 160e9cf to bd47008 Compare May 2, 2016 11:58

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

mfojtik deleted the serial-builds branch September 5, 2018 21:08

Allow to specify runPolicy for BuildConfig #8453

Allow to specify runPolicy for BuildConfig #8453

Conversation

mfojtik commented Apr 11, 2016 • edited Loading

mfojtik commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Apr 12, 2016

mfojtik commented Apr 12, 2016

bparees commented Apr 12, 2016

mfojtik commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Apr 12, 2016

mfojtik commented Apr 12, 2016

smarterclayton commented Apr 12, 2016

Choose a reason for hiding this comment

bparees commented Apr 12, 2016

mfojtik commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Apr 28, 2016

mfojtik commented Apr 29, 2016

mfojtik commented Apr 29, 2016

jimmidyson commented Apr 29, 2016

bparees commented Apr 29, 2016

smarterclayton commented Apr 29, 2016

bparees commented Apr 29, 2016

mfojtik commented May 2, 2016

mfojtik commented May 2, 2016

openshift-bot commented May 2, 2016

openshift-bot commented May 2, 2016

mfojtik commented May 2, 2016

openshift-bot commented May 2, 2016

openshift-bot commented May 2, 2016

mfojtik commented May 2, 2016

openshift-bot commented May 2, 2016 • edited Loading

openshift-bot commented May 2, 2016

smarterclayton commented May 2, 2016

mfojtik commented May 2, 2016

mfojtik commented Apr 11, 2016 •

edited

Loading

openshift-bot commented May 2, 2016 •

edited

Loading