1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 08:12:40 +01:00
Commit graph

274 commits

Author SHA1 Message Date
epriestley
6ebe8db380 Add a missing key to HarbormasterBuildArtifact
Summary: Fixes T10192. This key improves some common queries and is not currently present.

Test Plan: See discussion in T10192. Verified current query plan of real queries is garbage and improved by adding this key.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10192

Differential Revision: https://secure.phabricator.com/D15075
2016-01-21 10:21:19 -08:00
epriestley
5c2e49a812 Allow any user to watch any project they can see
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.

Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.

Test Plan:
  - Watched a project I was not a member of.
  - Clicked the feed watch/unwatch button.

{F1064909}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6183, T10054

Differential Revision: https://secure.phabricator.com/D15063
2016-01-19 19:38:30 -08:00
epriestley
bcfd6bdd81 Move various other callsites away from callsigns
Summary: Ref T4245. These mostly relate to building URIs.

Test Plan: Tried to hunt down as many of these in the UI as I could. Some are a bit tricky but they should be low-risk.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14933
2016-01-04 06:54:42 -08:00
epriestley
37532a0bf0 Remove various additional calls to getCallsign()
Summary: Ref T4245. These are all straightforward to remove.

Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14927
2016-01-02 11:04:22 -08:00
epriestley
2868a69f65 Remove all setObjectURI() from ActionListViews
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.

(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)

Test Plan:
  - As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
  - Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T10004

Differential Revision: https://secure.phabricator.com/D14805
2015-12-17 08:30:22 -08:00
epriestley
39cf013472 Add objectPHID keys to Harbormaster task schedulers
Summary:
Fixes T9816. It's currently hard to hunt down some particulars in the worker queue if things go awry in Harbormaster.

Supplement the queue with `objectPHID` keys so we can hunt tasks down more easily if the issues in T9816 continue.

