From 3fda965288cc64101594e051287aa618640b5efd Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 07:33:07 -0700 Subject: [PATCH] When multiple web hosts are in service, don't require setup warnings to be dismissed on each one Summary: Fixes T10876. Currently, we can end up with a setup warning banner sticking on each web device, since the state is stored in local cache. Instead: - When we actually run the setup checks, save the current state in the database. - Before we show a cached banner, make sure the database still says the checks are a problem. This could lead to some inconsistencies if setup checks legitimately pass on some hosts but not on others. For example, if you have `git` installed on one machine but not on another, we may raise a setup warning ("No Git Binary!") about it on one host only. For now, assume users have their operational environments in some sort of reasonable shape and can install the same stuff everywhere. In the future, we could split the issues into "global" and "per-host" issues if we run into problems with this. Test Plan: This is somewhat tricky to test locally since you really need multiple webservers to test it properly, but I: - Created some setup issues, saw banner. - Ignored/cleared them, saw banner go away. - Verified database cache writes were occurring properly. Then I sort of faked it like this: - Created a setup issue. - Manually set the database cache value to `[]` ("no issues"). - Reloaded page. - No more banner. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10876 Differential Revision: https://secure.phabricator.com/D15802 --- .../config/check/PhabricatorSetupCheck.php | 44 ++++++++++++++++++- .../PhabricatorConfigIssueListController.php | 3 +- .../PhabricatorConfigIssueViewController.php | 3 +- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 4c6c0b581c..a657c52799 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -50,9 +50,35 @@ abstract class PhabricatorSetupCheck extends Phobject { return $cache->getKey('phabricator.setup.issue-keys'); } - final public static function setOpenSetupIssueKeys(array $keys) { + final public static function setOpenSetupIssueKeys( + array $keys, + $update_database) { $cache = PhabricatorCaches::getSetupCache(); $cache->setKey('phabricator.setup.issue-keys', $keys); + + if ($update_database) { + $db_cache = new PhabricatorKeyValueDatabaseCache(); + try { + $json = phutil_json_encode($keys); + $db_cache->setKey('phabricator.setup.issue-keys', $json); + } catch (Exception $ex) { + // Ignore any write failures, since they likely just indicate that we + // have a database-related setup issue that needs to be resolved. + } + } + } + + final public static function getOpenSetupIssueKeysFromDatabase() { + $db_cache = new PhabricatorKeyValueDatabaseCache(); + try { + $value = $db_cache->getKey('phabricator.setup.issue-keys'); + if (!strlen($value)) { + return null; + } + return phutil_json_decode($value); + } catch (Exception $ex) { + return null; + } } final public static function getUnignoredIssueKeys(array $all_issues) { @@ -97,7 +123,21 @@ abstract class PhabricatorSetupCheck extends Phobject { ->setView($view); } } - self::setOpenSetupIssueKeys(self::getUnignoredIssueKeys($issues)); + $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 + // a warning banner to show on every device so that each one needs to + // be dismissed individually, which is pretty annoying. See T10876. + + // To avoid this, check if the issues we found have already been cleared + // in the database. If they have, we'll just wipe out our own cache and + // move on. + $issue_keys = self::getOpenSetupIssueKeysFromDatabase(); + if ($issue_keys !== null) { + self::setOpenSetupIssueKeys($issue_keys, $update_database = false); + } } // Try to repair configuration unless we have a clean bill of health on it. diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index 02bec4e082..c3dc7f577c 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -11,7 +11,8 @@ final class PhabricatorConfigIssueListController $issues = PhabricatorSetupCheck::runAllChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues)); + PhabricatorSetupCheck::getUnignoredIssueKeys($issues), + $update_database = true); $important = $this->buildIssueList( $issues, PhabricatorSetupCheck::GROUP_IMPORTANT); diff --git a/src/applications/config/controller/PhabricatorConfigIssueViewController.php b/src/applications/config/controller/PhabricatorConfigIssueViewController.php index b1d1c299a5..36fb77ce74 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueViewController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueViewController.php @@ -9,7 +9,8 @@ final class PhabricatorConfigIssueViewController $issues = PhabricatorSetupCheck::runAllChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues)); + PhabricatorSetupCheck::getUnignoredIssueKeys($issues), + $update_database = true); if (empty($issues[$issue_key])) { $content = id(new PHUIInfoView())