Page MenuHomePhabricator

[harbormaster/run-build-plan] Implement build step for running another build plan
AbandonedPublic

Authored by hach-que on Jul 3 2014, 7:11 AM.
Referenced Files
Unknown Object (File)
Sat, Mar 23, 7:08 PM
Unknown Object (File)
Thu, Mar 21, 10:40 AM
Unknown Object (File)
Fri, Mar 15, 4:51 AM
Unknown Object (File)
Fri, Mar 15, 4:03 AM
Unknown Object (File)
Feb 17 2024, 4:23 AM
Unknown Object (File)
Feb 14 2024, 8:49 AM
Unknown Object (File)
Feb 4 2024, 4:22 AM
Unknown Object (File)
Dec 25 2023, 9:47 AM
Tokens
"Love" token, awarded by mikn.

Details

Summary

This implements a Harbormaster build step which runs another build plan on the current buildable and waits for it to finish. It has the following semantics:

  • If the buildable does not have a build with the specified build plan, it starts a build and waits for it to complete.
  • If the buildable does have a build, but it's in a failed or error status, it restarts that build and waits for it to complete.
  • If the buildable does have a build, but it's in a passed status, it returns immediately.
  • If the buildable does have a build, but it's currently building or stopped, it waits for it to complete.

"complete" here means moving to either a pass or fail status.

This is primarily useful with D9806, where a build plan can run a bunch of other build plans in parallel and wait for them to complete. It also resolves T5542 by making it redundant. If you need a part of your build process to be restartable (without performing prior steps), you can split the top level build plan into other build plans, and it won't repeat the ones that have already passed (but you can still manually restart those build plans from the UI).

Test Plan

Ran it with Plan A running Plan B, with Plan B having a Throw Exception step and saw it fail as expected. Removed the Throw Exception step and restarted Plan A, and saw Plan B get restarted as expected, followed by Plan B passing then Plan A passing.

Diff Detail

Repository
rP Phabricator
Branch
arcpatch-D9807_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2100
Build 2104: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que retitled this revision from to Implement build step for running another build plan.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
39

This makes the plan view controller a bit nicer (without this, the plan name isn't actually shown anywhere on the page).

Is this warning bugged because I'm using braces in the case statement? It doesn't occur for the other fallthrough statement at the end of the build step.

The braces aren't necessary and probably cause the message.

hach-que edited edge metadata.
  • Implement wait-only switch

Remove wait-only; this wasn't very useful since it can only apply to the current buildable

Use PhabricatorHarbormasterApplication

Emit URI artifacts for the target build and plan

Use the plan directly, because sometimes it isn't attached. Append build target ID to the artifact name to avoid conflicts.

Use HarbormasterBuildFailureException on build failure

@epriestley I don't suppose you'll have any time to review this soon? We do have it patched, but I think this is probably useful for other people with complex build processes, so it'd be nice to have it in upstream.

Split unrelated UI / UX change into D13356

hach-que retitled this revision from Implement build step for running another build plan to [harbormaster/run-build-plan] Implement build step for running another build plan.Jul 1 2015, 4:46 AM
epriestley edited edge metadata.

These semantics don't make any sense to me for a "Run another plan" step. They mean that all builds from the same build plan are equivalent. This is probably mostly-true today, but not entirely, and definitely won't be true in the future: they may have different parameters. I think the semantics should always be "start a new build". If plan X says to run plan Y 5 times, plan Y should run 5 times.

I think you're solving some more complicated problem here, but that "run build" should always run a build. Some version of this might be reasonable as "generate artifact if not already generated", but that needs more infrastructure in place first. As written, this plan is not appropriate for subdividing work, because if you have a build plan like "make" and run two builds on different hosts, only one of them will actually build the binary. This is unintuitive. We should have "run build" first, which does what it says, and once that actually works we can have "generate or wait for artifact".

We can not put while(true) { sleep(...); } loops in build plans. These can lock the queue. Build plans which need to wait must yield, like HarbormasterWaitForPreviousBuildStepImplementation does.

I'm not really sure this is the right solution to any problem. Presumably you're doing something like:

  • Build 1
  • Run or wait for "make" artifact
  • Run lint

Then:

  • Build 2
  • Run or wait for "make" artifact
  • Run unit

But this pattern won't work if you want to do this across two hosts, since the "make" artifact on one host will conflict with the "make" artifact on the other host. I'd expect this to be structured as:

  • Build 1
  • Run "make"
  • Run unit (artifacts = make)
  • Run lint (artifacts = make)

Then you can run as many copies of that as you want without issues.

Basically, this looks like a hack to solve a very specific problem, but the general version of that problem requires solving a number of larger problems (sub-builds, artifact scope, parameterized builds, parallelism [now solved or better-solved in HEAD?], partial restarts). This kind-of fixes all those problems in your environment, but isn't nearly general enough for the upstream. I suspect almost no other builds outside of your environment could actually use this step.

I'll accept a version of this which actually runs another plan, but I don't think that does much to solve the problem you encountered on its own.

This revision now requires changes to proceed.Aug 8 2015, 5:37 PM

I believe these concerns are mitigated by D13635.

The semantics of this build step is also "Run build plan", not "Embed build plan" which sounds closer to what you are describing. When build plans run, they are isolated in terms of artifacts; the artifacts of a second build plan can not be using the artifacts of the first build plan, if the second build plan is independently restartable. This is because if the build using the first build plan fails, and the host artifact is cleaned up, the second build using the second build plan can not be restarted.

With "Run build plan", artifacts remain independent (but D13635 allows plans to be parameterized which solves the general case). "Embed build plan" would copy the build steps from the target plan as targets into the current build, allowing artifacts to be shared, but at the cost of the embedded build plan not be independently restartable.

hach-que edited edge metadata.

Rebase on origin/master

I still use this and continuously rebase it on the latest version of Phabricator, but I don't plan on doing the work to implement "Embed build plan" with artifact expiries or anything like that since it's a significant amount of work and since Harbormaster isn't a focus for upstream right now (based on the feed), it's unlikely any work in this area would be reviewed for quite some time anyway.