Test Plan:
```
mysql> select * from worker_archivetask order by id desc limit 30;
+--------+------------------------------------------------+-----------------------------------+--------------+--------------+--------+--------+----------+-------------+--------------+----------+--------------------------------+
| id     | taskClass                                      | leaseOwner                        | leaseExpires | failureCount | dataID | result | duration | dateCreated | dateModified | priority | objectPHID                     |
+--------+------------------------------------------------+-----------------------------------+--------------+--------------+--------+--------+----------+-------------+--------------+----------+--------------------------------+
| 496024 | HarbormasterTargetWorker                       | 8514:1448232248:Orbital.local:3   |   1448318648 |            0 | 311880 |      0 |   233758 |  1448232248 |   1448232248 |     2000 | PHID-HMBT-thq4oof4byllmbc4q3tt |
| 496023 | PhabricatorApplicationTransactionPublishWorker | 8514:1448232247:Orbital.local:1   |   1448239447 |            0 | 311879 |      0 |    53731 |  1448232247 |   1448232247 |     1000 | PHID-HMBD-i6zo2ltc73rre7o54s7v |
| 496022 | HarbormasterBuildWorker                        | 8514:1448232247:Orbital.local:2   |   1448239447 |            0 | 311878 |      0 |    30736 |  1448232248 |   1448232248 |     2000 | PHID-HMBD-i6zo2ltc73rre7o54s7v |
...
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9816

Differential Revision: https://secure.phabricator.com/D14541
2015-11-23 05:58:54 -08:00
Aviv Eyal
1898864b6c add initiator.phid parameter to HM builds
Summary:
Fix T9662.

Record who initiated the build, and allow this information as a parameter.

In this implementation, a 're-run' keeps the original initiator, which we maybe not desired?

Test Plan:
Make a HTTP step with initiator.phid, trigger manually, via HM, via ./bin/harbormaster build.
Look at requests made.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T9662

Differential Revision: https://secure.phabricator.com/D14380
2015-11-04 18:32:18 +00:00
Joshua Spence
c35b564f4d Various translation improvements
Summary: Depends on D14070.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D14073
2015-11-03 07:02:46 +11:00
Joshua Spence
495cb7a2e0 Mark PhabricatorPHIDType::getPHIDTypeApplicationClass() as abstract
Summary: Fixes T9625. As explained in a `TODO` comment, seems reasonable enough.

Test Plan: Unit tests.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, hach-que

Maniphest Tasks: T9625

Differential Revision: https://secure.phabricator.com/D14068
2015-11-03 06:47:12 +11:00
Joshua Spence
98a301a59b Set $can_edit for Harbormaster steps
Summary: Sets the `$can_edit` value correctly (previously it was hardcoded to `true`).

Test Plan: Went to http://phabricator.local/harbormaster/step/view/1/ and saw "Edit Step" disabled.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14373
2015-10-31 04:54:16 +00:00
epriestley
f48a833704 Fix an issue with incorrect authorization handling in Working Copy build steps
Summary:
Fixes T9669. Two issues:

  - We were using `repositoryPHIDs` instead of `blueprintPHIDs` for the list of allowed blueprints. Use the correct value.
  - We weren't enforcing `allowedBlueprintPHIDs` fully correctly. We //did// require an authorization, so the net effect was correct in nearly all cases, but we could have selected from too large a pool in the case where the application itself was doing the authorization (e.g., from the command line).

Test Plan: Ran a build through Drydock/Harbormaster locally.

Reviewers: chad, tycho.tatitscheff

Reviewed By: chad, tycho.tatitscheff

Subscribers: tycho.tatitscheff

Maniphest Tasks: T9669

Differential Revision: https://secure.phabricator.com/D14368
2015-10-30 16:02:35 +00:00
epriestley
1b8337871b Correct the handle URI for build steps
Summary: Fixes T9674. This was wrong to start with (URI is `/edit/X/`, not `/X/edit/`) but we have a new view page anyway.

Test Plan:
  - Visited an exmaple URI in my browser.
  - Followed a build step link from "Authorized By: ..." in Drydock.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T9674

Differential Revision: https://secure.phabricator.com/D14366
2015-10-30 15:54:10 +00:00
epriestley
c059149eb9 Remove Drydock host resource limits and give working copies simple limits
Summary:
Ref T9252. Right now, we have very strict limits on Drydock: one lease per host, and one working copy per working copy blueprint.

These are silly and getting in the way of using "Land Revision" more widely, since we need at least one working copy for each landable repository.

For now, just remove the host limit and put a simple limit on working copies. This might need to be fancier some day (e.g., limit working copies per-host) but it is generally reasonable for the use cases of today.

Also add a `--background` flag to make testing a little easier.

(Limits are also less important nowadays than they were in the past, because pools expand slowly now and we seem to have stamped out all the "runaway train" bugs where allocators go crazy and allocate a million things.)

Test Plan:
  - With a limit of 5, ran 10 concurrent builds and saw them finish after allocating 5 total resources.
  - Removed limit, raised taskmaster concurrency to 128, ran thousands of builds in blocks of 128 or 256.
    - Saw Drydock gradually expand the pool, allocating a few more working copies at first and a lot of working copies later.
    - Got ~256 builds in ~140 seconds, which isn't a breakneck pace or anything but isn't too bad.
    - This stuff seems to be mostly bottlenecked on `sbuild` throttling inbound SSH connections. I haven't tweaked it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14334
2015-10-26 12:39:47 -07:00
epriestley
3f193cb9e0 Give Harbormaster build steps a "View" page
Summary:
Fixes T9519. Right now, build steps go straight from the build to the edit screen.

This means that there's no way to see their edit history or review details without edit permission. In particular, this makes it a bit harder to catch the Drydock Blueprint authorization warnings from T9519.

  - Add a standard view screen.
  - Add a little warning callout to blueprint authorizations.

This also does a bit of a touchup on the weird dropshadow element from T9586. Maybe not totally design-approved now but it's less ugly, at least.

Test Plan:
{F906695}

{F906696}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9519

Differential Revision: https://secure.phabricator.com/D14330
2015-10-26 12:38:32 -07:00
epriestley
5ee4a1a306 Give Harbormaster Build Plans real policies
Summary:
Ref T9614. Currently, a lot of Build Plan behavior is covered by a global "can manage" policy.

One install in particular is experiencing difficulty with warring factions within engineering aborting one another's builds.

As a first step to remedy this, and also generally make Harbormaster more flexible and bring it in line with other applications in terms of policy power:

  - Give Build Plans normal view/edit policies.
  - Require "Can Edit" to run a plan manually.

Having "Can View" on plans may be a little weird in some cases (the status of a Buildable might be bad because of a build you can't see) but we can cross that bridge when we come to it.

Next change here will require "Can Edit" to abort a build. This will reasonably allow installs to reserve pause/abort for administrators/adults. (I might let anyone restart a plan, though?)

Test Plan:
  - Created a new build plan.
  - Verified defaults were inherited from application defaults (swapped them around, too).
  - Saved build plan.
  - Edited policies.
  - Verified autoplans get the right policies.
  - Verified old plans got migrated properly.
  - Tried to run a plan I couldn't edit (denied).
  - Ran a plan from CLI with `bin/harbormaster`.
  - Tried to create a plan with an unprivileged user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9614

Differential Revision: https://secure.phabricator.com/D14321
2015-10-26 12:38:21 -07:00
epriestley
43569d4e27 Make WorkingCopy build step slightly more durable
Summary: Fixes T9631. Build steps created before I added this option may not have it specified, which could throw later. Make handling a little more robust.

Test Plan: Will ask @yelirekim to report back.

Reviewers: chad

Reviewed By: chad

Subscribers: yelirekim

Maniphest Tasks: T9631

Differential Revision: https://secure.phabricator.com/D14336
2015-10-25 14:53:24 -07:00
epriestley
1bdf225354 Use Drydock authorizations when acquiring leases
Summary:
Ref T9519. When acquiring leases on resources:

  - Only consider resources created by authorized blueprints.
  - Only consider authorized blueprints when creating new resources.
  - Fail with a tailored error if no blueprints are allowed.
  - Fail with a tailored error if missing authorizations are causing acquisition failure.

One somewhat-substantial issue with this is that it's pretty hard to figure out from the Harbormaster side. Specifically, the Build step UI does not show field value anywhere, so the presence of unapproved blueprints is not communicated. This is much more clear in Drydock. I'll plan to address this in future changes to Harbormaster, since there are other related/similar issues anyway.

Test Plan: {F872527}

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9519

Differential Revision: https://secure.phabricator.com/D14254
2015-10-12 17:02:35 -07:00
Chad Little
02f42628c3 Update Harbormaster for handleRequest
Summary: Updates Harbormaster for handleRequest over processRequest

Test Plan: Went through various Harbormaster areas, buildables, actions.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14255
2015-10-12 11:39:01 -07:00
epriestley
4d5278af11 Put Drydock build steps into their own group in Harbormaster
Summary: Ref T9252. Move these into a new "Drydock" group.

Test Plan: Clicked "Add Build Step", saw Drydock steps in a Drydock group.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14237
2015-10-05 15:59:35 -07:00
epriestley
4cf1270ecd In Harbormaster, make sure artifacts are destroyed even if a build is aborted
Summary:
Ref T9252. Currently, Harbormaster and Drydock work like this in some cases:

  # Queue a lease for activation.
  # Then, a little later, save the lease PHID somewhere.
  # When the target/resource is destroyed, destroy the lease.

However, something can happen between (1) and (2). In Drydock this window is very short and the "something" would have to be a lighting strike or something similar, but in Harbormaster we wait until the resource activates to do (2) so the window can be many minutes long. In particular, a user can use "Abort Build" during those many minutes.

If they do, the target is destroyed but it doesn't yet have a record of the artifact, so the artifact isn't cleaned up.

Make these things work like this instead:

  # Create a new lease and pre-generate a PHID for it.
  # Save that PHID as something that needs to be cleaned up.
  # Queue the lease for activation.
  # When the target/resource is destroyed, destroy the lease if it exists.

This makes sure there's no step in the process where we might lose track of a lease/resource.

Also, clean up and standardize some other stuff I hit.

Test Plan:
  - Stopped daemons.
  - Restarted a build in Harbormaster.
  - Stepped through the build one stage at a time using `bin/worker execute ...`.
  - After the lease was queued, but before it activated, aborted the build.
  - Processed the Harbormaster side of things only.
  - Saw the lease get destroyed properly.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14234
2015-10-05 05:58:53 -07:00
epriestley
14d6325394 Acccept any HTTP 2xx status as success in Harbormaster
Summary: Ref T9478. This should probably be configurable eventually, but for now treat any 200-block status as success. Also show the result code.

Test Plan:
  - Hit a bad URI, saw "HTTP 503" + failure.
  - Hit a good URI, saw "HTTP 200" + success.

Reviewers: chad, hach-que

Reviewed By: chad, hach-que

Maniphest Tasks: T9478

Differential Revision: https://secure.phabricator.com/D14226
2015-10-02 09:17:51 -07:00
epriestley
bb4667cb84 Fix WorkingCopy step to read correct commit variables
Summary: Ref T9252. This variable was always wrong but we fell back to just resetting to `HEAD` before. Use the correct variable name.

Test Plan: Verified variable name.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14224
2015-10-02 06:37:17 -07:00
epriestley
2728a9f964 Allow builds to have parameters
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).

Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9352

Differential Revision: https://secure.phabricator.com/D14222
2015-10-02 06:32:08 -07:00
epriestley
e431ab2189 Use getPhobjectClassConstant() to access class constants
Summary: Ref T9494. Depends on D14216. Remove 10 copies of this code.

Test Plan: Ran `arc unit --everything`, browsed Config > Modules, clicked around Herald / etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9494

Differential Revision: https://secure.phabricator.com/D14217
2015-10-01 16:56:21 -07:00
epriestley
4496176924 Add staging area support to Harbormaster/Drydock + various fixes
Summary:
Ref T9252. This primarily allows Harbormaster to request (and Drydock to fulfill) working copies with a patch from a staging area. Doing this means we can do builds on in-review changes from `arc diff`.

This is a little cobbled-together but should basically work.

Also fix some other issues:

  - Yielded, awakend workers are fine to update but could complain.
  - We can't log slot lock failures to resources if we don't end up saving them.
  - Killing the transaction would wipe out the log.
  - Fix some TODOs, etc.

Test Plan: Ran Harbormaster builds on a local revision.

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14214
2015-10-01 16:55:01 -07:00
epriestley
4ac82be5ed Merge the DrydockLease workers into a single worker
Summary:
Ref T9252. This is the same as D14201, but for lease stuff instead of resource stuff.

This one is a little heavier but still feels pretty reasonable to me at the end of the day (worker is <1K lines and has a ton of comment stuff).

Also fixes a few random bugs I hit in the task queue.

Test Plan:
  - Restarted some Harbormaster builds, saw them go through cleanly.
  - Released pre-activation resources/leases.
  - Probably still kinda buggy but I'll iron the details out over time.

Logs are starting to look somewhat plausible:

{F855747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14202
2015-10-01 08:11:02 -07:00
epriestley
a5c4177160 Fix an issue with BuildLogs and web UI during builds
Summary:
Ref T9123. After recent changes, viewing a live build log from the web UI throws a CSRF exception.

Check `start` ("this object is an active log"), not `live` ("this log is open somewhere").

Test Plan: Will push / etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9123

Differential Revision: https://secure.phabricator.com/D14185
2015-09-28 19:11:51 -07:00
epriestley
bfaa93aa9b Allow Harbormaster build plans to request additional working copies
Summary:
Ref T9123. To run upstream builds in Harbormaster/Drydock, we need to be able to check out `libphutil`, `arcanist` and `phabricator` next to one another.

This adds an "Also Clone: ..." field to Harbormaster working copy build steps so I can type all three repos into it and get a proper clone with everything we need.

This is somewhat upstream-centric and a bit narrow, but I don't think it's totally unreasonable, and most of the underlying stuff is relatively general.

This adds some more typechecking and improves data/type handling for custom fields, too. In particular, it prevents users from entering an invalid/restricted value in a field (for example, you can't "Also Clone" a repository you don't have permission to see).

Test Plan: Restarted build, got a Drydock resource with multiple repositories in it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9123

Differential Revision: https://secure.phabricator.com/D14183
2015-09-28 17:57:41 -07:00
epriestley
33be8f719f Allow WorkingCopy resources to have multiple working copies
Summary:
Ref T9252. For building Phabricator itself, we need to have `libphutil/`, `arcanist/` and `phabricator/` next to one another on disk.

Expand the Drydock WorkingCopy resource so that it can have multiple repositories if the caller needs them.

I'm not sure if I'm going to put the actual config for this in Harbormaster or Drydock yet, but the WorkingCopy resource itself should work the same way in either case.

Test Plan: Restarted a Harbormaster build which leases a working copy, saw it build as expected.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14180
2015-09-28 09:35:58 -07:00
epriestley
9b29d46e60 Make Drydock lease infrastructure more nimble
Summary:
Ref T9252. Currently, Harbormaster does this when trying to acquire a working copy:

  - Ask for a working copy.
  - Yield for 15 seconds.
  - Check if we have a working copy yet.

That's OK, but Drydock takes ~1s to acquire a working copy lease if a resource is already available, so we end up doing this:

  - T+0: Ask for a working copy.
  - T+0: Yield for 15 seconds.
  - T+1: Working copy lease activates.
  - T+15: Working copy lease is used.
  - T+16: Build finishes.

So we end up spending about 2 seconds doing work and 14 seconds sleeping.

One way to fix this would be to fiddle with the yield duration, so we yield for 1, 2, 4, ... seconds or something. This probably isn't a bad idea for longer leases (i.e., wait for 15, 30, 45 ... seconds or similar) but it implies a lot of churn for short leases.

Instead, let tasks "awaken" other tasks when they complete. The "awaken" operation means: if a task is in a yielded state (no failures, no owner, explicitly yielded, future expires time), pretend it only yielded until right now instead of whenever it really yielded to.

Basically, this rewrites history so that even though Harbormaster did a `yield(15)`, we pretend it did a `yield(4)` after we activate the lease if lease activation took 4 seconds.

If this misses, it's fine: we fall back to the normal yield behavior and things move forward normally a few seconds later.

If it hits, we get a more nimble process pretty cleanly.

Test Plan:
  - Restarted a build plan (lease working copy + run `ls`) with this patch no-op'd, took about 16 seconds.
  - Restarted a build plan with this patch active, took about 1 second.

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14178
2015-09-28 09:35:40 -07:00
epriestley
d735c7adf2 Allow Harbormaster to run commands on Drydock working copies
Summary: Ref T9252. This mostly cleans up future and log handling, and edges us closer to being able to do useful work with Harbormaster / Drydock.

Test Plan:
  - Added a "Run `ls -alh`" step to my trivial build plan.
  - Ran it a bunch of times.
  - Worked great.
  - Also did an HTTP plan.

{F835227}

{F835228}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14161
2015-09-25 10:43:32 -07:00
epriestley
8ce90a7c42 Allow lint codes to be up to 128 bytes long
Summary:
Fixes T9145. We currently restrict lint codes to 32 bytes, but PHPCS generates codes like "PHPCS.E.PEAR.Comments.Messages.Line.TooLong".

These codes seem reasonable as codes, and we don't currently have any key-length problems or other technical concerns with simply raising the size of this column.

Test Plan: Ran `bin/storage upgrade` to pick up adjustments.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9145

Differential Revision: https://secure.phabricator.com/D14166
2015-09-25 10:42:57 -07:00
epriestley
284fe0fe51 Allow Harbormaster to lease working copies from Drydock
Summary: Ref T9252. This is still crude in a few ways but basically works, at least for commits.

Test Plan:
  - Made a build plan with just this build step.
  - Ran `bin/harbormaster build --plan 10 ...` on a commit.
  - It actually built a working copy, leased it, took no action, and released the lease. MAGIC~~~

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14160
2015-09-24 17:29:47 -07:00
epriestley
789df89c84 Add a command queue to Drydock to manage lease/resource release
Summary:
Ref T9252. Broadly, Drydock currently races on releasing objects from the "active" state. To reproduce this:

  - Scatter some sleep()s pretty much anywhere in the release code.
  - Release several times from web UI or CLI in quick succession.

Resources or leases will execute some release code twice or otherwise do inconsistent things.

(I didn't chase down a detailed reproduction scenario for this since inspection of the code makes it clear that there are no meaningful locks or mechanisms preventing this.)

Instead, add a Harbormaster-style command queue to resources and leases. When something wants to do a release, it adds a command to the queue and schedules a worker. The workers acquire a lock, then try to consume commands from the queue.

This guarantees that only one process is responsible for writes to active resource/leases.

This is the last major step to giving resources and leases a single writer during all states:

  - Resource, Unsaved: AllocatorWorker
  - Resource, Pending: ResourceWorker (Possible rename to "Allocated?")
  - Resource, Open: This diff, ResourceUpdateWorker. (Likely rename to "Active").
  - Resource, Closed/Broken: Future destruction worker. (Likely rename to "Released" / "Broken"; maybe remove "Broken").
  - Resource, Destroyed: No writes.
  - Lease, Unsaved: Whatever wants the lease.
  - Lease, Pending: AllocatorWorker
  - Lease, Acquired: LeaseWorker
  - Lease, Active: This diff, LeaseUpdateWorker.
  - Lease, Released/Broken: Future destruction worker (Maybe remove "Broken"?)
  - Lease, Expired: No writes. (Likely rename to "Destroyed").

In most phases, we can already guarantee that there is a single writer without doing any extra work. This is more complicated in the "Active" case because the release buttons on the web UI, the release tools on the CLI, the lease requestor itself, the garbage collector, and any other release process cleaning up related objects may try to effect a release. All of these could race one another (and, in many cases, race other processes from other phases because all of these get to act immediately) as this code is currently written. Using a queue here lets us make sure there's only a single writer in this phase.

One thing which is notable is that whatever acquires a lease **can not write to it**! It is never the writer once it queues the lease for activation. It can not write to any resources, either. And, likewise, Blueprints can not write to resources while acquiring or releasing leases.

We may need to provide a mechinism so that blueprints and/or resource/lease holders get to attach some storage to resources/leases for bookkeeping. For example, a blueprint might need to keep some kind of cache on a resource to help it manage state. But I think we can cross that bridge when we come to it, and nothing else would need to write to this storage so it's technically straightforward to introduce such a mechanism if we need one.

Test Plan:
  - Viewed buttons in web UI, checked enabled/disabled states.
  - Clicked the buttons.
  - Saw commands show up in the command queue.
  - Saw some daemon stuff get scheduled.
  - Ran CLI tools, saw commands get consumed and resources/leases release.

Reviewers: hach-que, chad

Reviewed By: chad

Maniphest Tasks: T9252

Differential Revision: https://secure.phabricator.com/D14143
2015-09-23 07:42:08 -07:00
June Rhodes
d5dc4588fc [harbormaster/abort-builds] Support aborting builds in Harbormaster
Summary: Ref T1049.  This adds support for aborting builds in Harbormaster.

Test Plan: Tested it in production.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: yelirekim, traviscline, joshuaspence, Korvin, epriestley

Projects: #harbormaster

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D11870
2015-09-21 12:07:38 -07:00
Chad Little
666f19e504 Make icon setting in Section Headers easier/consistent
Summary: You can already pass other icons, but this makes it a bit simpler.

Test Plan: Test Maniphest, Badges

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14131
2015-09-19 11:29:01 -07:00
epriestley
25786df3a1 Give "Arcanist Client Results" a more clear name
Summary: Fixes T9380. See that task for discussion. This doesn't feel awesome but is maybe the least-bad fix? I think this name is clearer.

Test Plan: Looked at autoplan in Harbormaster, saw new name.

Reviewers: meitros, chad

Reviewed By: chad

Maniphest Tasks: T9380

Differential Revision: https://secure.phabricator.com/D14088
2015-09-09 19:18:08 -07:00
Joshua Spence
ff6ccd5f68 Remove leftover code for "postponed" lint and unit status
Summary: Ref T9134. It looks like this functionality was removed in D13848.

Test Plan: Submitted a diff successfully.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T9134

Differential Revision: https://secure.phabricator.com/D13869
2015-08-29 22:19:28 +10:00
June Rhodes
254b394f2c Associate Harbormaster build target with leases
Summary: Ref T1049.  This ensures the Harbormaster build target is associated with leases, so in the future we can query things and find out whether builds are still running with associated leases.

Test Plan: Leased a host, checked the DB and saw the field populated.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: joshuaspence, Korvin, epriestley

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D10870
2015-08-24 17:20:59 +10:00
Chad Little
a1d1cf77a7 Remove call to loadDrydockLease
Summary: Fixes T9219... I think.

Test Plan: swagging a guess for epriestley

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9219

Differential Revision: https://secure.phabricator.com/D13932
2015-08-18 17:52:55 -07:00
epriestley
b21270d498 Probably fix bad loadArtifactLease() call
Summary: Ref T9205. This is a likely fix.

Test Plan: This isn't straightforward to test in the upstream unless you have custom code on top of it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9205

Differential Revision: https://secure.phabricator.com/D13928
2015-08-18 10:46:41 -07:00
epriestley
74bf0d6ec6 Show external build links in applications
Summary: Fixes T8659. This isn't //explicitly// documented but I'm going to wait for a bit until the "Harbormaster" doc splits into internal/external builds to add docs for it. There's other similar stuff coming soon anyway.

Test Plan:
{F716439}

{F716440}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8659

Differential Revision: https://secure.phabricator.com/D13903
2015-08-15 07:29:26 -07:00
epriestley
57b0353034 Add harbormaster.createartifact
Summary:
Ref T8659. In the general case, this eventually allows build processes to do things like:

  - Upload build results (like a ".app" or ".exe" or other binary).
  - Pass complex results between build steps (e.g., build step A does something hard and build step B uses it to do something else).

Today, we're a long way away from having the infrastructure for that. However, it is useful to let third party build processes (like Jenkins) upload URIs that link back to the external build results.

This adds `harbormaster.createartifact` so they can do that. The only useful thing to do with this method today is have your Jenkins build do this:

  params = array(
    "uri": "https://jenkins.mycompany.com/build/23923/details/",
    "name": "View Build Results in Jenkins",
    "ui.external": true,
  );
  harbormaster.createartifact(target, 'uri', params);

Then (after the next diff) we'll show a link in Differential and a prominent link in Harbormaster. I didn't actually do the UI stuff in this diff since it's already pretty big.

This change moves a lot of code around, too:

  - Adds PHIDs to artifacts.
  - It modularizes build artifact types (currently "file", "host" and "URI").
  - It formalizes build artifact parameters and construction:
    - This lets me generate usable documentation about how to create artifacts.
    - This prevents users from doing dangerous or policy-violating things.
  - It does some other general modernization.

Test Plan:
{F715633}

{F715634}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8659

Differential Revision: https://secure.phabricator.com/D13900
2015-08-15 07:28:56 -07:00
Joshua Spence
368f359114 Use PhutilClassMapQuery instead of PhutilSymbolLoader
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.

Test Plan: Poked around a bunch of pages.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13589
2015-08-14 07:49:01 +10:00
epriestley
c2c8b00c5c Always show build target tabs, even if they have no data
Summary:
Ref T8096. Hit this on IRC:

> epriestley: what's in the Messages tab?
> jdoe: what Messages tab??!
> epriestley: ughhhh

Test Plan: Clicked "Messages" tab, saw helpful "no messages" message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13846
2015-08-10 14:15:43 -07:00
epriestley
a1431e53cc Link to Harbormaster build targets from the Daemon worker page
Summary:
Fixes T7370. Two changes:

  - Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
  - For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).

Test Plan: {F698325}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7370

Differential Revision: https://secure.phabricator.com/D13845
2015-08-10 14:15:19 -07:00
epriestley
a1499ceb8f Don't let autoplans be run manually
Summary: Ref T8096. Like stop/start, autoplans are pushed into the system from outside (normally by `arc`) so it doesn't make any sense to run them manually.

Test Plan:
  - Tried to run an autoplan from web UI, got an error.
  - Ran a normal plan from web UI.
  - Tried to run an autoplan from CLI, got an error.
  - Ran a noraml plan from CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8096

Differential Revision: https://secure.phabricator.com/D13844
2015-08-10 14:14:52 -07:00
epriestley
580790cd6e Prevent Harbormaster autobuilds from being stopped, paused or restarted
Summary: Fixes T8657. "Auto" builds are pushed into Harbormaster by an external system (currently, `arc`) so it does not make sense to stop or resume them: Harbormaster has no way to control the external system.

Test Plan:
  - Tried to restart an autobuild, got an error.
  - Restarted a normal build.
  - Did "Restart All" on a buildable, got restarts on non-autoplans and no restarts on autoplans.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8657

Differential Revision: https://secure.phabricator.com/D13812
2015-08-06 09:54:17 -07:00
epriestley
a9763062f7 Unprototype Harbormaster
Summary: Ref T8089. There's still some cleanup but none of it needs to block this.

Test Plan: Viewed `/applications/`, read documentation.

Reviewers: chad

Reviewed By: chad

Subscribers: devurandom, swisspol

Maniphest Tasks: T8089

Differential Revision: https://secure.phabricator.com/D13811
2015-08-06 09:54:00 -07:00
epriestley
8860f4724f Lump Harbormaster build steps into groups
Summary:
Ref T8089. We have a lot of broken/confusing/prototype build steps that I want to hide from users when we unprototype Harbormaster.

The dialog is also just kind of unwieldy.

Organize this UI a little better and put all the sketchy junk in a "prototypes" group that you can't see unless prototypes are enabled.

This doesn't break anything (the old steps will still work fine), but should reduce user confusion.

Test Plan:
Old UI:

{F691439}

New UI (prototypes off):

{F691440}

New UI (prototypes on):

{F691441}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8089

Differential Revision: https://secure.phabricator.com/D13803
2015-08-06 04:19:42 -07:00