Page MenuHomePhabricator
Create Task
Maniphest T12244

Provide a crude way to selectively disable reads and writes of repository devices by setting flags on Almanac bindings
Closed, WontfixPublic
Actions

Description

We can provide a bad version of T10883 / T10884 by letting bindings have reads and writes disabled individually.

The eventual upgrade path out of this isn't great but it's simple and won't hurt anything in the meantime.

Related Objects

Event Timeline

epriestley created this task.Feb 10 2017, 6:03 PM
Herald added a subscriber: eadler. · View Herald TranscriptFeb 10 2017, 6:03 PM
jmeador added a project: Restricted Project.Mar 27 2017, 9:53 PM
jmeador moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 10 2017, 10:32 PM
epriestley added a comment.Apr 12 2017, 1:34 PM

(For reference, see discussion elsewhere.)

diff --git a/src/applications/almanac/storage/AlmanacService.php b/src/applications/almanac/storage/AlmanacService.php
index 9ef42d6407..1ac6db95dd 100644
--- a/src/applications/almanac/storage/AlmanacService.php
+++ b/src/applications/almanac/storage/AlmanacService.php
@@ -103,6 +103,18 @@ final class AlmanacService
     return $bindings;
   }
 
+  public function getActiveBindingsForTraffic() {
+    $bindings = $this->getActiveBindings();
+
+    foreach ($bindings as $key => $binding) {
+      if ($binding->getAlmanacPropertyValue('offline')) {
+        unset($bindings[$key]);
+      }
+    }
+
+    return $bindings;
+  }
+
   public function attachBindings(array $bindings) {
     $this->bindings = $bindings;
     return $this;
diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index e87c798e28..d4dd9a1d6d 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -629,7 +629,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
     }
 
     $device_map = array_fuse($device_phids);
-    $bindings = $service->getActiveBindings();
+    $bindings = $service->getActiveBindingsForTraffic();
 
     $fetchable = array();
     foreach ($bindings as $binding) {
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index fd7413c392..5b96d0758b 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -1949,7 +1949,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       return null;
     }
 
-    $bindings = $service->getActiveBindings();
+    $bindings = $service->getActiveBindingsForTraffic();
     if (!$bindings) {
       throw new Exception(
         pht(
epriestley closed this task as Wontfix.Apr 12 2017, 2:03 PM

DiffusionSSHWorkflow->execute() currently makes a proxying decision before it determines if a request is a read or write request. This needs to be reworked before we can support separate flags, since the host we'd choose to proxy the request will vary if only some hosts are readable/writable.

This isn't a huge issue, but since reworking (and testing the reworking) is somewhat substantive I think a crude fix is probably not a good shortcut here. Particularly, all the testing for changes here could save a lot of time testing other changes -- like a real fix to T10883 -- so I currently expect to implement this properly rather than provide a shortcut.

Phabricator · Built by Phacility