From 201f29fbf4ab74a2cd41433c4ba57c77d64e4e6e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 09:24:48 -0700 Subject: [PATCH 1/6] Fix truncation in "bin/storage probe" of tables larger than 100GB Summary: Ref T13164. PHI805 incidentally includes some `bin/storage probe` output for 100GB+ tables which renders wrong. We have the tools to render it properly, so stop doing this manually and let ConsoleTable figure out the alignment. Test Plan: Faked very large table sizes, ran `bin/storage probe`: {F5785946} (Then, un-faked the very large table sizes and ran it again, got sensible output.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19567 --- ...PhabricatorStorageManagementProbeWorkflow.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php index a75afe69e8..5c55dc779e 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php +++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementProbeWorkflow.php @@ -56,8 +56,18 @@ final class PhabricatorStorageManagementProbeWorkflow ->setShowHeader(false) ->setPadding(2) ->addColumn('name', array('title' => pht('Database / Table'))) - ->addColumn('size', array('title' => pht('Size'))) - ->addColumn('percentage', array('title' => pht('Percentage'))); + ->addColumn( + 'size', + array( + 'title' => pht('Size'), + 'align' => PhutilConsoleTable::ALIGN_RIGHT, + )) + ->addColumn( + 'percentage', + array( + 'title' => pht('Percentage'), + 'align' => PhutilConsoleTable::ALIGN_RIGHT, + )); foreach ($totals as $db => $size) { list($database_size, $database_percentage) = $this->formatSize( @@ -98,7 +108,7 @@ final class PhabricatorStorageManagementProbeWorkflow private function formatSize($n, $o) { return array( - sprintf('%8.8s MB', number_format($n / (1024 * 1024), 1)), + pht('%s MB', new PhutilNumber($n / (1024 * 1024), 1)), sprintf('%3.1f%%', 100 * ($n / $o)), ); } From 3ca3f09a1aa6c0dc407ee2ae9e5f9cf4f995bbd6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 09:06:22 -0700 Subject: [PATCH 2/6] Add an "--as" flag to "bin/conduit call ..." to improve flexibility and ease of profiling Summary: Ref T13164. In PHI801, an install reported a particular slow Conduit method call. Conduit calls aren't easily profilable with normal tools (for example, `arc call-conduit --xprofile ...` gives you a profile of the //client//). They can be profiled most easily with `bin/conduit call ... --xprofile`. However, `bin/conduit call` currently doesn't let you pick a user to execute the command on behalf of, so it's not terribly useful for profiling `*.edit`-style methods which do a write: these need a real acting user. Test Plan: Ran `bin/conduit call --method user.whoami --as epriestley ...` with valid, invalid, and no acting users. ``` $ echo '{}' | ./bin/conduit call --method user.whoami --as epriestley --input - Reading input from stdin... { "result": { "phid": "PHID-USER-icyixzkx3f4ttv67avbn", "userName": "epriestley", "realName": "Evan Priestley", ... ``` Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19566 --- ...abricatorConduitCallManagementWorkflow.php | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php index 6cb3bd2409..f9ba48b372 100644 --- a/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php +++ b/src/applications/conduit/management/PhabricatorConduitCallManagementWorkflow.php @@ -21,6 +21,13 @@ final class PhabricatorConduitCallManagementWorkflow 'File to read parameters from, or "-" to read from '. 'stdin.'), ), + array( + 'name' => 'as', + 'param' => 'username', + 'help' => pht( + 'Execute the call as the given user. (If omitted, the call will '. + 'be executed as an omnipotent user.)'), + ), )); } @@ -39,6 +46,22 @@ final class PhabricatorConduitCallManagementWorkflow pht('Specify a file to read parameters from with "--input".')); } + $as = $args->getArg('as'); + if (strlen($as)) { + $actor = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withUsernames(array($as)) + ->executeOne(); + if (!$actor) { + throw new PhutilArgumentUsageException( + pht( + 'No such user "%s" exists.', + $as)); + } + } else { + $actor = $viewer; + } + if ($input === '-') { fprintf(STDERR, tsprintf("%s\n", pht('Reading input from stdin...'))); $input_json = file_get_contents('php://stdin'); @@ -49,7 +72,7 @@ final class PhabricatorConduitCallManagementWorkflow $params = phutil_json_decode($input_json); $result = id(new ConduitCall($method, $params)) - ->setUser($viewer) + ->setUser($actor) ->execute(); $output = array( From df31405d64a44a9ae7a12a324fb292dc8357be46 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 11:26:34 -0700 Subject: [PATCH 3/6] Improve compatibility of "Config > Cache Status" across APCu versions Summary: Ref T13164. See PHI790. Older versions of APCu reported cache keys as "key" from `apcu_cache_info()`. APC and newer APCu report it as "info". Check both indexes for compatibility. Test Plan: - Locally, with newer APCu, saw no behavioral change. - Will double check on `admin`, which has an older APCu with the "key" behavior. - (I hunted this down by dumping `apcu_cache_info()` on `admin` to see what was going on.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19569 --- .../cache/spec/PhabricatorDataCacheSpec.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/applications/cache/spec/PhabricatorDataCacheSpec.php b/src/applications/cache/spec/PhabricatorDataCacheSpec.php index bf6a495f7f..103b0d8394 100644 --- a/src/applications/cache/spec/PhabricatorDataCacheSpec.php +++ b/src/applications/cache/spec/PhabricatorDataCacheSpec.php @@ -106,8 +106,21 @@ final class PhabricatorDataCacheSpec extends PhabricatorCacheSpec { $cache = $info['cache_list']; $state = array(); foreach ($cache as $item) { - $info = idx($item, 'info', ''); - $key = self::getKeyPattern($info); + // Some older versions of APCu report the cachekey as "key", while + // newer APCu and APC report it as "info". Just check both indexes + // for commpatibility. See T13164 for details. + + $info = idx($item, 'info'); + if ($info === null) { + $info = idx($item, 'key'); + } + + if ($info === null) { + $key = ''; + } else { + $key = self::getKeyPattern($info); + } + if (empty($state[$key])) { $state[$key] = array( 'max' => 0, From 6df278bea84a2c8a66752bf4b591d7f3d4432f89 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Aug 2018 10:07:00 -0700 Subject: [PATCH 4/6] In "bin/ssh-auth", cache a structure instead of a flat file because paths may change at runtime Summary: Fixes T12397. Ref T13164. See PHI801. Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves. These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making. Test Plan: - Compared output on `master` to output after change, found them byte-for-byte identical. - Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output. - Added a new SSH key, saw it appear in the output. - Grepped for `AUTHFILE_CACHEKEY` (no hits). - Dropped the cache, verified that the file regenerates cleanly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164, T12397 Differential Revision: https://secure.phabricator.com/D19568 --- scripts/ssh/ssh-auth.php | 89 +++++++++++++------ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 4 +- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index b25905668b..3c4f3f2b33 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -2,13 +2,27 @@ getKey($authfile_key); +$authstruct_key = PhabricatorAuthSSHKeyQuery::AUTHSTRUCT_CACHEKEY; +$authstruct_raw = $cache->getKey($authstruct_key); -if ($authfile === null) { +$authstruct = null; + +if (strlen($authstruct_raw)) { + try { + $authstruct = phutil_json_decode($authstruct_raw); + } catch (Exception $ex) { + // Ignore any issues with the cached data; we'll just rebuild the + // structure below. + } +} + +if ($authstruct === null) { $keys = id(new PhabricatorAuthSSHKeyQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withIsActive(true) @@ -19,7 +33,7 @@ if ($authfile === null) { exit(1); } - $bin = $root.'/bin/ssh-exec'; + $key_list = array(); foreach ($keys as $ssh_key) { $key_argv = array(); $object = $ssh_key->getObject(); @@ -42,18 +56,7 @@ if ($authfile === null) { $key_argv[] = '--phabricator-ssh-key'; $key_argv[] = $ssh_key->getID(); - $cmd = csprintf('%s %Ls', $bin, $key_argv); - - $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { - $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd); - } - - // This is additional escaping for the SSH 'command="..."' string. - $cmd = addcslashes($cmd, '"\\'); - // Strip out newlines and other nonsense from the key type and key body. - $type = $ssh_key->getKeyType(); $type = preg_replace('@[\x00-\x20]+@', '', $type); if (!strlen($type)) { @@ -66,22 +69,54 @@ if ($authfile === null) { continue; } - $options = array( - 'command="'.$cmd.'"', - 'no-port-forwarding', - 'no-X11-forwarding', - 'no-agent-forwarding', - 'no-pty', + $key_list[] = array( + 'argv' => $key_argv, + 'type' => $type, + 'key' => $key, ); - $options = implode(',', $options); - - $lines[] = $options.' '.$type.' '.$key."\n"; } - $authfile = implode('', $lines); + $authstruct = array( + 'keys' => $key_list, + ); + + $authstruct_raw = phutil_json_encode($authstruct); $ttl = phutil_units('24 hours in seconds'); - $cache->setKey($authfile_key, $authfile, $ttl); + $cache->setKey($authstruct_key, $authstruct_raw, $ttl); } +$bin = $root.'/bin/ssh-exec'; +$instance = PhabricatorEnv::getEnvConfig('cluster.instance'); + +$lines = array(); +foreach ($authstruct['keys'] as $key_struct) { + $key_argv = $key_struct['argv']; + $key = $key_struct['key']; + $type = $key_struct['type']; + + $cmd = csprintf('%s %Ls', $bin, $key_argv); + + if (strlen($instance)) { + $cmd = csprintf('PHABRICATOR_INSTANCE=%s %C', $instance, $cmd); + } + + // This is additional escaping for the SSH 'command="..."' string. + $cmd = addcslashes($cmd, '"\\'); + + $options = array( + 'command="'.$cmd.'"', + 'no-port-forwarding', + 'no-X11-forwarding', + 'no-agent-forwarding', + 'no-pty', + ); + $options = implode(',', $options); + + $lines[] = $options.' '.$type.' '.$key."\n"; +} + +$authfile = implode('', $lines); + echo $authfile; + exit(0); diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index 77b666ea44..d8474085b2 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -3,7 +3,7 @@ final class PhabricatorAuthSSHKeyQuery extends PhabricatorCursorPagedPolicyAwareQuery { - const AUTHFILE_CACHEKEY = 'ssh.authfile'; + const AUTHSTRUCT_CACHEKEY = 'ssh.authstruct'; private $ids; private $phids; @@ -13,7 +13,7 @@ final class PhabricatorAuthSSHKeyQuery public static function deleteSSHKeyCache() { $cache = PhabricatorCaches::getMutableCache(); - $authfile_key = self::AUTHFILE_CACHEKEY; + $authfile_key = self::AUTHSTRUCT_CACHEKEY; $cache->deleteKey($authfile_key); } From 2951694c27370b501816f9eecff76e4cb65fb081 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 9 Aug 2018 12:30:42 -0700 Subject: [PATCH 5/6] Correctly spell 'committer' Summary: It's a funny word. h/t @joshuaspence Test Plan: Inspection of correct spelling. Reviewers: epriestley, joshuaspence Reviewed By: joshuaspence Subscribers: Korvin, joshuaspence Differential Revision: https://secure.phabricator.com/D19570 --- .../repository/storage/PhabricatorRepositoryCommit.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index abcf862b40..cf88c0eb54 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -206,7 +206,7 @@ final class PhabricatorRepositoryCommit return $this->assertAttached($this->authorIdentity); } - public function getCommiterIdentity() { + public function getCommitterIdentity() { return $this->assertAttached($this->committerIdentity); } From a6951a0a5aa0510b702f32ea68c014e99e174b18 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 9 Aug 2018 12:24:36 -0700 Subject: [PATCH 6/6] Add migration to encourage rebuilding repository identities Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them. Test Plan: - Ran migration, observed expected setup issue: {F5788217} - Ran `bin/config done identities` and observed setup issue get marked as done. - Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected. - Ran `./bin/config done reindex` and observed reindex issue cleared as expected. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T12164 Differential Revision: https://secure.phabricator.com/D19497 --- .../20180809.repo_identities.activity.php | 20 ++ .../PhabricatorManualActivitySetupCheck.php | 177 ++++++++++++------ .../PhabricatorConfigManualActivity.php | 2 + 3 files changed, 146 insertions(+), 53 deletions(-) create mode 100644 resources/sql/autopatches/20180809.repo_identities.activity.php diff --git a/resources/sql/autopatches/20180809.repo_identities.activity.php b/resources/sql/autopatches/20180809.repo_identities.activity.php new file mode 100644 index 0000000000..e1077d4ddb --- /dev/null +++ b/resources/sql/autopatches/20180809.repo_identities.activity.php @@ -0,0 +1,20 @@ +loadAllWhere('authorIdentityPHID IS NULL LIMIT 1'); + +if (!$commits) { + return; +} + +try { + id(new PhabricatorConfigManualActivity()) + ->setActivityType(PhabricatorConfigManualActivity::TYPE_IDENTITIES) + ->save(); +} catch (AphrontDuplicateKeyQueryException $ex) { + // If we've already noted that this activity is required, just move on. +} diff --git a/src/applications/config/check/PhabricatorManualActivitySetupCheck.php b/src/applications/config/check/PhabricatorManualActivitySetupCheck.php index 44db1b57a6..c5c756e49b 100644 --- a/src/applications/config/check/PhabricatorManualActivitySetupCheck.php +++ b/src/applications/config/check/PhabricatorManualActivitySetupCheck.php @@ -13,63 +13,134 @@ final class PhabricatorManualActivitySetupCheck foreach ($activities as $activity) { $type = $activity->getActivityType(); - // For now, there is only one type of manual activity. It's not clear - // if we're really going to have too much more of this stuff so this - // is a bit under-designed for now. + switch ($type) { + case PhabricatorConfigManualActivity::TYPE_REINDEX: + $this->raiseSearchReindexIssue(); + break; - $activity_name = pht('Rebuild Search Index'); - $activity_summary = pht( - 'The search index algorithm has been updated and the index needs '. - 'be rebuilt.'); + case PhabricatorConfigManualActivity::TYPE_IDENTITIES: + $this->raiseRebuildIdentitiesIssue(); + break; - $message = array(); - - $message[] = pht( - 'The indexing algorithm for the fulltext search index has been '. - 'updated and the index needs to be rebuilt. Until you rebuild the '. - 'index, global search (and other fulltext search) will not '. - 'function correctly.'); - - $message[] = pht( - 'You can rebuild the search index while Phabricator is running.'); - - $message[] = pht( - 'To rebuild the index, run this command:'); - - $message[] = phutil_tag( - 'pre', - array(), - (string)csprintf( - 'phabricator/ $ ./bin/search index --all --force --background')); - - $message[] = pht( - 'You can find more information about rebuilding the search '. - 'index here: %s', - phutil_tag( - 'a', - array( - 'href' => 'https://phurl.io/u/reindex', - 'target' => '_blank', - ), - 'https://phurl.io/u/reindex')); - - $message[] = pht( - 'After rebuilding the index, run this command to clear this setup '. - 'warning:'); - - $message[] = phutil_tag( - 'pre', - array(), - (string)csprintf('phabricator/ $ ./bin/config done %R', $type)); - - $activity_message = phutil_implode_html("\n\n", $message); - - $this->newIssue('manual.'.$type) - ->setName($activity_name) - ->setSummary($activity_summary) - ->setMessage($activity_message); + default: + } } + } + private function raiseSearchReindexIssue() { + $activity_name = pht('Rebuild Search Index'); + $activity_summary = pht( + 'The search index algorithm has been updated and the index needs '. + 'be rebuilt.'); + + $message = array(); + + $message[] = pht( + 'The indexing algorithm for the fulltext search index has been '. + 'updated and the index needs to be rebuilt. Until you rebuild the '. + 'index, global search (and other fulltext search) will not '. + 'function correctly.'); + + $message[] = pht( + 'You can rebuild the search index while Phabricator is running.'); + + $message[] = pht( + 'To rebuild the index, run this command:'); + + $message[] = phutil_tag( + 'pre', + array(), + (string)csprintf( + 'phabricator/ $ ./bin/search index --all --force --background')); + + $message[] = pht( + 'You can find more information about rebuilding the search '. + 'index here: %s', + phutil_tag( + 'a', + array( + 'href' => 'https://phurl.io/u/reindex', + 'target' => '_blank', + ), + 'https://phurl.io/u/reindex')); + + $message[] = pht( + 'After rebuilding the index, run this command to clear this setup '. + 'warning:'); + + $message[] = phutil_tag( + 'pre', + array(), + 'phabricator/ $ ./bin/config done reindex'); + + $activity_message = phutil_implode_html("\n\n", $message); + + $this->newIssue('manual.reindex') + ->setName($activity_name) + ->setSummary($activity_summary) + ->setMessage($activity_message); + } + + private function raiseRebuildIdentitiesIssue() { + $activity_name = pht('Rebuild Repository Identities'); + $activity_summary = pht( + 'The mapping from VCS users to Phabricator users has changed '. + 'and must be rebuilt.'); + + $message = array(); + + $message[] = pht( + 'The way Phabricator attributes VCS activity to Phabricator users '. + 'has changed. There is a new indirection layer between the strings '. + 'that appear as VCS authors and committers (such as "John Developer '. + '") and the Phabricator user that gets associated '. + 'with VCS commits. This is to support situations where users '. + 'are incorrectly associated with commits by Phabricator making bad '. + 'guesses about the identity of the corresponding Phabricator user. '. + 'This also helps with situations where existing repositories are '. + 'imported without having created accounts for all the committers to '. + 'that repository. Until you rebuild these repository identities, you '. + 'are likely to encounter problems with future Phabricator features '. + 'which will rely on the existence of these identities.'); + + $message[] = pht( + 'You can rebuild repository identities while Phabricator is running.'); + + $message[] = pht( + 'To rebuild identities, run this command:'); + + $message[] = phutil_tag( + 'pre', + array(), + (string)csprintf( + 'phabricator/ $ ./bin/repository rebuild-identities --all')); + + $message[] = pht( + 'You can find more information about this new identity mapping '. + 'here: %s', + phutil_tag( + 'a', + array( + 'href' => 'https://phurl.io/u/repoIdentities', + 'target' => '_blank', + ), + 'https://phurl.io/u/repoIdentities')); + + $message[] = pht( + 'After rebuilding repository identities, run this command to clear '. + 'this setup warning:'); + + $message[] = phutil_tag( + 'pre', + array(), + 'phabricator/ $ ./bin/config done identities'); + + $activity_message = phutil_implode_html("\n\n", $message); + + $this->newIssue('manual.identities') + ->setName($activity_name) + ->setSummary($activity_summary) + ->setMessage($activity_message); } } diff --git a/src/applications/config/storage/PhabricatorConfigManualActivity.php b/src/applications/config/storage/PhabricatorConfigManualActivity.php index 44c26f77dd..9952021575 100644 --- a/src/applications/config/storage/PhabricatorConfigManualActivity.php +++ b/src/applications/config/storage/PhabricatorConfigManualActivity.php @@ -7,6 +7,8 @@ final class PhabricatorConfigManualActivity protected $parameters = array(); const TYPE_REINDEX = 'reindex'; + const TYPE_IDENTITIES = 'identities'; + protected function getConfiguration() { return array(