From 6f508a22586991319a92f25300ea2085843d8ee2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Feb 2018 12:03:56 -0800 Subject: [PATCH] Update buildable containerPHIDs in a proper way via BuildWorker rather than via sneaky uncoordinated write Summary: Depends on D19065. Ref T13054. Instead of just updating `containerPHID` and hoping for the best, queue a proper BuildWorker to process a "your container has changed, update it" message. We also need to remove a (superfluous) `withContainerPHIDs()` when loading active diffs for a revision. Test Plan: - Without daemons, created a revision and saw builds stick in "preparing" with no container PHID, but also stay in draft mode. - With daemons, saw builds actually build and get the right container PHID. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13054 Differential Revision: https://secure.phabricator.com/D19066 --- .../editor/DifferentialTransactionEditor.php | 29 ++++++++++--------- .../storage/DifferentialRevision.php | 3 +- .../engine/HarbormasterBuildEngine.php | 19 +++++++++++- .../engine/HarbormasterMessageType.php | 1 + 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 063df6c602..a092bf2693 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -365,21 +365,22 @@ final class DifferentialTransactionEditor $diff->setRevisionID($object->getID()); $diff->save(); - // Update Harbormaster to set the containerPHID correctly for any - // existing buildables. We may otherwise have buildables stuck with - // the old (`null`) container. + // If there are any outstanding buildables for this diff, tell + // Harbormaster that their containers need to be updated. This is + // common, because `arc` creates buildables so it can upload lint + // and unit results. - // TODO: This is a bit iffy, maybe we can find a cleaner approach? - // In particular, this could (rarely) be overwritten by Harbormaster - // workers. - $table = new HarbormasterBuildable(); - $conn_w = $table->establishConnection('w'); - queryfx( - $conn_w, - 'UPDATE %T SET containerPHID = %s WHERE buildablePHID = %s', - $table->getTableName(), - $object->getPHID(), - $diff->getPHID()); + $buildables = id(new HarbormasterBuildableQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withManualBuildables(false) + ->withBuildablePHIDs(array($diff->getPHID())) + ->execute(); + foreach ($buildables as $buildable) { + $buildable->sendMessage( + $this->getActor(), + HarbormasterMessageType::BUILDABLE_CONTAINER, + true); + } return; } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index e8fdf7e514..6813c124d7 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -743,9 +743,10 @@ final class DifferentialRevision extends DifferentialDAO public function loadActiveBuilds(PhabricatorUser $viewer) { $diff = $this->getActiveDiff(); + // NOTE: We can't use `withContainerPHIDs()` here because the container + // update in Harbormaster is not synchronous. $buildables = id(new HarbormasterBuildableQuery()) ->setViewer($viewer) - ->withContainerPHIDs(array($this->getPHID())) ->withBuildablePHIDs(array($diff->getPHID())) ->withManualBuildables(false) ->execute(); diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index ea20c64538..454fec7f37 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -447,11 +447,15 @@ final class HarbormasterBuildEngine extends Phobject { ->execute(); $done_preparing = false; + $update_container = false; foreach ($messages as $message) { switch ($message->getType()) { case HarbormasterMessageType::BUILDABLE_BUILD: $done_preparing = true; break; + case HarbormasterMessageType::BUILDABLE_CONTAINER: + $update_container = true; + break; default: break; } @@ -463,7 +467,6 @@ final class HarbormasterBuildEngine extends Phobject { // If we received a "build" command, all builds are scheduled and we can // move out of "preparing" into "building". - if ($done_preparing) { if ($buildable->isPreparing()) { $buildable @@ -472,6 +475,20 @@ final class HarbormasterBuildEngine extends Phobject { } } + // If we've been informed that the container for the buildable has + // changed, update it. + if ($update_container) { + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($buildable->getBuildablePHID())) + ->executeOne(); + if ($object) { + $buildable + ->setContainerPHID($object->getHarbormasterContainerPHID()) + ->save(); + } + } + // Don't update the buildable status if we're still preparing builds: more // builds may still be scheduled shortly, so even if every build we know // about so far has passed, that doesn't mean the buildable has actually diff --git a/src/applications/harbormaster/engine/HarbormasterMessageType.php b/src/applications/harbormaster/engine/HarbormasterMessageType.php index 8b3ebe2e6f..135fb23ffc 100644 --- a/src/applications/harbormaster/engine/HarbormasterMessageType.php +++ b/src/applications/harbormaster/engine/HarbormasterMessageType.php @@ -7,6 +7,7 @@ final class HarbormasterMessageType extends Phobject { const MESSAGE_WORK = 'work'; const BUILDABLE_BUILD = 'build'; + const BUILDABLE_CONTAINER = 'container'; public static function getAllMessages() { return array_keys(self::getMessageSpecifications());