1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-01 03:02:43 +01:00
Commit graph

1868 commits

Author SHA1 Message Date
Chad Little
5dd18a7ec1 Modernize PhortuneAccount with EditEngine/Modular Transactions
Summary: This updates the backend of PhortuneAccount to use EditEngine and Modular Transactions and updates language to "account manager" for clarity of role.

Test Plan:
- Wiped `phortune_account` table
- Visit Phortune, see new account automatically created.
- Edit name and managers
- Try to set no name or remove myself as a manager, get error messages
- Visit `/phortune/` and create another new account

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17585
2017-04-11 12:33:15 -07:00
Chad Little
28941b3105 Update PhortuneMerchant to Modular Transactions
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.

Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17584
2017-04-11 09:32:12 -07:00
epriestley
d1421bc3a1 Add "bin/storage optimize" to run OPTIMIZE TABLE on everything
Summary:
Even with `innodb_file_per_table` enabled, individual table files on disk don't normally shrink.

For most tables, like `maniphest_task`, this is fine, since the data in the table normally never shrinks, or only shinks a tiny amount.

However, some tables (like the "worker" and "daemon" tables) grow very large during a huge import but most of the data is later deleted by garbage collection. In these cases, this lost space can be reclaimed by running `OPTIMIZE TABLE` on the tables.

Add a script to `OPTIMIZE TABLE` every table.

My primary goal here is just to reduce storage pressure on `db001` since there are a couple of "import the linux kernel" installs on that host wasting a bunch of space. We're not in any trouble, but this should buy us a good chunk of headroom.

Test Plan: Ran `bin/storage optimize` locally and manually ran `OPTIMIZE TABLE` in production, saw tables get optimized.

Reviewers: chad

Reviewed By: chad

Subscribers: cspeckmim

Differential Revision: https://secure.phabricator.com/D17640
2017-04-08 15:15:49 -07:00
epriestley
3d816e94df Rename "PhabricatorHash::digest()" to "weakDigest()"
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17632
2017-04-06 15:43:33 -07:00
epriestley
3a3626834e Replace Remarkup calls to PhabricatorHash::digest() with SHA256
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.

Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.

I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.

Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17631
2017-04-06 15:43:18 -07:00
epriestley
d450a08890 Support HMAC+SHA256 with automatic key generation and management
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.

The new mechanism also automatically generates and stores HMAC keys.

Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.

We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.

Beyond that, this isn't something users should really have to think about or bother configuring.

Test Plan:
  - Added unit tests.
  - Used `bin/files integrity` to verify, strip, and recompute hashes.
  - Tampered with a generated HMAC key, verified it invalidated hashes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17630
2017-04-06 15:42:59 -07:00
epriestley
08a4225437 Provide "bin/files integrity" for debugging, maintaining and backfilling integrity hashes
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:

  - Verify: check that hashes match.
  - Compute: backfill missing hashes.
  - Strip: remove hashes. Useful for upgrading across a hash change.
  - Corrupt: intentionally corrupt hashes. Useful for debugging.
  - Overwrite: force hash recomputation.

Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.

I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.

Test Plan:
  - Ran the script in many modes against various files, saw expected operation, including:
  - Verified a file, corrupted it, saw it fail.
  - Verified a file, stripped it, saw it have no hash.
  - Stripped a file, computed it, got a clean verify.
  - Stripped a file, overwrote it, got a clean verify.
  - Corrupted a file, overwrote it, got a clean verify.
  - Overwrote a file, overwrote again, got a no-op.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17629
2017-04-06 15:42:43 -07:00
epriestley
7e6f37fffb Rename "ElasticSearch" filenames to "Elasticsearch" (2/2)
Sometimes git does some odd magic on case-insensitive filesystems, try to
trick it.

Auditors: chad
2017-04-02 14:59:36 -07:00
epriestley
a9e2732a5c Spell "Elasticsearch" correctly, not "ElasticSearch"
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.

Test Plan: `grep ElasticSearch`

Reviewers: chad, 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17601
2017-04-02 14:58:59 -07:00
epriestley
304d19f92a After a fulltext write to a particular service fails, keep trying writes to other services
Summary:
Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues.

Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update).

Test Plan:
  - Configured a bad ElasticSearch engine and a good MySQL engine.
  - Ran `bin/search index ... --force`.
  - Saw MySQL get updated even though ElasticSearch failed.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17599
2017-04-02 13:47:52 -07:00
epriestley
515cb98819 When running unit tests, ignore any custom task fields
Summary:
If you have `maniphest.custom-field-definitions` set to include "required" fields, a bunch of tests which create tasks can fail.

To avoid this, reset this config while running tests.

This mechanism should probably be more general (e.g., reset all config by default, only whitelist some config) but just fix this for now since it's a one-liner and doesn't make eventual cleanup any harder.

Test Plan: Ran `arc unit`, hitting tests that create tasks.

Reviewers: chad, 20after4

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17595
2017-04-02 09:36:17 -07:00
Daniel Stone
1c5503cb29 Custom fields: Render 'required' for tokenizer fields
Summary: When building a tokenizer-based edit control for a custom field (e.g. a datasource type), preserve a field validation error whilst building edit controls.

Test Plan:
  - Create custom datasource field, set it to required
  - Observe that 'Required' does not appear next to control
  - Apply patch
  - Observe 'Required' appears next to control

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17592
2017-04-02 15:26:26 +00:00
Mukunda Modell
cb1d904654 Make sure writes go to the right cluster
Summary:
Two little issues

1. there was an extra call to getHostForWrite,
2. The engine instance was shared between multiple service definitions so it
was overwriting the list of writable hosts from one service with hosts from another.

Test Plan:
tested in wikimedia production with multiple services defined like this:

