From 984dff0ae335dcb6987f2d72ca28f79b4cdab818 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 May 2016 14:22:11 -0700 Subject: [PATCH] Provide a more consistent, mostly relaxed severity for updating non-cluster repositories on cluster devices Summary: Fixes T10940. Two issues currently: First, `PullLocal` deamon refuses to update non-cluster repositories on cluster devices. However, this is surprising/confusing/bad because as soon as you enroll a repository host in the cluster, most of the repositories on it stop working until you `clusterize` them. This is especially confusing because the documentation gives you a very nice, gradual walkthrough about going through things slowly and being able to check your work at every step, but we really drop you off a bit of a cliff here. The workflow implied by the documentation is a desirable one. This operation is generally only unsafe/problematic if the daemon would be creating a //new// working copy. If a working copy already exists, we can reasonably guess that it's almost certainly because you've enrolled a previously un-clustered host into a new cluster. This allows the nice, gradual workflow the documentation describes to proceed as expected, without any weird surprises. Instead of refusing to update these repositories, only refuse to update them if updating would create a new working copy. This should make transitioning much smoother without any meaningful reduction in safety. Second, the lower-level `bin/repository update`, `refs`, `mirror`, etc., commands don't apply this same check. However, these commands are potentially just as dangerous. Use the same code to do a similar check there, making sure we only operate on repositories that are either expected to be on the current device, or which already exist here. Test Plan: - Ran `bin/phd debug pull`, saw diagnostic information choose to update most repositories (including some non-cluster repositories) but properly skip non-cluster repositories that do not exist locally. - Ran `bin/repository update`, etc., saw the command apply consistent rules to the rules applied by `PullLocal` and refuse to update non-local repositories it would need to create. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10940 Differential Revision: https://secure.phabricator.com/D15902 --- src/__phutil_library_map__.php | 2 + .../data/DiffusionLocalRepositoryFilter.php | 184 ++++++++++++++++++ .../PhabricatorRepositoryPullLocalDaemon.php | 116 +---------- ...orRepositoryManagementDiscoverWorkflow.php | 2 +- ...atorRepositoryManagementMirrorWorkflow.php | 2 +- ...icatorRepositoryManagementPullWorkflow.php | 2 +- ...icatorRepositoryManagementRefsWorkflow.php | 2 +- ...atorRepositoryManagementUpdateWorkflow.php | 2 +- ...habricatorRepositoryManagementWorkflow.php | 26 +++ 9 files changed, 225 insertions(+), 113 deletions(-) create mode 100644 src/applications/diffusion/data/DiffusionLocalRepositoryFilter.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8f4756e9e4..1bca354414 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -665,6 +665,7 @@ phutil_register_library_map(array( 'DiffusionLintController' => 'applications/diffusion/controller/DiffusionLintController.php', 'DiffusionLintCountQuery' => 'applications/diffusion/query/DiffusionLintCountQuery.php', 'DiffusionLintSaveRunner' => 'applications/diffusion/DiffusionLintSaveRunner.php', + 'DiffusionLocalRepositoryFilter' => 'applications/diffusion/data/DiffusionLocalRepositoryFilter.php', 'DiffusionLookSoonConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionLookSoonConduitAPIMethod.php', 'DiffusionLowLevelCommitFieldsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php', 'DiffusionLowLevelCommitQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php', @@ -4890,6 +4891,7 @@ phutil_register_library_map(array( 'DiffusionLintController' => 'DiffusionController', 'DiffusionLintCountQuery' => 'PhabricatorQuery', 'DiffusionLintSaveRunner' => 'Phobject', + 'DiffusionLocalRepositoryFilter' => 'Phobject', 'DiffusionLookSoonConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionLowLevelCommitFieldsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelCommitQuery' => 'DiffusionLowLevelQuery', diff --git a/src/applications/diffusion/data/DiffusionLocalRepositoryFilter.php b/src/applications/diffusion/data/DiffusionLocalRepositoryFilter.php new file mode 100644 index 0000000000..b443352206 --- /dev/null +++ b/src/applications/diffusion/data/DiffusionLocalRepositoryFilter.php @@ -0,0 +1,184 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setDevice(AlmanacDevice $device = null) { + $this->device = $device; + return $this; + } + + public function getDevice() { + return $this->device; + } + + public function setRepositories(array $repositories) { + $this->repositories = $repositories; + return $this; + } + + public function getRepositories() { + return $this->repositories; + } + + public function setRejectionReasons($rejection_reasons) { + $this->rejectionReasons = $rejection_reasons; + return $this; + } + + public function getRejectionReasons() { + return $this->rejectionReasons; + } + + public function execute() { + $repositories = $this->getRepositories(); + $device = $this->getDevice(); + $viewer = $this->getViewer(); + + $reasons = array(); + + $service_phids = array(); + foreach ($repositories as $key => $repository) { + $service_phid = $repository->getAlmanacServicePHID(); + + // If the repository is bound to a service but this host is not a + // recognized device, or vice versa, don't pull the repository unless + // we're sure it's safe because the repository has no local working copy + // or the working copy already exists on disk. + $is_cluster_repo = (bool)$service_phid; + $is_cluster_device = (bool)$device; + if ($is_cluster_repo != $is_cluster_device) { + $has_working_copy = $repository->hasLocalWorkingCopy(); + if ($is_cluster_device) { + if (!$has_working_copy) { + $reasons[$key] = pht( + 'Repository "%s" is not a cluster repository, but the current '. + 'host is a cluster device ("%s") and updating this repository '. + 'would create a new local working copy. This is dangerous, so '. + 'the repository will not be updated on this host.', + $repository->getDisplayName(), + $device->getName()); + unset($repositories[$key]); + continue; + } + } else { + $reasons[$key] = pht( + 'Repository "%s" is a cluster repository, but the current host '. + 'is not a cluster device (it has no device ID), so the '. + 'repository will not be updated on this host.', + $repository->getDisplayName()); + unset($repositories[$key]); + continue; + } + } + + if ($service_phid) { + $service_phids[] = $service_phid; + } + } + + if (!$device) { + $this->rejectionReasons = $reasons; + return $repositories; + } + + $device_phid = $device->getPHID(); + + if ($service_phids) { + // We could include `withDevicePHIDs()` here to pull a smaller result + // set, but we can provide more helpful diagnostic messages below if + // we fetch a little more data. + $services = id(new AlmanacServiceQuery()) + ->setViewer($viewer) + ->withPHIDs($service_phids) + ->withServiceTypes( + array( + AlmanacClusterRepositoryServiceType::SERVICETYPE, + )) + ->needBindings(true) + ->execute(); + $services = mpull($services, null, 'getPHID'); + } else { + $services = array(); + } + + foreach ($repositories as $key => $repository) { + $service_phid = $repository->getAlmanacServicePHID(); + + if (!$service_phid) { + continue; + } + + $service = idx($services, $service_phid); + if (!$service) { + $reasons[$key] = pht( + 'Repository "%s" is on cluster service "%s", but that service '. + 'could not be loaded, so the repository will not be updated on '. + 'this host.', + $repository->getDisplayName(), + $service_phid); + unset($repositories[$key]); + continue; + } + + $bindings = $service->getBindings(); + $bindings = mgroup($bindings, 'getDevicePHID'); + $bindings = idx($bindings, $device_phid); + if (!$bindings) { + $reasons[$key] = pht( + 'Repository "%s" is on cluster service "%s", but that service is '. + 'not bound to this device ("%s"), so the repository will not be '. + 'updated on this host.', + $repository->getDisplayName(), + $service->getName(), + $device->getName()); + unset($repositories[$key]); + continue; + } + + $all_disabled = true; + foreach ($bindings as $binding) { + if (!$binding->getIsDisabled()) { + $all_disabled = false; + break; + } + } + + if ($all_disabled) { + $reasons[$key] = pht( + 'Repository "%s" is on cluster service "%s", but the binding '. + 'between that service and this device ("%s") is disabled, so it '. + 'can not be updated on this host.', + $repository->getDisplayName(), + $service->getName(), + $device->getName()); + unset($repositories[$key]); + continue; + } + + // We have a valid service that is actively bound to the current host + // device, so we're good to go. + } + + $this->rejectionReasons = $reasons; + return $repositories; + } + +} diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index da8d9336d2..bbd73d2d90 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -354,117 +354,17 @@ final class PhabricatorRepositoryPullLocalDaemon } } - $service_phids = array(); - foreach ($repositories as $key => $repository) { - $service_phid = $repository->getAlmanacServicePHID(); + $viewer = $this->getViewer(); - // If the repository is bound to a service but this host is not a - // recognized device, or vice versa, don't pull the repository. - $is_cluster_repo = (bool)$service_phid; - $is_cluster_device = (bool)$device; - if ($is_cluster_repo != $is_cluster_device) { - if ($is_cluster_device) { - $this->log( - pht( - 'Repository "%s" is not a cluster repository, but the current '. - 'host is a cluster device ("%s"), so the repository will not '. - 'be updated on this host.', - $repository->getDisplayName(), - $device->getName())); - } else { - $this->log( - pht( - 'Repository "%s" is a cluster repository, but the current '. - 'host is not a cluster device (it has no device ID), so the '. - 'repository will not be updated on this host.', - $repository->getDisplayName())); - } - unset($repositories[$key]); - continue; - } + $filter = id(new DiffusionLocalRepositoryFilter()) + ->setViewer($viewer) + ->setDevice($device) + ->setRepositories($repositories); - if ($service_phid) { - $service_phids[] = $service_phid; - } - } + $repositories = $filter->execute(); - if ($device) { - $device_phid = $device->getPHID(); - - if ($service_phids) { - // We could include `withDevicePHIDs()` here to pull a smaller result - // set, but we can provide more helpful diagnostic messages below if - // we fetch a little more data. - $services = id(new AlmanacServiceQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($service_phids) - ->withServiceTypes( - array( - AlmanacClusterRepositoryServiceType::SERVICETYPE, - )) - ->needBindings(true) - ->execute(); - $services = mpull($services, null, 'getPHID'); - } else { - $services = array(); - } - - foreach ($repositories as $key => $repository) { - $service_phid = $repository->getAlmanacServicePHID(); - - $service = idx($services, $service_phid); - if (!$service) { - $this->log( - pht( - 'Repository "%s" is on cluster service "%s", but that service '. - 'could not be loaded, so the repository will not be updated '. - 'on this host.', - $repository->getDisplayName(), - $service_phid)); - unset($repositories[$key]); - continue; - } - - $bindings = $service->getBindings(); - $bindings = mgroup($bindings, 'getDevicePHID'); - $bindings = idx($bindings, $device_phid); - if (!$bindings) { - $this->log( - pht( - 'Repository "%s" is on cluster service "%s", but that service '. - 'is not bound to this device ("%s"), so the repository will '. - 'not be updated on this host.', - $repository->getDisplayName(), - $service->getName(), - $device->getName())); - unset($repositories[$key]); - continue; - } - - $all_disabled = true; - foreach ($bindings as $binding) { - if (!$binding->getIsDisabled()) { - $all_disabled = false; - break; - } - } - - if ($all_disabled) { - $this->log( - pht( - 'Repository "%s" is on cluster service "%s", but the binding '. - 'between that service and this device ("%s") is disabled, so '. - 'the not be updated on this host.', - $repository->getDisplayName(), - $service->getName(), - $device->getName())); - unset($repositories[$key]); - continue; - } - - // We have a valid service that is actively bound to the current host - // device, so we're good to go. - } + foreach ($filter->getRejectionReasons() as $reason) { + $this->log($reason); } // Shuffle the repositories, then re-key the array since shuffle() diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php index 03c4745841..c534edf907 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php @@ -27,7 +27,7 @@ final class PhabricatorRepositoryManagementDiscoverWorkflow } public function execute(PhutilArgumentParser $args) { - $repos = $this->loadRepositories($args, 'repos'); + $repos = $this->loadLocalRepositories($args, 'repos'); if (!$repos) { throw new PhutilArgumentUsageException( diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementMirrorWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementMirrorWorkflow.php index 5db4899b41..caa7574424 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementMirrorWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementMirrorWorkflow.php @@ -23,7 +23,7 @@ final class PhabricatorRepositoryManagementMirrorWorkflow } public function execute(PhutilArgumentParser $args) { - $repos = $this->loadRepositories($args, 'repos'); + $repos = $this->loadLocalRepositories($args, 'repos'); if (!$repos) { throw new PhutilArgumentUsageException( diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementPullWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementPullWorkflow.php index f41f8b95a1..808986f9aa 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementPullWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementPullWorkflow.php @@ -22,7 +22,7 @@ final class PhabricatorRepositoryManagementPullWorkflow } public function execute(PhutilArgumentParser $args) { - $repos = $this->loadRepositories($args, 'repos'); + $repos = $this->loadLocalRepositories($args, 'repos'); if (!$repos) { throw new PhutilArgumentUsageException( diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php index ad9bbbd0db..8c806edac8 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php @@ -22,7 +22,7 @@ final class PhabricatorRepositoryManagementRefsWorkflow } public function execute(PhutilArgumentParser $args) { - $repos = $this->loadRepositories($args, 'repos'); + $repos = $this->loadLocalRepositories($args, 'repos'); if (!$repos) { throw new PhutilArgumentUsageException( diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php index 9d987636b6..11ad6a9418 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php @@ -44,7 +44,7 @@ final class PhabricatorRepositoryManagementUpdateWorkflow $this->setVerbose($args->getArg('verbose')); $console = PhutilConsole::getConsole(); - $repos = $this->loadRepositories($args, 'repos'); + $repos = $this->loadLocalRepositories($args, 'repos'); if (count($repos) !== 1) { throw new PhutilArgumentUsageException( pht('Specify exactly one repository to update.')); diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementWorkflow.php index 56a75764c8..654a974e6c 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementWorkflow.php @@ -33,6 +33,32 @@ abstract class PhabricatorRepositoryManagementWorkflow return array_values($repositories); } + protected function loadLocalRepositories( + PhutilArgumentParser $args, + $param) { + + $repositories = $this->loadRepositories($args, $param); + if (!$repositories) { + return $repositories; + } + + $device = AlmanacKeys::getLiveDevice(); + $viewer = $this->getViewer(); + + $filter = id(new DiffusionLocalRepositoryFilter()) + ->setViewer($viewer) + ->setDevice($device) + ->setRepositories($repositories); + + $repositories = $filter->execute(); + + foreach ($filter->getRejectionReasons() as $reason) { + throw new PhutilArgumentUsageException($reason); + } + + return $repositories; + } + protected function loadCommits(PhutilArgumentParser $args, $param) { $names = $args->getArg($param); if (!$names) {