Details

Reviewers

Group Reviewers

Commits

Restricted Diffusion Commit
rPcad41ea2947e: Implement build simulation; convert Harbormaster to be purely dependency based

Summary

Depends on D9806. This implements the build simulator, which is used to calculate the order of build steps in the plan editor. This includes a migration script to convert existing plans from sequential based to dependency based, and then drops the sequence column.

Because build plans are now dependency based, the grippable and re-order behaviour has been removed.

Test Plan

Tested the migration, saw the dependencies appear correctly.

Diff Detail

Repository

rP Phabricator

Branch

harbormaster-dependency-graph

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1770
Build 1771: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que updated this revision to Diff 23612.Jul 6 2014, 9:03 AM

hach-que retitled this revision from to Implement build simulation; convert Harbormaster to be purely dependency based.

hach-que updated this object.

hach-que edited the test plan for this revision. (Show Details)

hach-que added a reviewer: epriestley.

hach-que added a parent revision: D9806: Implement artifact dependencies and parallisation in Harbormaster.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 6 2014, 9:03 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B1550: Diff 23612.Jul 6 2014, 9:04 AM

This mostly looks sensible, but the simulate stuff seems really messy. Is there a reason this can't use something that subclasses AbstractDirectedGraph, pass it all the dependencies, and then let it detect cycles? This is how we detect cycles in all other graphs.

