revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-19 05:12:41 +01:00
Code Issues Releases Wiki Activity

Provide a more consistent, mostly relaxed severity for updating non-cluster repositories on cluster devices

Browse source 
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
This commit is contained in:
epriestley 2016-05-12 14:22:11 -07:00
parent bd9bcaa8ff
commit 984dff0ae3
9 changed files with 225 additions and 113 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/__phutil_library_map__.php
View file

@ -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',

184
src/applications/diffusion/data/DiffusionLocalRepositoryFilter.php Normal file
View file

@ -0,0 +1,184 @@
<?php
/**
* Filter a list of repositories, removing repositories not local to the
* current device.
*/
final class DiffusionLocalRepositoryFilter extends Phobject {
private $viewer;
private $device;
private $repositories;
private $rejectionReasons;
public function setViewer(PhabricatorUser $viewer) {
$this->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;
}
}

116
src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
View file

@ -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()

2
src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php
View file

@ -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(

2
src/applications/repository/management/PhabricatorRepositoryManagementMirrorWorkflow.php
View file

@ -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(

2
src/applications/repository/management/PhabricatorRepositoryManagementPullWorkflow.php
View file

@ -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(

2
src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php
View file

@ -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(

2
src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
View file

@ -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.'));

26
src/applications/repository/management/PhabricatorRepositoryManagementWorkflow.php
View file

@ -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) {