From b6420e0f0ad8f0c14d018880700d0481d3d4f39c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Sep 2019 08:56:24 -0700 Subject: [PATCH] Allow repository service lookups to return an ordered list of service refs Summary: Ref T13286. To support request retries, allow the service lookup method to return an ordered list of structured service references. Existing callsites continue to immediately discard all but the first reference and pull a URI out of it. Test Plan: Ran `git pull` in a clustered repository with an "up" node and a "down" node, saw 50% serivce failures and 50% clean pulls. Maniphest Tasks: T13286 Differential Revision: https://secure.phabricator.com/D20775 --- src/__phutil_library_map__.php | 2 + .../diffusion/ref/DiffusionServiceRef.php | 48 ++++++++++++++ .../diffusion/ssh/DiffusionSSHWorkflow.php | 22 +++++-- .../storage/PhabricatorRepository.php | 62 +++++++++++++------ 4 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 src/applications/diffusion/ref/DiffusionServiceRef.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 96d3713b48..212a7b1607 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1021,6 +1021,7 @@ phutil_register_library_map(array( 'DiffusionSSHWorkflow' => 'applications/diffusion/ssh/DiffusionSSHWorkflow.php', 'DiffusionSearchQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php', 'DiffusionServeController' => 'applications/diffusion/controller/DiffusionServeController.php', + 'DiffusionServiceRef' => 'applications/diffusion/ref/DiffusionServiceRef.php', 'DiffusionSetPasswordSettingsPanel' => 'applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php', 'DiffusionSetupException' => 'applications/diffusion/exception/DiffusionSetupException.php', 'DiffusionSourceHyperlinkEngineExtension' => 'applications/diffusion/engineextension/DiffusionSourceHyperlinkEngineExtension.php', @@ -6967,6 +6968,7 @@ phutil_register_library_map(array( 'DiffusionSSHWorkflow' => 'PhabricatorSSHWorkflow', 'DiffusionSearchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionServeController' => 'DiffusionController', + 'DiffusionServiceRef' => 'Phobject', 'DiffusionSetPasswordSettingsPanel' => 'PhabricatorSettingsPanel', 'DiffusionSetupException' => 'Exception', 'DiffusionSourceHyperlinkEngineExtension' => 'PhabricatorRemarkupHyperlinkEngineExtension', diff --git a/src/applications/diffusion/ref/DiffusionServiceRef.php b/src/applications/diffusion/ref/DiffusionServiceRef.php new file mode 100644 index 0000000000..d6e6948e5d --- /dev/null +++ b/src/applications/diffusion/ref/DiffusionServiceRef.php @@ -0,0 +1,48 @@ +uri = $map['uri']; + $ref->isWritable = $map['writable']; + $ref->devicePHID = $map['devicePHID']; + $ref->protocol = $map['protocol']; + $ref->deviceName = $map['device']; + + return $ref; + } + + public function isWritable() { + return $this->isWritable; + } + + public function getDevicePHID() { + return $this->devicePHID; + } + + public function getURI() { + return $this->uri; + } + + public function getProtocol() { + return $this->protocol; + } + + public function getDeviceName() { + return $this->deviceName; + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index 08144eb0c9..358418f44c 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -73,13 +73,13 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { return $this->shouldProxy; } - protected function getProxyCommand($for_write) { + final protected function getAlmanacServiceRefs($for_write) { $viewer = $this->getSSHUser(); $repository = $this->getRepository(); $is_cluster_request = $this->getIsClusterRequest(); - $uri = $repository->getAlmanacServiceURI( + $refs = $repository->getAlmanacServiceRefs( $viewer, array( 'neverProxy' => $is_cluster_request, @@ -89,14 +89,28 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { 'writable' => $for_write, )); - if (!$uri) { + if (!$refs) { throw new Exception( pht( 'Failed to generate an intracluster proxy URI even though this '. 'request was routed as a proxy request.')); } - $uri = new PhutilURI($uri); + return $refs; + } + + final protected function getProxyCommand($for_write) { + $refs = $this->getAlmanacServiceRefs($for_write); + + $ref = head($refs); + + return $this->getProxyCommandForServiceRef($ref); + } + + final protected function getProxyCommandForServiceRef( + DiffusionServiceRef $ref) { + + $uri = new PhutilURI($ref->getURI()); $username = AlmanacKeys::getClusterSSHUser(); if ($username === null) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index bd15e3e8de..fdc9a695c4 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1842,6 +1842,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO PhabricatorUser $viewer, array $options) { + $refs = $this->getAlmanacServiceRefs($viewer, $options); + + if (!$refs) { + return null; + } + + $ref = head($refs); + return $ref->getURI(); + } + + public function getAlmanacServiceRefs( + PhabricatorUser $viewer, + array $options) { + PhutilTypeSpec::checkMap( $options, array( @@ -1856,7 +1870,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $cache_key = $this->getAlmanacServiceCacheKey(); if (!$cache_key) { - return null; + return array(); } $cache = PhabricatorCaches::getMutableStructureCache(); @@ -1869,7 +1883,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } if ($uris === null) { - return null; + return array(); } $local_device = AlmanacKeys::getDeviceID(); @@ -1893,7 +1907,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO if ($local_device && $never_proxy) { if ($uri['device'] == $local_device) { - return null; + return array(); } } @@ -1954,15 +1968,20 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } } + $refs = array(); + foreach ($results as $result) { + $refs[] = DiffusionServiceRef::newFromDictionary($result); + } + // If we require a writable device, remove URIs which aren't writable. if ($writable) { - foreach ($results as $key => $uri) { - if (!$uri['writable']) { + foreach ($refs as $key => $ref) { + if (!$ref->isWritable()) { unset($results[$key]); } } - if (!$results) { + if (!$refs) { throw new Exception( pht( 'This repository ("%s") is not writable with the given '. @@ -1974,23 +1993,30 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } if ($writable) { - $results = $this->sortWritableAlmanacServiceURIs($results); + $refs = $this->sortWritableAlmanacServiceRefs($refs); } else { - shuffle($results); + $refs = $this->sortReadableAlmanacServiceRefs($refs); } - $result = head($results); - return $result['uri']; + return array_values($refs); } - private function sortWritableAlmanacServiceURIs(array $results) { + private function sortReadableAlmanacServiceRefs(array $refs) { + assert_instances_of($refs, 'DiffusionServiceRef'); + shuffle($refs); + return $refs; + } + + private function sortWritableAlmanacServiceRefs(array $refs) { + assert_instances_of($refs, 'DiffusionServiceRef'); + // See T13109 for discussion of how this method routes requests. // In the absence of other rules, we'll send traffic to devices randomly. // We also want to select randomly among nodes which are equally good // candidates to receive the write, and accomplish that by shuffling the // list up front. - shuffle($results); + shuffle($refs); $order = array(); @@ -2002,8 +2028,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $this->getPHID()); if ($writer) { $device_phid = $writer->getWriteProperty('devicePHID'); - foreach ($results as $key => $result) { - if ($result['devicePHID'] === $device_phid) { + foreach ($refs as $key => $ref) { + if ($ref->getDevicePHID() === $device_phid) { $order[] = $key; } } @@ -2025,8 +2051,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } $max_devices = array_fuse($max_devices); - foreach ($results as $key => $result) { - if (isset($max_devices[$result['devicePHID']])) { + foreach ($refs as $key => $ref) { + if (isset($max_devices[$ref->getDevicePHID()])) { $order[] = $key; } } @@ -2034,9 +2060,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO // Reorder the results, putting any we've selected as preferred targets for // the write at the head of the list. - $results = array_select_keys($results, $order) + $results; + $refs = array_select_keys($refs, $order) + $refs; - return $results; + return $refs; } public function supportsSynchronization() {