From bf83fffca129c88dc69c87f196f9d12026133e5a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Jul 2021 10:44:15 -0700 Subject: [PATCH] Correct the flow of edit authority when sending messages to HarbormasterBuild objects Summary: Ref T13072. Currently, Harbormaster builds react to messages by applying a transaction inline (which can race) that has no real effect. Later, the BuildEngine picks up the mesasge and applies a real effect, but this isn't transactional. This is backwards, and makes it more difficult to transition to ModularTransaction and EditEngine. The desired workflow is: - sending a message //just// writes to the message table (and queues a worker to process the message); - the BuildEngine processes the message and applies effects in a transactional way. Force this into at least roughly the right sequence of behaviors. This paves the way for porting to ModularTransaction, which should allow further cleanup. Test Plan: Paused, resumed, aborted, and restarted a build. Ran BuildWorkers to process the commands, saw builds update appropriately. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21687 --- .../constants/HarbormasterBuildStatus.php | 21 +- .../HarbormasterBuildTransactionEditor.php | 54 ++--- .../engine/HarbormasterBuildEngine.php | 98 ++++----- .../storage/build/HarbormasterBuild.php | 189 +++++++++++------- 4 files changed, 182 insertions(+), 180 deletions(-) diff --git a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php index 7fdc68d6dd..16a4aee5c7 100644 --- a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php +++ b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php @@ -53,10 +53,6 @@ final class HarbormasterBuildStatus extends Phobject { return $this->getProperty('isComplete'); } - public function isPending() { - return $this->getProperty('isPending'); - } - public function isPassed() { return ($this->key === self::STATUS_PASSED); } @@ -81,6 +77,10 @@ final class HarbormasterBuildStatus extends Phobject { return ($this->key === self::PENDING_PAUSING); } + public function isPending() { + return ($this->key === self::STATUS_PENDING); + } + public function getIconIcon() { return $this->getProperty('icon'); } @@ -170,7 +170,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'yellow', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => false, ), self::STATUS_PENDING => array( 'name' => pht('Pending'), @@ -179,7 +178,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'yellow', 'isBuilding' => true, 'isComplete' => false, - 'isPending' => false, ), self::STATUS_BUILDING => array( 'name' => pht('Building'), @@ -188,7 +186,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'yellow', 'isBuilding' => true, 'isComplete' => false, - 'isPending' => false, ), self::STATUS_PASSED => array( 'name' => pht('Passed'), @@ -197,7 +194,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'green', 'isBuilding' => false, 'isComplete' => true, - 'isPending' => false, ), self::STATUS_FAILED => array( 'name' => pht('Failed'), @@ -206,7 +202,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => true, - 'isPending' => false, ), self::STATUS_ABORTED => array( 'name' => pht('Aborted'), @@ -215,7 +210,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => true, - 'isPending' => false, ), self::STATUS_ERROR => array( 'name' => pht('Unexpected Error'), @@ -224,7 +218,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => true, - 'isPending' => false, ), self::STATUS_PAUSED => array( 'name' => pht('Paused'), @@ -233,7 +226,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'yellow', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => false, ), self::STATUS_DEADLOCKED => array( 'name' => pht('Deadlocked'), @@ -242,7 +234,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => true, - 'isPending' => false, ), self::PENDING_PAUSING => array( 'name' => pht('Pausing'), @@ -251,7 +242,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => true, ), self::PENDING_RESUMING => array( 'name' => pht('Resuming'), @@ -260,7 +250,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => true, ), self::PENDING_RESTARTING => array( 'name' => pht('Restarting'), @@ -269,7 +258,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => true, ), self::PENDING_ABORTING => array( 'name' => pht('Aborting'), @@ -278,7 +266,6 @@ final class HarbormasterBuildStatus extends Phobject { 'color.ansi' => 'red', 'isBuilding' => false, 'isComplete' => false, - 'isPending' => true, ), ); } diff --git a/src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php b/src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php index d4c2a44957..b3bda8ffd2 100644 --- a/src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php +++ b/src/applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php @@ -65,48 +65,32 @@ final class HarbormasterBuildTransactionEditor HarbormasterBuild $build, HarbormasterBuildTransaction $xaction) { - $command = $xaction->getNewValue(); + $actor = $this->getActor(); + $message_type = $xaction->getNewValue(); - switch ($command) { - case HarbormasterBuildCommand::COMMAND_RESTART: - $issuable = $build->canRestartBuild(); + // TODO: Restore logic that tests if the command can issue without causing + // anything to lapse into an invalid state. This should not be the same + // as the logic which powers the web UI: for example, if an "abort" is + // queued we want to disable "Abort" in the web UI, but should obviously + // process it here. + + switch ($message_type) { + case HarbormasterBuildCommand::COMMAND_ABORT: + // TODO: This should move to external effects, perhaps. + $build->releaseAllArtifacts($actor); + $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED); break; - case HarbormasterBuildCommand::COMMAND_PAUSE: - $issuable = $build->canPauseBuild(); + case HarbormasterBuildCommand::COMMAND_RESTART: + $build->restartBuild($actor); + $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING); break; case HarbormasterBuildCommand::COMMAND_RESUME: - $issuable = $build->canResumeBuild(); + $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING); break; - case HarbormasterBuildCommand::COMMAND_ABORT: - $issuable = $build->canAbortBuild(); + case HarbormasterBuildCommand::COMMAND_PAUSE: + $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED); break; - default: - throw new Exception(pht('Unknown command %s', $command)); } - - if (!$issuable) { - return; - } - - $actor = $this->getActor(); - if (!$build->canIssueCommand($actor, $command)) { - return; - } - - HarbormasterBuildMessage::initializeNewMessage($actor) - ->setAuthorPHID($xaction->getAuthorPHID()) - ->setReceiverPHID($build->getPHID()) - ->setType($command) - ->save(); - - PhabricatorWorker::scheduleTask( - 'HarbormasterBuildWorker', - array( - 'buildID' => $build->getID(), - ), - array( - 'objectPHID' => $build->getPHID(), - )); } protected function applyCustomExternalTransaction( diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index 60d3040e33..29933a39eb 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -49,6 +49,7 @@ final class HarbormasterBuildEngine extends Phobject { } public function continueBuild() { + $viewer = $this->getViewer(); $build = $this->getBuild(); $lock_key = 'harbormaster.build:'.$build->getID(); @@ -68,7 +69,7 @@ final class HarbormasterBuildEngine extends Phobject { $lock->unlock(); - $this->releaseAllArtifacts($build); + $build->releaseAllArtifacts($viewer); throw $ex; } @@ -99,58 +100,60 @@ final class HarbormasterBuildEngine extends Phobject { // If we are no longer building for any reason, release all artifacts. if (!$build->isBuilding()) { - $this->releaseAllArtifacts($build); + $build->releaseAllArtifacts($viewer); } } private function updateBuild(HarbormasterBuild $build) { - if ($build->isAborting()) { - $this->releaseAllArtifacts($build); - $build->setBuildStatus(HarbormasterBuildStatus::STATUS_ABORTED); - $build->save(); + $viewer = $this->getViewer(); + + $content_source = PhabricatorContentSource::newForSource( + PhabricatorDaemonContentSource::SOURCECONST); + + $acting_phid = $viewer->getPHID(); + if (!$acting_phid) { + $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID(); } - if (($build->getBuildStatus() == HarbormasterBuildStatus::STATUS_PENDING) || - ($build->isRestarting())) { - $this->restartBuild($build); - $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING); - $build->save(); + $editor = $build->getApplicationTransactionEditor() + ->setActor($viewer) + ->setActingAsPHID($acting_phid) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $xactions = array(); + + $messages = $build->getUnprocessedMessagesForApply(); + foreach ($messages as $message) { + $message_type = $message->getType(); + + $xactions[] = $build->getApplicationTransactionTemplate() + ->setAuthorPHID($message->getAuthorPHID()) + ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) + ->setNewValue($message_type); } - if ($build->isResuming()) { - $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING); - $build->save(); + if (!$xactions) { + if ($build->isPending()) { + // TODO: This should be a transaction. + + $build->restartBuild($viewer); + $build->setBuildStatus(HarbormasterBuildStatus::STATUS_BUILDING); + $build->save(); + } } - if ($build->isPausing() && !$build->isComplete()) { - $build->setBuildStatus(HarbormasterBuildStatus::STATUS_PAUSED); - $build->save(); + if ($xactions) { + $editor->applyTransactions($build, $xactions); + $build->markUnprocessedMessagesAsProcessed(); } - $build->markUnprocessedMessagesAsProcessed(); - if ($build->getBuildStatus() == HarbormasterBuildStatus::STATUS_BUILDING) { $this->updateBuildSteps($build); } } - private function restartBuild(HarbormasterBuild $build) { - - // We're restarting the build, so release all previous artifacts. - $this->releaseAllArtifacts($build); - - // Increment the build generation counter on the build. - $build->setBuildGeneration($build->getBuildGeneration() + 1); - - // Currently running targets should periodically check their build - // generation (which won't have changed) against the build's generation. - // If it is different, they will automatically stop what they're doing - // and abort. - - // Previously we used to delete targets, logs and artifacts here. Instead, - // leave them around so users can view previous generations of this build. - } - private function updateBuildSteps(HarbormasterBuild $build) { $all_targets = id(new HarbormasterBuildTargetQuery()) ->setViewer($this->getViewer()) @@ -596,29 +599,6 @@ final class HarbormasterBuildEngine extends Phobject { ->publishBuildable($old, $new); } - private function releaseAllArtifacts(HarbormasterBuild $build) { - $targets = id(new HarbormasterBuildTargetQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withBuildPHIDs(array($build->getPHID())) - ->withBuildGenerations(array($build->getBuildGeneration())) - ->execute(); - - if (count($targets) === 0) { - return; - } - - $target_phids = mpull($targets, 'getPHID'); - - $artifacts = id(new HarbormasterBuildArtifactQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withBuildTargetPHIDs($target_phids) - ->withIsReleased(false) - ->execute(); - foreach ($artifacts as $artifact) { - $artifact->releaseArtifact(); - } - } - private function releaseQueuedArtifacts() { foreach ($this->artifactReleaseQueue as $key => $artifact) { $artifact->releaseArtifact(); diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index a96ab13c9f..5b059bbfcf 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -207,61 +207,17 @@ final class HarbormasterBuild extends HarbormasterDAO return $this->getBuildStatusObject()->isFailed(); } + public function isPending() { + return $this->getBuildstatusObject()->isPending(); + } + public function getURI() { $id = $this->getID(); return "/harbormaster/build/{$id}/"; } public function getBuildPendingStatusObject() { - - // NOTE: If a build has multiple unprocessed messages, we'll ignore - // messages that are obsoleted by a later or stronger message. - // - // For example, if a build has both "pause" and "abort" messages in queue, - // we just ignore the "pause" message and perform an "abort", since pausing - // first wouldn't affect the final state, so we can just skip it. - // - // Likewise, if a build has both "restart" and "abort" messages, the most - // recent message is controlling: we'll take whichever action a command - // was most recently issued for. - - $is_restarting = false; - $is_aborting = false; - $is_pausing = false; - $is_resuming = false; - - foreach ($this->getUnprocessedMessages() as $message_object) { - $message_type = $message_object->getType(); - switch ($message_type) { - case HarbormasterBuildCommand::COMMAND_RESTART: - $is_restarting = true; - $is_aborting = false; - break; - case HarbormasterBuildCommand::COMMAND_ABORT: - $is_aborting = true; - $is_restarting = false; - break; - case HarbormasterBuildCommand::COMMAND_PAUSE: - $is_pausing = true; - $is_resuming = false; - break; - case HarbormasterBuildCommand::COMMAND_RESUME: - $is_resuming = true; - $is_pausing = false; - break; - } - } - - $pending_status = null; - if ($is_restarting) { - $pending_status = HarbormasterBuildStatus::PENDING_RESTARTING; - } else if ($is_aborting) { - $pending_status = HarbormasterBuildStatus::PENDING_ABORTING; - } else if ($is_pausing) { - $pending_status = HarbormasterBuildStatus::PENDING_PAUSING; - } else if ($is_resuming) { - $pending_status = HarbormasterBuildStatus::PENDING_RESUMING; - } + list($pending_status) = $this->getUnprocessedMessageState(); if ($pending_status !== null) { return HarbormasterBuildStatus::newBuildStatusObject($pending_status); @@ -287,6 +243,72 @@ final class HarbormasterBuild extends HarbormasterDAO return $this->assertAttached($this->unprocessedMessages); } + public function getUnprocessedMessagesForApply() { + $unprocessed_state = $this->getUnprocessedMessageState(); + list($pending_status, $apply_messages) = $unprocessed_state; + + return $apply_messages; + } + + private function getUnprocessedMessageState() { + // NOTE: If a build has multiple unprocessed messages, we'll ignore + // messages that are obsoleted by a later or stronger message. + // + // For example, if a build has both "pause" and "abort" messages in queue, + // we just ignore the "pause" message and perform an "abort", since pausing + // first wouldn't affect the final state, so we can just skip it. + // + // Likewise, if a build has both "restart" and "abort" messages, the most + // recent message is controlling: we'll take whichever action a command + // was most recently issued for. + + $is_restarting = false; + $is_aborting = false; + $is_pausing = false; + $is_resuming = false; + + $apply_messages = array(); + + foreach ($this->getUnprocessedMessages() as $message_object) { + $message_type = $message_object->getType(); + switch ($message_type) { + case HarbormasterBuildCommand::COMMAND_RESTART: + $is_restarting = true; + $is_aborting = false; + $apply_messages = array($message_object); + break; + case HarbormasterBuildCommand::COMMAND_ABORT: + $is_aborting = true; + $is_restarting = false; + $apply_messages = array($message_object); + break; + case HarbormasterBuildCommand::COMMAND_PAUSE: + $is_pausing = true; + $is_resuming = false; + $apply_messages = array($message_object); + break; + case HarbormasterBuildCommand::COMMAND_RESUME: + $is_resuming = true; + $is_pausing = false; + $apply_messages = array($message_object); + break; + } + } + + $pending_status = null; + if ($is_restarting) { + $pending_status = HarbormasterBuildStatus::PENDING_RESTARTING; + } else if ($is_aborting) { + $pending_status = HarbormasterBuildStatus::PENDING_ABORTING; + } else if ($is_pausing) { + $pending_status = HarbormasterBuildStatus::PENDING_PAUSING; + } else if ($is_resuming) { + $pending_status = HarbormasterBuildStatus::PENDING_RESUMING; + } + + return array($pending_status, $apply_messages); + } + public function attachUnprocessedMessages(array $messages) { assert_instances_of($messages, 'HarbormasterBuildMessage'); $this->unprocessedMessages = $messages; @@ -475,32 +497,61 @@ final class HarbormasterBuild extends HarbormasterDAO } public function sendMessage(PhabricatorUser $viewer, $message_type) { - // TODO: This should not be an editor transaction, but there are plans to - // merge BuildCommand into BuildMessage which should moot this. As this - // exists today, it can race against BuildEngine. + HarbormasterBuildMessage::initializeNewMessage($viewer) + ->setReceiverPHID($this->getPHID()) + ->setType($message_type) + ->save(); - // This is a bogus content source, but this whole flow should be obsolete - // soon. - $content_source = PhabricatorContentSource::newForSource( - PhabricatorConsoleContentSource::SOURCECONST); + PhabricatorWorker::scheduleTask( + 'HarbormasterBuildWorker', + array( + 'buildID' => $this->getID(), + ), + array( + 'objectPHID' => $this->getPHID(), + 'containerPHID' => $this->getBuildablePHID(), + )); + } - $editor = id(new HarbormasterBuildTransactionEditor()) - ->setActor($viewer) - ->setContentSource($content_source) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true); + public function releaseAllArtifacts(PhabricatorUser $viewer) { + $targets = id(new HarbormasterBuildTargetQuery()) + ->setViewer($viewer) + ->withBuildPHIDs(array($this->getPHID())) + ->withBuildGenerations(array($this->getBuildGeneration())) + ->execute(); - $viewer_phid = $viewer->getPHID(); - if (!$viewer_phid) { - $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID(); - $editor->setActingAsPHID($acting_phid); + if (!$targets) { + return; } - $xaction = id(new HarbormasterBuildTransaction()) - ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) - ->setNewValue($message_type); + $target_phids = mpull($targets, 'getPHID'); - $editor->applyTransactions($this, array($xaction)); + $artifacts = id(new HarbormasterBuildArtifactQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($target_phids) + ->withIsReleased(false) + ->execute(); + foreach ($artifacts as $artifact) { + $artifact->releaseArtifact(); + } + } + + public function restartBuild(PhabricatorUser $viewer) { + // TODO: This should become transactional. + + // We're restarting the build, so release all previous artifacts. + $this->releaseAllArtifacts($viewer); + + // Increment the build generation counter on the build. + $this->setBuildGeneration($this->getBuildGeneration() + 1); + + // Currently running targets should periodically check their build + // generation (which won't have changed) against the build's generation. + // If it is different, they will automatically stop what they're doing + // and abort. + + // Previously we used to delete targets, logs and artifacts here. Instead, + // leave them around so users can view previous generations of this build. }