From bc4187d7094929548da8c1f739d11a061ee18e1d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 21 Nov 2016 06:09:10 -0800 Subject: [PATCH] When we "discover" new fatal setup issues, stop serving traffic Summary: Ref T10759. We may "discover" the presence of a fatal setup error later, after starting Phabricator. This can happen in a few ways, but most are unlikely. The one I'm immediately concerned about is: - Phabricator starts up during a disaster with some databases unreachable. - We start with warnings (unreachable databases are generally not fatal, since it's OK for some subset of hosts to be down in replicated/partitioned setups). - The unreachable databases later recover and become accessible again. - When we run checks against them, we discover that they are misconfigured. Currently, "fatal" setup issues are not truly fatal if we're "in flight" -- we've survived setup checks at least once in the past. This is bad in the scenario above. Especially with partitioning, it could lead to mangled data in a disaster scenario where operations staff makes a small configuration mistake while trying to get things running again. Instead, if we "discover" a fatal error while already "in flight", reset the whole setup process as though the webserver had just restarted. Don't serve requests again until we can make it through setup without hitting fatals. Test Plan: - Started Phabricator with multiple masters, one of which was down and broken. - Got a warning about the bad master. - Revived the master. - Before: Phabricator detects the fatal, but keeps serving requests. - After: Phabricator detects the fatal, resets the webserver, and stops serving requests until the fatal is resolved. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10759 Differential Revision: https://secure.phabricator.com/D16903 --- src/__phutil_library_map__.php | 2 + .../config/check/PhabricatorSetupCheck.php | 44 +++++++++++-- .../PhabricatorConfigIssueListController.php | 10 +-- .../PhabricatorConfigIssuePanelController.php | 15 +++-- .../PhabricatorConfigIssueViewController.php | 10 +-- .../config/engine/PhabricatorSetupEngine.php | 64 +++++++++++++++++++ 6 files changed, 123 insertions(+), 22 deletions(-) create mode 100644 src/applications/config/engine/PhabricatorSetupEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 074138cefe..17e6dbbb39 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3706,6 +3706,7 @@ phutil_register_library_map(array( 'PhabricatorSettingsTimezoneController' => 'applications/settings/controller/PhabricatorSettingsTimezoneController.php', 'PhabricatorSetupCheck' => 'applications/config/check/PhabricatorSetupCheck.php', 'PhabricatorSetupCheckTestCase' => 'applications/config/check/__tests__/PhabricatorSetupCheckTestCase.php', + 'PhabricatorSetupEngine' => 'applications/config/engine/PhabricatorSetupEngine.php', 'PhabricatorSetupIssue' => 'applications/config/issue/PhabricatorSetupIssue.php', 'PhabricatorSetupIssueUIExample' => 'applications/uiexample/examples/PhabricatorSetupIssueUIExample.php', 'PhabricatorSetupIssueView' => 'applications/config/view/PhabricatorSetupIssueView.php', @@ -8868,6 +8869,7 @@ phutil_register_library_map(array( 'PhabricatorSettingsTimezoneController' => 'PhabricatorController', 'PhabricatorSetupCheck' => 'Phobject', 'PhabricatorSetupCheckTestCase' => 'PhabricatorTestCase', + 'PhabricatorSetupEngine' => 'Phobject', 'PhabricatorSetupIssue' => 'Phobject', 'PhabricatorSetupIssueUIExample' => 'PhabricatorUIExample', 'PhabricatorSetupIssueView' => 'AphrontView', diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 7947f5aa79..0d9101efe3 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -68,6 +68,39 @@ abstract class PhabricatorSetupCheck extends Phobject { return $cache->getKey('phabricator.setup.issue-keys'); } + final public static function resetSetupState() { + $cache = PhabricatorCaches::getSetupCache(); + $cache->deleteKey('phabricator.setup.issue-keys'); + + $server_cache = PhabricatorCaches::getServerStateCache(); + $server_cache->deleteKey('phabricator.in-flight'); + + $use_scope = AphrontWriteGuard::isGuardActive(); + if ($use_scope) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(true); + } + + $caught = null; + try { + $db_cache = new PhabricatorKeyValueDatabaseCache(); + $db_cache->deleteKey('phabricator.setup.issue-keys'); + } catch (Exception $ex) { + $caught = $ex; + } + + if ($use_scope) { + unset($unguarded); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(false); + } + + if ($caught) { + throw $caught; + } + } + final public static function setOpenSetupIssueKeys( array $keys, $update_database) { @@ -161,14 +194,11 @@ abstract class PhabricatorSetupCheck extends Phobject { final public static function willProcessRequest() { $issue_keys = self::getOpenSetupIssueKeys(); if ($issue_keys === null) { - $issues = self::runNormalChecks(); - foreach ($issues as $issue) { - if ($issue->getIsFatal()) { - return self::newIssueResponse($issue); - } + $engine = new PhabricatorSetupEngine(); + $response = $engine->execute(); + if ($response) { + return $response; } - $issue_keys = self::getUnignoredIssueKeys($issues); - self::setOpenSetupIssueKeys($issue_keys, $update_database = true); } else if ($issue_keys) { // If Phabricator is configured in a cluster with multiple web devices, // we can end up with setup issues cached on every device. This can cause diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index a5553a34bb..7e4ee758f6 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -9,10 +9,12 @@ final class PhabricatorConfigIssueListController $nav = $this->buildSideNavView(); $nav->selectFilter('issue/'); - $issues = PhabricatorSetupCheck::runNormalChecks(); - PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues), - $update_database = true); + $engine = new PhabricatorSetupEngine(); + $response = $engine->execute(); + if ($response) { + return $response; + } + $issues = $engine->getIssues(); $important = $this->buildIssueList( $issues, diff --git a/src/applications/config/controller/PhabricatorConfigIssuePanelController.php b/src/applications/config/controller/PhabricatorConfigIssuePanelController.php index 3dd910f693..1aa3ab2fc1 100644 --- a/src/applications/config/controller/PhabricatorConfigIssuePanelController.php +++ b/src/applications/config/controller/PhabricatorConfigIssuePanelController.php @@ -5,11 +5,14 @@ final class PhabricatorConfigIssuePanelController public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); - $open_items = PhabricatorSetupCheck::getOpenSetupIssueKeys(); - $issues = PhabricatorSetupCheck::runNormalChecks(); - PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues), - $update_database = true); + + $engine = new PhabricatorSetupEngine(); + $response = $engine->execute(); + if ($response) { + return $response; + } + $issues = $engine->getIssues(); + $unresolved_count = count($engine->getUnresolvedIssues()); if ($issues) { require_celerity_resource('phabricator-notification-menu-css'); @@ -55,8 +58,6 @@ final class PhabricatorConfigIssuePanelController pht('Unresolved Setup Issues')), $content); - $unresolved_count = count($open_items); - $json = array( 'content' => $content, 'number' => (int)$unresolved_count, diff --git a/src/applications/config/controller/PhabricatorConfigIssueViewController.php b/src/applications/config/controller/PhabricatorConfigIssueViewController.php index 57d7b75137..43946a3e7b 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueViewController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueViewController.php @@ -7,10 +7,12 @@ final class PhabricatorConfigIssueViewController $viewer = $request->getViewer(); $issue_key = $request->getURIData('key'); - $issues = PhabricatorSetupCheck::runNormalChecks(); - PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues), - $update_database = true); + $engine = new PhabricatorSetupEngine(); + $response = $engine->execute(); + if ($response) { + return $response; + } + $issues = $engine->getIssues(); $nav = $this->buildSideNavView(); $nav->selectFilter('issue/'); diff --git a/src/applications/config/engine/PhabricatorSetupEngine.php b/src/applications/config/engine/PhabricatorSetupEngine.php new file mode 100644 index 0000000000..3a3e6f0419 --- /dev/null +++ b/src/applications/config/engine/PhabricatorSetupEngine.php @@ -0,0 +1,64 @@ +issues === null) { + throw new PhutilInvalidStateException('execute'); + } + + return $this->issues; + } + + public function getUnresolvedIssues() { + $issues = $this->getIssues(); + $issues = mpull($issues, null, 'getIssueKey'); + + $unresolved_keys = PhabricatorSetupCheck::getUnignoredIssueKeys($issues); + + return array_select_keys($issues, $unresolved_keys); + } + + public function execute() { + $issues = PhabricatorSetupCheck::runNormalChecks(); + + $fatal_issue = null; + foreach ($issues as $issue) { + if ($issue->getIsFatal()) { + $fatal_issue = $issue; + break; + } + } + + if ($fatal_issue) { + // If we've discovered a fatal, we reset any in-flight state to push + // web hosts out of service. + + // This can happen if Phabricator starts during a disaster and some + // databases can not be reached. We allow Phabricator to start up in + // this situation, since it may still be able to usefully serve requests + // without risk to data. + + // However, if databases later become reachable and we learn that they + // are fatally misconfigured, we want to tear the world down again + // because data may be at risk. + PhabricatorSetupCheck::resetSetupState(); + + return PhabricatorSetupCheck::newIssueResponse($issue); + } + + $issue_keys = PhabricatorSetupCheck::getUnignoredIssueKeys($issues); + + PhabricatorSetupCheck::setOpenSetupIssueKeys( + $issue_keys, + $update_database = true); + + $this->issues = $issues; + + return null; + } + +}