```language=json
 [
        {
          "hosts": [
            {
              "host": "search.svc.codfw.wmnet",
              "protocol": "https",
              "roles": {
                "read": true,
                "write": true
              },
              "version": 5
            }
          ],
          "path": "/phabricator",
          "port": 9243,
          "type": "elasticsearch"
        },
        {
          "hosts": [
            {
              "host": "search.svc.eqiad.wmnet",
              "protocol": "https",
              "roles": {
                "read": true,
                "write": true
              },
              "version": 5
            }
          ],
          "path": "/phabricator",
          "port": 9243,
          "type": "elasticsearch"
        }
      ]
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17581
2017-03-30 18:08:05 +00:00
Mukunda Modell
654f0f6043 Make messages translatable and more sensible.
Summary:
These exception messages & comments didn't quite match reality.
Fixed and added pht() around a couple of them.

Test Plan: I didn't test this :P

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17578
2017-03-28 23:17:35 +00:00
epriestley
88798354e8 Soften a possible cluster search setup fatal
Summary:
Ref T12450. The way that config repair and setup issues interact is kind of complicated, and if `cluster.search` is invalid we may end up using `cluster.search` before we repair it.

I poked at things for a bit but wasn't confident I could get it to consistently repair before we use it without doing a big messy change.

The only thing that really matters is whether "type" is valid or not, so just put a slightly softer/more-tailored check in for that.

Test Plan:
  - With `"type": "elastic"`, loaded setup issues.
  - Before patch: hard fatal.
  - After patch: softer fatal with more useful messaging.

{F4321048}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17576
2017-03-28 15:28:16 -07:00
epriestley
5f939dcce0 Re-run config validation from bin/search
Summary:
Ref T12450. Normally, we validate config when:

  - You restart the webserver.
  - You edit it with `bin/config set ...`.
  - You edit it with the web UI.

However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings.

Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error.

Test Plan:
```
$ ./bin/search init
Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql.
```

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17574
2017-03-28 14:53:26 -07:00
epriestley
8879118b69 Fix a mid-air collision around SearchService roles
My D17571 didn't interact nicely with D17564, which added callsites for one of
the methods I removed.

Auditors: 20after4
2017-03-28 14:01:45 -07:00
epriestley
c40be811ea Fix isReadable() and isWritable() in SearchService
Summary:
Ref T12450. Minor cleanup:

  - setRoles() has no callers.
  - getRoles() has no callers (these two methods are leftovers from an earlier iteration of the change).
  - The `hasRole()` logic doesn't work since nothing calls `setRole()`.
  - `hasRole()` has only `isreadable/iswritable` as callers.
  - The `isReadable()/isWritable()` logic doesn't work since `hasRole()` doesn't work.

Instead, just check if there are any readable/writable hosts. `Host` already inherits its config from `Service` so this gets the same answer without any fuss.

Also add some read/write constants to make grepping this stuff a little easier.

Test Plan:
  - Grepped for all removed symbols, saw only newer-generation calls in `Host`.
  - See next diff for use of `isWritable()`.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17571
2017-03-28 13:58:46 -07:00
Mukunda Modell
699228c73b Address some New Search Configuration Errata
Summary:
  [ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading.
  [ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch?
  [ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade?
  [ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem.
  [ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003.
  [ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines?
  [ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations.
  [ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations.
  [ ] Does every assigned task now match "user" in ElasticSearch?
  [x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`.
  [ ] `PhabricatorSearchService` throws an untranslated exception.
  [ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)?
  [ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me.
  [ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it".
  [ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages.
  [ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config.
  [ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.".
  [x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing.
  [x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working.
  [x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working.
  [x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330).
  [x] The "index incorrect" warning UI uses inconsistent title case.
  [x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think).

refs T12450

Test Plan:
* Stared blankly at the code.
* Disabled 'write' role on mysql fulltext service.
* Edited a task, ran search indexer, verified that the mysql index wasn't being updated.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17564
2017-03-28 20:19:38 +00:00
Mukunda Modell
e41c25de50 Support multiple fulltext search clusters with 'cluster.search' config
Summary:
The goal is to make fulltext search back-ends more extensible, configurable and robust.

When this is finished it will be possible to have multiple search storage back-ends and
potentially multiple instances of each.

Individual instances can be configured with roles such as 'read', 'write' which control
which hosts will receive writes to the index and which hosts will respond to queries.

These two roles make it possible to have any combination of:

* read-only
* write-only
* read-write
* disabled

This 'roles' mechanism is extensible to add new roles should that be needed in the future.

In addition to supporting multiple elasticsearch and mysql search instances, this refactors
the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and
utilizes the same system for monitoring the health of elasticsearch nodes. This will
allow Wikimedia's phabricator to be redundant across data centers (mysql already is,
elasticsearch should be as well).

The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters
in different data centers) but reading from only one. Then toggling the 'read' property when
we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x)

Hopefully this is useful in the upstream as well.

Remaining TODO:

* test cases
* documentation

Test Plan:
(WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes.

Tested with elasticsearch versions 2.4 and 5.2 using the following config:

```lang=json
  "cluster.search": [
    {
      "type": "elasticsearch",
      "hosts": [
        {
          "host": "localhost",
          "roles": { "read": true, "write": true }
        }
      ],
      "port": 9200,
      "protocol": "http",
      "path": "/phabricator",
      "version": 5
    },
    {
      "type": "mysql",
      "roles": { "write": true }
     }
  ]

Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever.
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Tags: #elasticsearch, #clusters, #wikimedia

Differential Revision: https://secure.phabricator.com/D17384
2017-03-26 08:16:47 +00:00
epriestley
a41d158490 Only hibernate the Taskmaster after 15 seconds of inactivity
Under some workloads, the taskmaster may hibernate and launch more rapidly
than it should. Require 15 seconds of inactivity before hibernating. Also
hibernate for longer.

Auditors: chad
2017-03-25 05:01:32 -07:00
epriestley
2cda280cde Make the default Trigger hibernation 3 minutes instead of 5 seconds
The `min()` vs `max()` fix in D17560 meant that the Trigger daemon only
hibernates for 5 seconds, so we do a full GC sweep every 5 seconds. This ends
up eating a fair amount of CPU for no real benefit.

The GC cursors should move to persistent storage, but just bump this default
up in the meantime.

Auditors: chad
2017-03-25 04:14:32 -07:00
epriestley
8b553d2f18 Allow taskmaster daemons to hibernate
Summary: Ref T12298. Like PullLocal daemons, this allows the last daemon in the pool to hibernate if there's no work to be done, and awakens the pool when work arrives.

Test Plan:
  - Ran `bin/phd debug task --trace`.
  - Saw the pool hibernate and look for tasks.
  - Commented on an object.
  - Saw the pool wake up and process the queue.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17559
2017-03-24 13:51:37 -07:00
epriestley
f13637627d Improve daemon "waiting" message, config reload behavior
Summary:
Ref T12298. Two minor daemon improvements:

  - Make the "waiting" message reflect hibernation.
  - Don't trigger a reload right after launching.

Test Plan:
- Read "waiting" message.
- Ran "bin/phd start", didn't see an immediate SIGHUP in the log.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17550
2017-03-24 08:32:08 -07:00
epriestley
9099485a71 Allow the PullLocal daemon to hibernate, and wake it when repositories need an update
Summary: Ref T12298. This allows the PullLocal daemon to hibernate like the Trigger daemon, but automatically wakes it back up when it needs to do something.

Test Plan:
  - Ran `bin/phd debug pulllocal --trace`.
  - Saw the daemon hibernate after doing a checkup on repositories.
  - Saw periodic queries to look for new update messages.
  - After clicking "Update Now" in the web UI to schedule an update, saw the daemon wake up immediately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17540
2017-03-23 10:52:28 -07:00
epriestley
9326b4d131 Fix some range issues and 32-bit issues with avatar generation
Summary:
Ref T12444. A few issues:

   - `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
   - `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
   - `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
   - FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)

Test Plan:
  - Used `bin/people profileimage ... --force` to regenerate images.
  - Added some debugging to verify that the math seemed to be working.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12444

Differential Revision: https://secure.phabricator.com/D17543
2017-03-23 10:51:33 -07:00
epriestley
fab37aa4e3 When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.

There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.

Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.

In cases where project/package reviewers aren't in use, this doesn't change anything.

For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.

Test Plan:
  - Accepted normally.
  - Accepted a subset.
  - Tried to accept none.
  - Tried to accept bogus reviewers.
  - Accepted with myself not a reviewer
  - Accepted with only one reviewer (just got normal "this will be accepted" text).

{F4251255}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12271

Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 14:25:04 -07:00
epriestley
688c120f9f Provide PhabricatorEnv::isSelfURI to test if a URI points at the current install
Summary:
Ref T5378. This repackages an existing check to see if a URI is a URI for the current install into a more reasonable form.

In an upcoming change, I'll use this new check to test whether `http://example.whatever.com/T123` is a link to a task on the current install or not.

Test Plan: This stuff has good test coverage already; added some more.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5378

Differential Revision: https://secure.phabricator.com/D17502
2017-03-17 16:44:53 -07:00
epriestley
be16f9b2cd Add a generic "edge.search" method
Summary:
Ref T12337. Ref T5873. This provides a generic "edge.search" method which feels like other "verison 3" `*.search` methods.

The major issues here are:

  1. Edges use constants internally, which aren't great for an API.
  2. A lot of edges are internal and probably not useful to query.
  3. Edges don't have a real "id", so paginating them properly is challenging.

I've solved these things like this:

  - Edges must opt-in to being available via Conduit by providing a human-readable key (like "mention" instead of "52"). This solvs (1) and (2).
  - I faked a mostly-reasonable behavior for paginating.

Test Plan:
Ran various valid and invalid searches. Paginated a large search. Reviewed UI.

{F3651818}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12337, T5873

Differential Revision: https://secure.phabricator.com/D17462
2017-03-04 15:26:29 -08:00
Chad Little
f095a81b00 Allow custom image generation when choosing a profile image
Summary: Ref T10319. This swaps the default in the Picture Chooser to allow picking of the custom unique avatar. We're currently going with 100k unique possibilities. The logic roughly hashes a user name and picks an image pack, color, and border. Based on that, we select the first character of their username, or fall back to Psyduck if not [a-z][0-9].

Test Plan:
Set the following usernames from ProfilePicture as a test: chad, epriestley, sally, 007, _cat_, -doggie-.

{F3453979}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10319

Differential Revision: https://secure.phabricator.com/D17430
2017-03-03 20:21:31 -08:00
epriestley
90ec21f999 Add "--pool" and "--duration" flags to daemon CLI tools
Summary: Ref T12331. These changes are intended to make it easier to debug T12331 since I'm having difficulty reproducing the issue locally.

Test Plan:
  - Ran `bin/phd debug task --pool 4` and got an autoscaling pool.
  - Ran `bin/worker flood --duration 3` and got some 3-second-long tasks to execute with `bin/worker execute ...`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12331

Differential Revision: https://secure.phabricator.com/D17431
2017-02-28 07:43:46 -08:00
epriestley
40cc403d23 Allow the Trigger daemon to hibernate, reducing processes to 0
Summary:
Ref T12298. The trigger daemon already has routine long-term sleep, and few external events can impact when it should ideally wake up. The relevant events are:

  - Someone creates a new Nuance source (ideally, we should wake up right away and start polling it).
  - Someone creates a Calendar event about 16 minutes in the future (ideally, we should send them a reminder in about a minute).
  - Someone changes GC config to be extremely aggressive (ideally, we should immediately respect the change).

None of these cases are very important. We don't hibernate for more than 3 minutes, so the worst case is that your Nuance source takes 3 minutes to start importing or your Calendar notification comes two minutes too late (13 minutes before the event instead of 15).

This change makes GC sightly more CPU-expensive on average: currently, we do a GC sweep every 4 hours. After this change, we'll end up doing one every 3 minutes, because we lose the fact that we did a sweep recently when the daemon restarts.

We could fix this by keeping track of when the last GC sweep was in the database, instead of in the Daemon process, but the cost of a sweep is normally very small so I don't plan to do this anytime soon.

Test Plan:
  - Ran `bin/phd debug trigger`, saw daemon go through 3-minute hibernate + restart cycles.
  - Ran `bin/phd debug task`, saw daemon run normally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17408
2017-02-24 10:54:05 -08:00
epriestley
ad032e72ca When the profiler is active, keep it active if the user submits forms
Summary: Ref T12297. When a page is generated with the profiler active, keep it active by adding a `__profile__` input to any forms we generate.

Test Plan: Hit Conduit API page with `__profile__` active, saw it reflected in forms.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12297

Differential Revision: https://secure.phabricator.com/D17399
2017-02-23 11:15:40 -08:00
Chad Little
bf44210dc8 Reduce application search engine results list for Dashboards
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels

Test Plan: Add a query panel, see less options.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10390

Differential Revision: https://secure.phabricator.com/D17341
2017-02-22 12:42:43 -08:00
Jakub Vrana
9f3cde4db7 Fix errors found by PHPStan
Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17377
2017-02-18 09:24:56 +00:00
Jakub Vrana
a778151f28 Fix errors found by PHPStan
Test Plan: Ran `phpstan analyze -a autoload.php phabricator/src`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D17371
2017-02-17 10:10:15 +00:00
epriestley
b11138a16b Remove extra parameter in newStandardEditField() call
Summary: See D14617. This could probably go either way but we don't currently need `$engine` in `newStandardEditField()`, so just get rid of it.

Test Plan: Edited a task with standard custom fields defined.

Reviewers: vrana, chad

Reviewed By: vrana

Differential Revision: https://secure.phabricator.com/D17370
2017-02-16 09:44:39 -08:00
Chad Little
4176bdeb5b Allow task graph task titles to go full width
Summary: Fixes T12213. Removes truncation and allows titles to be full width if needed.

Test Plan:
Chrome / Firefox / Safari on Mac, mobile and desktop widths.

{F2754679}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12213

Differential Revision: https://secure.phabricator.com/D17336
2017-02-10 10:58:15 -08:00
epriestley
743dc9fdb5 Stop "Header" fields (labels for form sections) from trying to generate Conduit edits
Summary: Fixes T12236. Headers are currently trying to generate an edit transaction for `maniphest.edit` and similar, but should not, since you can't edit them.

Test Plan:
  - Configured Maniphest with a custom header field.
  - Before change: `maniphest.edit` API console page fataled.
  - After change: all good, no weird "header" transaction.
  - Header still shows up on "Edit Task" form in web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12236

Differential Revision: https://secure.phabricator.com/D17332
2017-02-10 07:39:39 -08:00
epriestley
bc41c3f5a5 Use DifferentialCommitMessageParser and Modular Transactions to implement "Auditors: ..."
Summary:
Ref T10978. Updates how we implement "Auditors: ..." in commit messages:

  - Use the same parsing code as everything else.
    - (Also: parse package names.)
  - Use the new transaction code.

Also, fix some UI strings.

Test Plan: Used `bin/repository reparse --herald <commit>` to re-run this code on commits with various messages (valid Auditors, invalid Auditors, no Auditors). Saw appropriate auditors added in the UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17262
2017-01-30 15:23:05 -08:00
epriestley
f8cebdc418 Make Differential inline events actually trigger comment preview refreshes
Summary:
Earlier, I made some changes so that when you create or edit an inline, the comment at the bottom of the page updates (even though you didn't fiddle with the stacked actions inputs).

At the last second I broke them by spelling this wrong while cleaning things up, so they didn't actually work. Spell the property correctly ("showPreview", not "shouldPreview").

Also, we have some JS which rewrites "Not Visible" into "View", but it fires in an inconvenient way now and is flickery for me. Ideally this should get cleaned up slightly better eventualy, but at least make is stop doing so much flickery layout for now.

Test Plan:
  - Wrote no comment on a revision.
  - Added an inline.
  - Saw comment preview properly update immediately.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17229
2017-01-19 12:32:49 -08:00
epriestley
48187cdbbe Fix an unusual nonterminating task graph node
Summary:
Fixes T12114. There were a couple of bugs here:

  - We could draw too many joining lines if a node had a parent with multiple descendants.
  - We could incorrectly ignore columns because of an `unset()`.

I //think// this fixes both things without collateral damage. This whole thing is a little hard to understand/debug and has grown beyond its original scope, so I'll probably rewrite it if there are more issues.

Test Plan:
  - Unit tests.
  - My local repro is clean now:

{F2424920}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12114

Differential Revision: https://secure.phabricator.com/D17211
2017-01-17 09:18:26 -08:00
epriestley
b5722a9963 Use EditEngine stacked comments in Diffusion
Summary: Ref T10978. Ref T8739. Fixes T10446. Converts Diffusion to modern comment/preview code, like Differential.

Test Plan: {F2342933}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978, T10446, T8739

Differential Revision: https://secure.phabricator.com/D17183
2017-01-11 14:46:48 -08:00
epriestley
00313094d3 Make "View" links on Differential inline comment previews work again
Summary:
Ref T11114. Recent changes broke the links to jump to inline comments from the previews because they get hooked up with JS.

Restore the linking behavior.

Test Plan: Clicked "View" on an inline comment preview, jumped to that comment.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17131
2017-01-02 13:24:02 -08:00
epriestley
18249b097f Make inline comment preview and submission mostly work on EditEngine
Summary: Ref T11114. This comments nearly working on EditEngine. Only significant issue I caught is that the "View" link doesn't render properly because it depends on JS which is tricky to hook up. I'll clean that up in a future diff.

Test Plan: {F2279201}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17116
2016-12-31 10:10:29 -08:00
epriestley
914d9fa8b9 Simplify Auditors custom field in Differential
Summary: Ref T11114. This field just stores the value of "Auditors" so you can trigger auditors explicitly later on if you want.

Test Plan: Created and edited revisions with "Auditors".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17070
2016-12-16 10:09:30 -08:00
epriestley
102ea3cfa4 Replace Differential Edit controller with EditEngine-driven EditPro controller
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.

This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.

Test Plan:
  - Created a revision via web diffs.
  - Updated a revision via web diffs.
  - Edited a revision via web.
  - Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
  - Created a revision via CLI.
  - Updated a revision via CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17054
2016-12-14 07:27:39 -08:00
epriestley
eda64b8549 Add a very basic EditPro controller for Differential
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.

Test Plan: {F2117302}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17043
2016-12-13 14:36:06 -08:00
epriestley
237f94b830 Fix flaky subscribers policy rule unit test
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.

We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.

Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).

If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.

Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.

Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17029
2016-12-11 12:27:57 -08:00
epriestley
6058d3305f Normalize remote IP addresses when writing to logs, etc
Summary:
Ref T11939. IPv4 addresses can normally only be written in one way, but IPv6 addresses have several formats.

For example, the addresses "FFF::", "FfF::", "fff::", "0ffF::", "0fFf:0::", and "0FfF:0:0:0:0:0:0:0" are all the same address.

Normalize all addresses before writing them to logs, etc, so we store the most-preferred form ("fff::", above).

Test Plan:
Ran an SSH clone over IPv6:

```
$ git fetch ssh://local@::1/diffusion/26/locktopia.git
```

It worked; verified that address read out of `SSH_CLIENT` sensibly.

Faked my remote address as a non-preferred-form IPv6 address using `preamble.php`.

Failed to login, verified that the preferred-form version of the address appeared in the user activity log.

Made IPv6 requests over HTTP:

```
$ curl -H "Host: local.phacility.com" "http://[::1]/"
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11939

Differential Revision: https://secure.phabricator.com/D16987
2016-12-05 11:20:29 -08:00
epriestley
5a060b34df Add IPv6 reserved addresses to the default outbound blacklist
Summary:
Ref T11939. Depends on D16984. Now that CIDRLists can contain IPv6 addresses, blacklist all of the reserved IPv6 space.

This reserved blacklist is used to prevent users from accessing internal services via "Import Calendar" or "Add Macro".

They can't actually reach IPv6 addresses via these mechanisms yet because we need to do more work to support outbound IPv6 requests, but make sure reserved IPv6 space is blacklisted already when that support eventaully arrives.

Also, clean up some error messages (e.g., for trying to hit a bad URI in "Add Macro").

Test Plan:
  - Loaded pages with default blacklist.
  - Tried to make requests into IPv6 space.
  - Currently, this is impossible because of `parse_url()` and `gethostynamel()` calls.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11939

Differential Revision: https://secure.phabricator.com/D16986
2016-12-05 11:20:13 -08:00
epriestley
29a3cd5121 Add "Manual Activities", to tell administrators to rebuild the search index
Summary:
Ref T11922. After updating to HEAD of `master`, you need to manually rebuild the index. We don't do this during `bin/storage upgrade` because it can take a very long time (`secure.phabricator.com` took roughly an hour) and can happen while Phabricator is running.

However, if we don't warn users about this they'll just get a broken index unless they go read the changelog (or file an issue, then we tell them to go read the changelog).

This adds a very simple table for notes to administrators so we can write a "you need to go rebuild the index" note, then adds one.

Administrators clear the note by completing the activity and running `bin/config done reindex`. This isn't automatic because there are various strategies you can use to approach the issue, which I'll discuss in greater detail in the linked documentation.

Also, fix an issue where `bin/storage upgrade --apply <patch>` could try to re-mark an already-applied patch as applied.

Test Plan:
  - Ran storage ugrades.
  - Got instructions to rebuild search index.
  - Cleared instructions with `bin/config done reindex`.

Reviewers: chad

Reviewed By: chad

Subscribers: avivey

Maniphest Tasks: T11922

Differential Revision: https://secure.phabricator.com/D16965
2016-11-30 11:23:54 -08:00
epriestley
b2cdebefea Fix two errors from the error logs
Summary: Found these in the `secure` error logs: one bad call, one bad column.

Test Plan: Searched for empty string. Double-checked method name.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16948
2016-11-26 07:50:57 -08:00
epriestley
9d0752063e Allow bin/storage adjust to adjust table engines
Summary:
Ref T11741. On recent-enough versions of MySQL, we would prefer to use InnoDB for fulltext indexes instead of MyISAM.

Allow `bin/storage adjust` to read actual and expected table engines, and apply adjustments as necessary.

We have one existing bad table that uses the wrong engine, `metamta_applicationemail`. This change corrects that table.

Test Plan:
  - Ran `bin/storage upgrade`.
  - Saw the adjustment phase apply this change properly:

```
>>>[463] <query> ALTER TABLE `local_metamta`.`metamta_applicationemail` COLLATE = 'utf8mb4_bin', ENGINE = 'InnoDB'
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16941
2016-11-25 15:13:40 -08:00
epriestley
ff3333548f Create and populate a stopwords table for InnoDB fulltext indexes to use in the future
Summary:
Ref T11741. InnoDB uses a stopwords table instead of a stopwords file.

During `storage upgrade`, synchronize the table from the stopwords file on disk.

Test Plan:
  - Ran `storage upgrade`.
  - Ran `select * from stopwords`, saw stopwords.
  - Added some garbage to the table.
  - Ran `storage upgrade`, saw it remove it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16940
2016-11-25 15:13:08 -08:00
epriestley
ce18a8e208 Fix two setup issues arising from partitioning support
Summary:
Ref T11044.

  - Use shorter lock names. Fixes T11916.
  - These granular exceptions now always raise as a more generic "Cluster" exception, even for a single host, because there's less special code around running just one database.

Test Plan:
  - Configured bad `mysql.port`, ran `bin/storage upgrade`, got a more helpful error message.
  - Ran `bin/storage upgrade --trace`, saw shorter lock names.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044, T11916

Differential Revision: https://secure.phabricator.com/D16924
2016-11-23 07:20:29 -08:00
epriestley
4dadad53ae Prevent media from autoplaying when rendered as a feed story
Summary: Fixes T11845. Users can still embed a text panel on the home page to give it some ambiance.

Test Plan: Wrote an autoplay video as a comment, saw it in feed. Before change: autoplay. After change: no auto play. On task: still autoplay.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11845

Differential Revision: https://secure.phabricator.com/D16920
2016-11-22 14:19:56 -08:00
epriestley
0ed767b967 Fix a couple of partition migration bugs
Summary:
Ref T11044. Few issues here:

  - The `PhutilProxyException` is missing an argument (hit this while in read-only mode).
  - The `$ref_key` is unused.
  - When you add a new master to an existing cluster, we can incorrectly apply `.php` patches which we should not reapply. Instead, mark them as already-applied.

Test Plan:
  - Poked this locally, but will initialize `secure004` as an empty master to be sure.

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16916
2016-11-22 10:57:24 -08:00
epriestley
8c89fc38fc Allow persistent connections to be configured per database host
Summary: Ref T11044. Fixes T11672. In T11672, persistent connections seem to work fine, but they can require `max_connections` and other settings to be raised. Since most users don't need them, make them an advanced option.

Test Plan: Configured persistent connections, loaded some pages, observed persistent connections get used.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044, T11672

Differential Revision: https://secure.phabricator.com/D16913
2016-11-22 10:55:45 -08:00
epriestley
f89f708692 Apply storage patches patch-by-patch, not database-by-database
Summary:
Ref T11044. Sometimes we have a sequence of patches like this:

  - `01.newtable.sql`: Adds a new table to Files.
  - `02.movedata.php`: Moves some data that used to live in Tokens to the new table.

This is fairly rare, but not unheard of. More commonly, we can have this sequence:

  - `03.newtable.sql`: Add a new column to Phame.
  - `04.setvalue.php`: Copy data into that new column.

In both cases, when applying database-by-database, we can get into trouble.

  - In the first case, if Files is on a different master, we'll try to move data into a new table before creating the table.
  - In the second case, if Phame is on a different master, the PHP patch will connect to it before we add the new column.

In either case, we try to interact with tables or columns which don't exist yet.

Instead, apply each patch in order, to all databases which need it. So we'll apply `01.newtable.sql` EVERYWHERE first, then move on.

In the case of PHP patches, we also now only apply them once, since they never make schema changes. It should normally be OK to apply them more than once safely, but this is a little faster and a little safer if we ever make a mistake.

Test Plan:
  - Ran `bin/storage upgrade` on single-host and clustered setups.
  - Initialized new storage on single-host and clustered setups.
  - Upgraded again after initialization.
  - Ran with `--apply`.
  - Ran with `--dry-run`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16912
2016-11-22 09:24:58 -08:00
epriestley
e6bfa1bd23 Remove "mysql.configuration-provider" configuration option
Summary:
Ref T11044. This was old Facebook cruft for reading configuration from SMC (and maybe doing some other questionable things). See D183.

(See also D175 for discussion of this from 2011.)

In modern Phabricator, you can subclass `SiteConfig` to provide dynamic configuration, and we do so in the Phacility cluster. This lets you change any config, and change in response to requests (e.g., for instancing) and is generally more powerful than this mechanism was.

This configuration provider theoretically let you roll your own replication or partitioning, but in practice I believe no one ever did, and no one ever could have anyway without more support in the upstream (for migrations, read-after-write, etc).

Test Plan:
  - Grepped for removed option.
  - Browsed around with clustering off.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16911
2016-11-22 09:24:46 -08:00
epriestley
4da74166fe When storage is partitioned, refuse to serve requests unless web and databases agree on partitioning
Summary:
Ref T11044. One popular tool in a modern operations environment is Puppet. The primary purpose of this tool is to randomly revert hosts to older or different configurations.

Introducing an element of chaotic unpredictability into operations trains staff to be on high alert at all times, rather than lulled into complacency by predictability or consistency.

When Puppet reverts a Phabricator host's configuration to an older version, we might start writing data to a lot of crazy places where it shouldn't go. This will create a big sticky mess that is virtually impossible to undo, mostly because we'll get two files with ID 123 or two tasks with ID 456 or whatever else and good luck with that.

Instead, after changing the partition layout, require `bin/storage partition` to be run. This writes a copy of the config everywhere.

Then, when we start serving web requests, make sure every database has the exact same config. This will foil Puppet by refusing to run requests on hosts it has reverted.

Test Plan:
  - Changed partition configuration.
  - Ran Phabricator.
  - FOILED!
  - Ran `bin/storage partition` to sync config.
  - Things worked again.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16910
2016-11-22 04:15:46 -08:00
epriestley
bac27fb403 Remove "mysql.implementation" configuration
Summary:
Ref T11044. Fixes T10931. This option has essentially never been useful for anything, and we've picked the best implementation for a long time (MySQLi if available, MySQL if not).

I am not aware of any reason to ever set this manually. If someone comes up with some bizarre but legitimate use case that I haven't thought of, we can modularize it.

Test Plan: Browsed around. Grepped for `mysql.implementation`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10931, T11044

Differential Revision: https://secure.phabricator.com/D16909
2016-11-22 04:15:34 -08:00
Josh Cox
ac66522c2e Add a flag to ./bin/worker to select tasks based on their failureCount
Summary:
I frequently run into a situation where I want to kill tasks that have accumulated a lot of failures regardless of what class they are. Or I'll want to kill every worker of a certain class but only if it has failed at least once. This change allows me to run `./bin/worker cancel --class <MYCLASS> --min-failure-count 5` to only kill tasks with at least 5 failed attempts.

The `--min-failure-count N` argument can be used by itself as well as with `--class CLASSNAME`. I don't think it makes sense for it to work with `--id ID`, but I'm not dead set on that or anything.

Test Plan: I ran the worker management workflow with and without the `--min-failure-count` argument and it worked as expected.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley, yelirekim

Differential Revision: https://secure.phabricator.com/D16906
2016-10-12 09:49:29 -04:00
epriestley
bcfd515b32 Run all minor setup checks on all configured database hosts
Summary:
Fixes T10759. Fixes T11817. This runs all the general sanity/configuration checks on all the active servers.

None of these warnings are very important, and this doesn't change any logical stuff.

Depends on D16904.

Test Plan: Painstakingly triggered each warning, verified that they rendered correctly and that messages told me which host was affected.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10759, T11817

Differential Revision: https://secure.phabricator.com/D16905
2016-11-21 15:55:54 -08:00
epriestley
326d5bf800 Detect replicating masters and fatal (also, warn on nonreplicating replicas)
Summary:
Ref T10759. Check master/replica status during startup.

After D16903, this also means that we check this status after a database comes back online after being unreachable.

If a master is replicating, fatal (since this can do a million kinds of bad things).

If a replica is not replicating, warn (this just means the replica is behind so some data is at risk).

Also: if your masters were actually configured properly (mine weren't until this change detected it), we would throw away patches as we applied them, so they would only apply to the //first// master. Instead, properly apply all migration patches to all masters.

Test Plan:
  - Started Phabricator with a replicating master, got a fatal.
  - Stopped replication on a replica, got a warning.
  - With two non-replicating masters, upgraded storage.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10759

Differential Revision: https://secure.phabricator.com/D16904
2016-11-21 15:55:22 -08:00
epriestley
78040e0ff5 Run "DatabaseSetup" checks against all configured hosts
Summary:
Ref T10759. Currently, these checks run only against configured masters. Instead, check every host.

These checks also sort of cheat through restart during a recovery, when some hosts will be unreachable: they test for "disaster" by seeing if no masters are reachable, and just skip all the checks in that case.

This is bad for at least two reasons:

  - After recent changes, it is possible that //some// masters are dead but it's still OK to start. For example, "slowvote" may have no master, but everything else is reachable. We can safely run without slowvote.
  - It's possible to start during a disaster and miss important setup checks completely, since we skip them, get a clean bill of health, and never re-test them.

Instead:

  - Test each host individually.
  - Fundamental problems (lack of InnoDB, bad schema) are fatal on any host.
  - If we can't connect, raise it as a //warning// to make sure we check it later. If you start during a disaster, we still want to make sure that schemata are up to date if you later recover a host.

In particular, I'm going to add these checks soon:

  - Fatal if a "master" is replicating.
  - Fatal if a "replica" is not replicating.
  - Fatal if a database partition config differs from web partition config.
  - When we let a database off with a warning because it's down, and later upgrade it to a fatal because we discover it is broken after it comes up again, fatal everything. Currently, we keep running if we "discover" the presence of new fatals after surviving setup checks for the first time.

Test Plan:
  - Configured with multiple masters, intentionally broke one (simulating a disaster where one master is lost), saw Phabricator still startup.
  - Tested individual setup checks by intentionally breaking them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10759

Differential Revision: https://secure.phabricator.com/D16902
2016-11-21 15:49:07 -08:00
epriestley
55e21565b5 Support application partitioning across multiple masters
Summary:
Ref T11044. I'm going to hold this until after the release cut, but I think it's good to go.

This allows installs to configure multiple masters in `cluster.databases` and partition applications across them (for example, put Maniphest on a dedicated database).

When we make a Maniphest connection we go look up which master we should be hitting first, then connect to it.

This has at least approximately been planned for many years, so the actual change is largely just making sure that your config makes sense.

Test Plan:
  - Configured `db001.epriestley.com` and `db002.epriestley.com` as master/master.
  - Partitioned applications between them.
  - Interacted with various applications, saw writes go to the correct host.
  - Viewed "Database Servers" and saw partitioning information.
  - Ran schema upgrades.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16876
2016-11-19 14:14:39 -08:00
epriestley
79132311f4 Generate slightly shorter summaries in the typeahead browse dialog
Summary: Ref T11034. Try to produce a roughly-one-sentence summary instead of a roughly-one-paragraph summary for the browse dialog.

Test Plan:
  - Added unit tests, ran unit tests.
  - Wrote a longer summary for a project, browsed to it, saw a shorter summary.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11034

Differential Revision: https://secure.phabricator.com/D16892
2016-11-17 13:08:08 -08:00
epriestley
c7f2e4a924 Document calendar summary icons
Summary:
Fixes T11809. Ref

  - Explicitly document the summary icon hints -- I don't think these are too hard to figure out (and maybe this stuff should just go in the tooltips) but we can start here.
  - Use color + shape to distinguish between "cancelled" and "declined", not just color (for users with vision accessibility issues).
  - Translate a "minute(s)" string into sensible English.
  - Use RSVP status on the month view green circle thing.

Test Plan:
  - Read docs.
  - Looked at month view.
  - Read reminder mail.
  - Viewed month view mobile view.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11809

Differential Revision: https://secure.phabricator.com/D16872
2016-11-15 13:44:20 -08:00
epriestley
558d194302 Update bin/storage workflows to accommodate multiple masters
Summary: Depends on D16847. Ref T11044. This updates the remaining storage-related workflows from the CLI to accommodate multiple masters.

Test Plan:
  - Configured multiple masters.
  - Ran all `bin/storage` workflows.
  - Ran `arc unit --everything`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16848
2016-11-12 16:37:47 -08:00
epriestley
bc15eee3f2 Update SchemaQuery and the web UI to accommodate multiple master databases
Summary:
Depends on D16115. Ref T11044. In the brave new world of multiple masters, we need to check the schemata on each master when looking for missing storage patches, keys, schema changes, etc.

This realigns all the "check out what's up with that schema" calls to work for multiple hosts, and updates the web UI to include a "Server" column and allow you to browse per-server.

This doesn't update `bin/storage`, so it breaks things on its own (and unit tests probably won't pass). I'll update that in the next change.

Test Plan: Configured local environment in cluster mode with multiple masters, saw both hosts' status reported in web UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16847
2016-11-12 16:36:52 -08:00
epriestley
ecc598f18d Support multiple database masters and convert easy callers
Summary:
Ref T11044. This moves toward partitioned application databases:

  - You can define multiple masters.
  - Convert all the easily-convertible code to become multi-master aware.

This doesn't convert most of `bin/storage` or "Config > Database (Stuff)" yet, as both are quite involved. They still work for now, but only operate on the first master instead of all masters.

Test Plan: Configured multiple masters, browsed around, ran `bin/storage` commands, ran `bin/storage --host ...`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11044

Differential Revision: https://secure.phabricator.com/D16115
2016-11-12 16:30:20 -08:00
epriestley
706c21375e Remove empty implementations of describeAutomaticCapabilities()
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:

  - Rebuild Celerity map to fix grumpy unit test.
  - Fix one issue on the policy exception workflow to accommodate the new code.

Test Plan:
  - `arc unit --everything`
  - Viewed policy explanations.
  - Viewed policy errors.

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D16831
2016-11-09 15:24:22 -08:00
epriestley
9803674525 Extract variable type information from pht() calls
Summary:
Ref T5267. When extrating data from `pht()` calls, also extract the argument types and export them into the map so they can be used by consumers.

We recognize plurals (`phutil_count()`, `new PhutilNumber`) and genders (`phutil_person()`). We'll need to annotate the codebase for those, since they're currently runtime-only.

Test Plan:
Rebuilt extraction maps, got data like this (note "number" type annotation).

```
  "Scaling pool \"%s\" up to %s daemon(s).": {
    "uses": [
      {
        "file": "/daemon/PhutilDaemonOverseer.php",
        "line": 378
      }
    ],
    "types": [
      null,
      "number"
    ]
  },
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5267

Differential Revision: https://secure.phabricator.com/D16823
2016-11-08 08:33:15 -08:00
epriestley
a8866c0b31 In Maniphest, don't render the task graph drawing if we're only showing parents/children
Summary:
Ref T4788. I thought I implemented this, but actualy didn't.

When we're in the "mid-sized" fallback mode (graph has more than 100 nodes, but not more than than 100 parents/children), don't actually draw the graph. It's almost always uninteresting and huge.

Instead, this just renders a list of direct parents, then the task, then the direct children, which is pretty straightforward.

Test Plan: Set limit to 5, saw mid-sized fallback graph with no actual graph drawing.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4788

Differential Revision: https://secure.phabricator.com/D16816
2016-11-07 11:09:20 -08:00
epriestley
960c0be689 Fix some issues with Phabricator i18n string extraction
Summary: Ref T5267. Fix one minor bug (paths were not being resolved properly) and one minor string issue (missing `%d` in a string).

Test Plan: Extracted strings, got a cleaner result.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5267

Differential Revision: https://secure.phabricator.com/D16808
2016-11-06 11:12:45 -08:00
epriestley
6b16f930c4 Automatically send (not-so-great) email notifications for upcoming events
Summary: Ref T7931. This is still quite rough, but should technically send vaguely-useful email as part of the standard trigger infrastructure.

Test Plan: Ran `bin/phd start`, created an event shortly, saw reminder email send in `bin/mail list-outbound`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7931

Differential Revision: https://secure.phabricator.com/D16784
2016-11-01 13:24:40 -07:00
epriestley
1993005651 Fix a variant translation issue
Summary: Fixes T11799. This string is varying on the first parameter, but should vary on the second parameter.

Test Plan: Ran `bin/garbage set-policy ...`, saw proper translation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11799

Differential Revision: https://secure.phabricator.com/D16769
2016-10-29 17:12:33 -07:00
epriestley
0f800a3cd8 In Phabricator, drop PhutilClassMap caches after loading additional libraries
Summary:
Depends on D16755. Right now, we build a setup check map (to run preflight checks), then later load libraries.

This means any checks included in third-party libraries don't get added to the map, and no longer run.

(These are rare, but Phacility has a couple).

Instead, delete the caches after loading extra libraries.

Test Plan: With this and D16755, re-ran setup checks and saw Phacility setup checks run.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16756
2016-10-26 15:46:33 -07:00
epriestley
7678f412be Hold a lock while collecting garbage
Summary:
Fixes T11771. Adds a lock around each GC process so we don't try to, e.g., delete old files on two machines at once just because they're both running trigger daemons.

The other aspects of this daemon (actual triggers; nuance importers) already have separate locks.

Test Plan: Ran `bin/phd debug trigger --trace`, saw daemon acquire locks and collect garbage.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11771

Differential Revision: https://secure.phabricator.com/D16739
2016-10-20 13:40:00 -07:00
epriestley
272046ae77 Write a basic SSH pull log for Git
Summary: Ref T11766. When users run `git pull` or similar, log the operation in the pull log.

Test Plan: Performed SSH pulls, got a log in the database. Today, this event log is purely diagnostic and has no UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11766

Differential Revision: https://secure.phabricator.com/D16738
2016-10-20 13:39:30 -07:00
epriestley
a3253f78ce Make query engines "overheat" instead of stalling when filtering too many results
Summary: Ref T11773. This is an initial first step toward a more complete solution, but should make the worst case much less bad: prior to this change, the worst case was "30 second exeuction timeout". After this patch, the worst case is "no results + explanatory message", which is strictly better.

Test Plan:
Made all feed stories fail policy checks, loaded home page.

  - Before adding overheating: 9,600 queries / 20 seconds
  - After adding overheating: 376 queries / 800ms

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11773

Differential Revision: https://secure.phabricator.com/D16735
2016-10-20 09:31:37 -07:00
epriestley
49448a87c1 Rough in most of Calendar exports
Summary:
Ref T10747. Rough flow is:

  - Run a query.
  - Select a new "Export Events..." action.
  - This lets you define an "Export", which has a unique URL you can paste into Google Calendar or Calendar.app or whatever.

Most of this does nothing yet but here's the boilerplate.

Test Plan: Doesn't do anything yet.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10747

Differential Revision: https://secure.phabricator.com/D16675
2016-10-06 04:06:35 -07:00
epriestley
38b10f05a2 For now, disable persistent connections and the "max_connections" setup warning
Summary:
Ref T11672. At low loads, this causes us to use more connections, which is pushing some installs over the default limits.

Rather than trying to walk users through changing `max_connections`, `open_files_limit`, `fs.file-max`, `ulimit`, etc., just put things back for now. After T11044 we should have headroom to use persistent connections within the default limits on all reasonable systems..

Test Plan: Loaded Phabricator, poked around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11672

Differential Revision: https://secure.phabricator.com/D16591
2016-09-23 12:42:26 -07:00
epriestley
66c7f22c27 Truncate and scroll task graph tables instead of fitting task titles to the display
Summary: Fixes T11676. Instead of trying to fit task titles to the display, truncate them and let the table scroll.

Test Plan:
Table now scrolls when cramped:

{F1843396}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11676

Differential Revision: https://secure.phabricator.com/D16583
2016-09-21 18:22:26 -07:00
epriestley
db2425b300 Do initial repository imports at a lower priority and finish importing commits before starting new ones
Summary:
Fixes T11677. This makes two minor adjustments to the repository import daemons:

  - The first step ("Message") now queues at a slightly-lower-than-default (for already-imported repositories) or very-low (for newly importing repositories) priority level.
  - The other steps now queue at "default" priority level. This is actually what they already did, but without this change their behavior would be to inherit the priority level of their parents.

This has two effects:

  - When adding new repositories to an existing install, they shouldn't block other things from happening anymore.
  - The daemons will tend to start one commit and run through all of its steps before starting another commit. This makes progress through the queue more even and predictable.
    - Before, they did ALL the message tasks, then ALL the change tasks, etc. This works fine but is confusing/uneven/less-predictable because each type of task takes a different amount of time.

Test Plan:
  - Added a new repository.
  - Saw all of its "message" steps queue at priority 4000.
  - Saw followups queue at priority 2000.
  - Saw progress generally "finish what you started" -- go through the queue one commit at a time, instead of one type of task at a time.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11677

Differential Revision: https://secure.phabricator.com/D16585
2016-09-21 16:41:01 -07:00
epriestley
8941bbfcea Make "text" custom fields appear in ApplicationSearch again
Summary:
Fixes T11675. This capability was erroneously (probably?) removed in D14766.

This search implementation (which uses exact match) probably isn't perfect for all cases of "text" fields, but empirically it seems to be what a significant number of users are after.

Test Plan:
Searched for a custom text field value.

{F1843383}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11675

Differential Revision: https://secure.phabricator.com/D16582
2016-09-21 16:39:43 -07:00
epriestley
f8c2225268 Use persistent database connections from web contexts
Summary:
Ref T11672. Depends on D16577. When establishing a connection from a webserver context, try to use persistent connections.

The hope is that this will fix outbound port exhaustion issues experienced on repository hosts handling large queue volumes.

Test Plan:
Added this to a page:

```lang=php
    $tables = array(
      new PhabricatorUser(),
      new ManiphestTask(),
      new DifferentialRevision(),
      new PhabricatorRepository(),
      new PhabricatorPaste(),
    );

    $ids = array();
    foreach ($tables as $table) {
      $conn = $table->establishConnection('r');

      $cid = queryfx_one(
        $conn,
        'SELECT CONNECTION_ID() cid');

      $ids[get_class($table)] = $cid['cid'];
    }

    var_dump($ids);
```

Reloaded the page a bunch of times and saw no reissued connections (the pool seems to keep a particular connection bound to a particular database), but did see connection reuse across requests.

That is, across reloads the same connection IDs appeared, but the same connection ID never appeared twice in the same request. This is what we want.

Also googled for issues with persistent connections, but everything I found was unconcerning and obscure (local variables and other very complex state that we don't use), and a bunch of the docs are reassuring (transactions, etc., get reset properly).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11672

Differential Revision: https://secure.phabricator.com/D16578
2016-09-21 14:46:28 -07:00
epriestley
1ee426e4ac Add a specialized cache for storing "has setup ever worked?"
Summary:
Ref T11613. In D16503/T11598 I refined the setup flow to improve messaging for early-stage setup issues, but failed to fully untangle things.

We sometimes still try to access a cache which uses configuration before we build configuration, which causes an error.

Instead, store "are we in flight / has setup ever worked?" in a separate cache which doesn't use the cache namespace. This stops us from trying to read config before building config.

Test Plan:
Hit bad extension error with a fake extension, got a proper setup help page:

{F1812803}

Solved the error, reloaded, broke things again, got a "friendly" page:

{F1812805}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11613

Differential Revision: https://secure.phabricator.com/D16542
2016-09-12 08:04:32 -07:00
epriestley
17cd119b91 Add inline styles for {key ...} in HTML mail
Summary: Fixes T11607.

Test Plan:
  - Made a comment using `{key ...}`.
  - Used `bin/mail show-outbound --id X --dump-html > test.html` to review HTML:

{F1805304}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11607

Differential Revision: https://secure.phabricator.com/D16523
2016-09-08 17:10:58 -07:00
epriestley
5f43abd7ef Add a {key ..} Remarkup rule for discussing keystrokes
Summary: Ruleset for styles in D16506.

Test Plan: {F1803883}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16510
2016-09-07 09:09:40 -07:00
epriestley
b7e51877c3 Make storage adjustment a little nicer, especially the first time
Summary:
Fixes T11583.

  - When users run `bin/storage upgrade` for the first time on a new install, we currently give them a prompt which feels rough and which they can only reasonably ever answer "yes" to.
  - We generally use cautionary language ("found issues with schema") in this workflow. Adjustments are now routine, so use more neutral and progress-oriented language ("found adjustments to apply").

Test Plan:
  - Ran `bin/storage upgrade --namesapce kappa123`, got an adjustment using neutral language without prompting.
  - Dropped a key, ran `bin/storage upgrade`, got normal workflow (but with more neutral language).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11583

Differential Revision: https://secure.phabricator.com/D16509
2016-09-07 09:09:05 -07:00
epriestley
76af4d649b Fix ContainsConstraints for array-valued custom field constraints in "*.search" methods
Summary: Fixes T11593. We ask for a list of values when searching for custom "link" fields, but don't handle it correctly when actually construcitng a query.

Test Plan:
Added this custom field:

```
{
  "mycompany.target-version": {
    "name": "Target Version",
    "type": "link",
    "search": true
  }
}
```

Set a task to "beta". Let daemons index it. Queried for:

```
constraints: {
  "custom.mycompany.target-version": [
    "beta"
  ]
}
```

Got just one result back.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11593

Differential Revision: https://secure.phabricator.com/D16508
2016-09-07 09:08:10 -07:00
epriestley
0e8ceeb690 Continue on bad database configuration from select scripts
Summary: Ref T11589. Provide a way for scripts to say "just continue if database config fails", and use it in `bin/config` and `bin/storage`.

Test Plan:
  - Broke database config.
  - Ran `bin/config`, worked fine.
  - Ran `bin/storage`, got helpful "set up the database" message.
  - Ran `bin/repository`, got fatal.
  - Ran normal site with valid/invalid config, got proper feedback.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16502
2016-09-06 14:20:57 -07:00
epriestley
3099601463 Split setup check phases into "preflight" and "post-config"
Summary:
Ref T11589. This runs:

  - preflight checks (critical checks: PHP version stuff, extensions);
  - configuration;
  - normal checks.

The PHP checks are split into critical ("bad version") and noncritical ("sub-optimal config").

I tidied up the extension checks slightly, we realistically depend on `cURL` nowadays.

Test Plan:
  - Faked a preflight failure.
  - Hit preflight check.
  - Got expected error screen.
  - Loaded normal pages.
  - Hit a normal setup check.
  - Used DarkConsole "Startup" tab to verify that preflight checks take <1ms to run (we run them on every page without caching, at least for now, but they only do trivial checks like PHP versions).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

Differential Revision: https://secure.phabricator.com/D16500
2016-09-06 14:20:11 -07:00
epriestley
7eee5c5f6f Fix two "proably" typos
Summary: Caught one of these while reviewing docs, grepped for the other one.

Test Plan: `grep`, reading

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16498
2016-09-06 08:59:07 -07:00
epriestley
7e365fd3f1 Allow bin/storage renamespace to work with underscores
If the namespace is something like "test_example" we currently fail to
renamespace the dump.

(Cowboy committing this since this is currently blocking a data export.)

Test Plan:

  - Renamespaced a local dump, examined the output, saw 60 create / 60 use, reimported it.
  - Will export in production.

Auditors: chad
2016-09-06 07:03:45 -07:00
epriestley
d0013d0898 Distinguish between unreachable cluster database hosts and missing MySQL databases
Summary:
Fixes T11577. When we connect to a host and try to select a database which does not exist, we currently treat it as though the host wasn't reachable.

This isn't correct, and prevents storage from being initialized while already in cluster mode, since the "config" database won't exist yet the first time we connect.

Instead, distinguish between `AphrontSchemaQueryException` (thrown on connection if the requested database is not present) and other errors.

Test Plan:
  - Put Phabricator into cluster database mode (`cluster.databases = ...`).
  - Swapped `storage.default-namespace` to force initialization of a new install.
  - Ran `bin/storage upgrade`.
    - Before patch: Immediate fatal about unreachablility.
    - After patch: Database initialized.
  - Also ran initialization steps in tranditional single-host mode (`cluster.databases` empty, `mysql.host` configured).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11577

Differential Revision: https://secure.phabricator.com/D16489
2016-09-02 08:23:21 -07:00
Mike Riley
403073c989 Provide a workflow to restart Harbormaster builds
Summary:
Ref T10867 for original use case.  This workflow provides a plausible way for administrators to stop the daemons when performing upgrades or maintenance, then bring those daemons back up without resulting in the failure of builds that were running at the time.

On our organization's phab install, builds are running 24/7. The majority of these builds last for at least several minutes, and contain build steps which fail if interrupted and then resumed, as happens when turning daemons on and off.

Instead of allowing these build steps to resume execution as normal, this workflow will instruct active builds to restart their entire build process instead of just resuming whichever step they were on.

Test Plan:
contrived a build plan which would fail if resumed partway through:

 - lease a working copy
 - command `touch restart_{build.id}`
 - command `test -e restart_{build.id} && rm restart_{build.id} && sleep 60`

followed old procedure:

 - run a few of these builds manually
 - `./bin/phd stop`
 - `./bin/phd start`
 - saw the builds fail

followed new procedure:

 - run a few of these builds manually
 - `./bin/phd stop`
 - `./bin/harbormaster restart --active`
 - `./bin/phd start`
 - saw the builds pass

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T10867

Differential Revision: https://secure.phabricator.com/D16485
2016-09-02 13:32:02 +00:00