resources/sql/autopatches/20140706.harbormasterdepend.1.php
42	Because of how migrations work, this isn't safe. This migration may be getting run on some future version of the code which adds new columns, and `save()` will try to update them. Instead, you have to update the column specifically: UPDATE %T SET details = %s WHERE id = %d
resources/sql/autopatches/20140706.harbormasterdepend.2.sql
1–2 ↗	(On Diff #23612)	We should probably do this later in case installs run into trouble, so we aren't destroying any data.
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
140	This won't work well for 27+ parallel steps.

Update based on feedback

hach-que added inline comments.Jul 17 2014, 3:16 AM

resources/sql/autopatches/20140706.harbormasterdepend.1.php
46	I am assuming this does what I intend it to.

Harbormaster completed remote builds in B1733: Diff 23893.Jul 17 2014, 3:17 AM

hach-que added inline comments.Jul 17 2014, 3:17 AM

src/applications/harbormaster/engine/HarbormasterBuildEngine.php
158–179	I can revert this refactoring too if you want.

This now depends on D9956 since AbstractDirectedGraph lives in libphutil.

Use new dependency graph logic

Harbormaster completed remote builds in B1770: Diff 23982.Jul 18 2014, 3:38 AM

Merged in other diff

Harbormaster completed remote builds in B1771: Diff 23983.Jul 18 2014, 3:48 AM

Use new modular data source

Harbormaster completed remote builds in B1772: Diff 23984.Jul 18 2014, 4:07 AM

Rebase onto master

Harbormaster completed remote builds in B1928: Diff 24225.Jul 30 2014, 12:49 AM

For consistency with modern code, let's spell depends_on as dependsOn instead.

The only real thing I'm still not sure about here is exposing dependencies as implicit artifiacts. This seems like it increases complexity, and it would be simpler to have artifacts generate implicit dependencies instead. As far as I can tell, this diff also doesn't actually cause artifacts to create dependencies -- you mentioned doing that in a future diff, I think?

It seems like we could just make getDependencies() return the explicit dependencies (from dependsOn) combined with the implicit dependencies (by examining input artifacts) and we (a) wouldn't have to convert dependencies into pseudo-artifacts and (b) would get artifacts-imply-dependencies for cheap. Am I missing something?

resources/sql/autopatches/20140706.harbormasterdepend.1.php
53–54	Instead of printing out a sequence number, maybe just print out the step ID? That's probably a bit more useful / unambiguous.
src/applications/harbormaster/controller/HarbormasterPlanViewController.php
413	It seems like this could just be `$depends_on`, without needing to wrap them in an artifact-like shell. We aren't sharing any code by making these look like they're artifacts.
437–445	...then this would be impossible.
src/applications/harbormaster/controller/HarbormasterStepEditController.php
78	Do these objects not support normal handle loads? Is the reasoning that you don't want to show steps from other plans? Maybe doing a normal load (adding the ability to do a normal load first, if necessary) and then doing a cleanup pass to color/mark the bad ones would be cleaner?
155–159	This looks unused.
src/applications/harbormaster/engine/HarbormasterBuildEngine.php
175	Splitting this function like this makes sense, but the name is a little surprising, since it also changes the build state, rather than just selecting steps. I don't have a better name offhand which isn't super generic, though -- `updateRunState()`? Pretty meh.
295–296	Maybe remove "yet" since the database can now represent deadlocking plans.
src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
101–112	Could this just return a list of PHIDs, by combining explicit dependencies with the steps that generate artifacts the build uses? It's not clear why we're making dependencies look like artifacts instead of making artifacts look like dependencies.
114–118	(This has no callsites that I can see, and I'm not sure what the goal is.)
164–166	We should be able to, shouldn't we? That is, if I add steps A and B, it's OK to edit A and make it depend on something in B. That will change execution order, but I clearly intend/expect that if I'm generating something in B and then saying A uses it. Otherwise I'd have to do something like add an explicit dependency, then edit the thing again and add the artifact? We should only exclude artifacts generated by steps which already depend on this one (but ideally we should still show them in the UI, just with a clear explanation about why they can't be used).
src/applications/harbormaster/typeahead/HarbormasterBuildDependencyDatasource.php
11 ↗	(On Diff #24225)	This is now `PhabricatorHarbormasterApplication` in HEAD.
20–23 ↗	(On Diff #24225)	(Do we need to do this? Don't steps automatically load the plan to do policy checks?)

This revision now requires changes to proceed.Jul 30 2014, 9:47 PM

Primarily the reason for storing them as artifacts was that to me, there's not a particularly large difference between external dependencies ("I depend on a file") and internal dependencies ("I depend on the build being in this state"). They're both dependencies, and currently we represent the former with artifacts. Having a secondary system to represent internal dependencies seems redundant.

resources/sql/autopatches/20140706.harbormasterdepend.1.php
53–54	Will do.
src/applications/harbormaster/controller/HarbormasterStepEditController.php
78	Oh, I can use `loadHandles` or w/e to load these in? Originally when developing this I think I was trying to pass in HarbormasterBuildStep into v_depends_on and the typeahead wasn't rendering what I was expecting, so I just constructed the input manually.
155–159	It's probably left over from the diff merge, where the first diff still checked the order to make sure dependencies existed. I'll remove it.
src/applications/harbormaster/engine/HarbormasterBuildEngine.php
175	I can pretty much revert this split I think? (There are still some changes in the function itself that need to be kept) This was originally used by the build simulator, but we changed that to use the abstract graph, so this is no longer needed.
src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
164–166	I think this might have been left over from the manual deadlock detection. Really there's no reason to not just return all of the artifacts in the entire build plan here, since we detect deadlocks in the UI and warn the user if they've created a deadlock.
src/applications/harbormaster/typeahead/HarbormasterBuildDependencyDatasource.php
20–23 ↗	(On Diff #24225)	Oh yeah, this can probably be removed.

I'll make dependencies separate from artifacts and we'll see what that looks like.

Changes requested in feedback

Harbormaster completed remote builds in B1949: Diff 24257.Jul 31 2014, 12:13 AM

Remove getFullArtifactOutputs

Harbormaster completed remote builds in B1950: Diff 24259.Jul 31 2014, 12:47 AM

epriestley added inline comments.Jul 31 2014, 12:49 AM

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
102	Can we reasonably include the implicit dependencies from artifacts in this diff, or did you want to wait on that?
134–152	These two functions are now exactly the same, and I think `get...()` has no callers except `load...()`.

hach-que added inline comments.Jul 31 2014, 12:53 AM

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
102	I wanted to do that separately since this diff is already big enough.

Remove loadAvailableArtifacts

Harbormaster completed remote builds in B1951: Diff 24260.Jul 31 2014, 12:58 AM

Found a couple issues in local testing:

Fatal on empty plans because of bad return type, see inline.
Fatal when saving a new step with strict tables, #1364: Field 'sequence' doesn't have a default value. Give protected $sequence a default = 0; in HarbormasterBuildStep. We can remove it when we drop the column later.

Also:

I can't get steps to show any level of parallelism in the UI? They seem to be popping out of the graph fine, but I think the UI is a little off. Some inlines, I think this is just some weird rendering logic.

src/applications/harbormaster/controller/HarbormasterPlanViewController.php
117	I think this is wrong, and it should be the other way around?
118	And otherwise we increment $i, but then reset it when changing depths? And then swap $i and $depth in the display strings? Not exactly sure.
src/applications/harbormaster/engine/HarbormasterBuildGraph.php
22–24	This can be expressed more simply as `if (!$steps)`. The return value has the wrong type and fatals on empty build plans. It should just `return array()`.
src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php
12	This has to be retained with `= 0;` until we drop the column.

Fixed fatals and incorrect step rendering

Harbormaster completed remote builds in B1952: Diff 24261.Jul 31 2014, 1:17 AM

epriestley accepted this revision.Jul 31 2014, 1:37 AM

epriestley edited edge metadata.

This revision is now accepted and ready to land.Jul 31 2014, 1:37 AM

Closed by commit rPcad41ea2947e (authored by @hach-que).

Implement build simulation; convert Harbormaster to be purely dependency based
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23982

resources/sql/autopatches/20140706.harbormasterdepend.1.php

src/__phutil_library_map__.php

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

src/applications/harbormaster/controller/HarbormasterPlanOrderController.php

src/applications/harbormaster/controller/HarbormasterPlanViewController.php

src/applications/harbormaster/controller/HarbormasterStepEditController.php

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

src/applications/harbormaster/engine/HarbormasterBuildGraph.php

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php

src/applications/harbormaster/storage/build/HarbormasterBuild.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

src/applications/typeahead/controller/PhabricatorTypeaheadStepDependenciesDatasourceController.php

webroot/rsrc/js/application/harbormaster/behavior-reorder-steps.js

Implement build simulation; convert Harbormaster to be purely dependency basedClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23982

resources/sql/autopatches/20140706.harbormasterdepend.1.php

src/__phutil_library_map__.php

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

src/applications/harbormaster/controller/HarbormasterPlanOrderController.php

src/applications/harbormaster/controller/HarbormasterPlanViewController.php

src/applications/harbormaster/controller/HarbormasterStepEditController.php

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

src/applications/harbormaster/engine/HarbormasterBuildGraph.php

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php

src/applications/harbormaster/storage/build/HarbormasterBuild.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

src/applications/typeahead/controller/PhabricatorTypeaheadStepDependenciesDatasourceController.php

webroot/rsrc/js/application/harbormaster/behavior-reorder-steps.js

Implement build simulation; convert Harbormaster to be purely dependency based
ClosedPublic
Actions

Revision Contents
Changeset List