Summary:
Ref T9252. Broadly, Drydock currently races on releasing objects from the "active" state. To reproduce this:
- Scatter some sleep()s pretty much anywhere in the release code.
- Release several times from web UI or CLI in quick succession.
Resources or leases will execute some release code twice or otherwise do inconsistent things.
(I didn't chase down a detailed reproduction scenario for this since inspection of the code makes it clear that there are no meaningful locks or mechanisms preventing this.)
Instead, add a Harbormaster-style command queue to resources and leases. When something wants to do a release, it adds a command to the queue and schedules a worker. The workers acquire a lock, then try to consume commands from the queue.
This guarantees that only one process is responsible for writes to active resource/leases.
This is the last major step to giving resources and leases a single writer during all states:
- Resource, Unsaved: AllocatorWorker
- Resource, Pending: ResourceWorker (Possible rename to "Allocated?")
- Resource, Open: This diff, ResourceUpdateWorker. (Likely rename to "Active").
- Resource, Closed/Broken: Future destruction worker. (Likely rename to "Released" / "Broken"; maybe remove "Broken").
- Resource, Destroyed: No writes.
- Lease, Unsaved: Whatever wants the lease.
- Lease, Pending: AllocatorWorker
- Lease, Acquired: LeaseWorker
- Lease, Active: This diff, LeaseUpdateWorker.
- Lease, Released/Broken: Future destruction worker (Maybe remove "Broken"?)
- Lease, Expired: No writes. (Likely rename to "Destroyed").
In most phases, we can already guarantee that there is a single writer without doing any extra work. This is more complicated in the "Active" case because the release buttons on the web UI, the release tools on the CLI, the lease requestor itself, the garbage collector, and any other release process cleaning up related objects may try to effect a release. All of these could race one another (and, in many cases, race other processes from other phases because all of these get to act immediately) as this code is currently written. Using a queue here lets us make sure there's only a single writer in this phase.
One thing which is notable is that whatever acquires a lease **can not write to it**! It is never the writer once it queues the lease for activation. It can not write to any resources, either. And, likewise, Blueprints can not write to resources while acquiring or releasing leases.
We may need to provide a mechinism so that blueprints and/or resource/lease holders get to attach some storage to resources/leases for bookkeeping. For example, a blueprint might need to keep some kind of cache on a resource to help it manage state. But I think we can cross that bridge when we come to it, and nothing else would need to write to this storage so it's technically straightforward to introduce such a mechanism if we need one.
Test Plan:
- Viewed buttons in web UI, checked enabled/disabled states.
- Clicked the buttons.
- Saw commands show up in the command queue.
- Saw some daemon stuff get scheduled.
- Ran CLI tools, saw commands get consumed and resources/leases release.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14143
Summary:
See discussion in D10304. There's a lot of context there, but the general idea is:
- Blueprints should manage locks in a granular way during the actual allocation/acquisition phase.
- Optimistic "slot locks" might a pretty good primitive to make that easy to implement and reason about in most cases.
The way these locks work is that you just pick some name for the lock (like the PHID of a resource) and say that it needs to be acquired for the allocation/acquisition to work:
```
...
->needSlotLock("mylock(PHID-XYZQ-...)")
...
```
When you fire off the acquisition or allocation, it fails unless it could acquire the slot with that name. This is really simple (no explicit lock management) and a pretty good fit for most of the locking that blueprints and leases need to do.
If you need to do limit-based locks (e.g., maximum of 3 locks) you could acquire a lock like this:
```
mylock(whatever).slot(2)
```
Blueprints generally only contend with themselves, so it's normally OK for them to pick whatever strategy works best for them in naming locks.
This may not work as well if you have a huge number of slots (e.g., 100TB you want to give out in 1MB chunks), or other complex needs for locks (like you have to synchronize access to some external resource), but slot locks don't need to be the only mechanism that blueprints use. If they run into a problem that slot locks aren't a good fit for, they can use something else instead. For now, slot locks seem like a good fit for the problems we currently face and most of the problems I anticipate facing.
(The release workflows have other race issues which I'm not addressing here. They work fine if nothing races, but aren't race-safe.)
Test Plan:
To create a race where the same binding is allocated as a resource twice:
- Add `sleep(10)` near the beginning of `allocateResource()`, after the free bindings are loaded but before resources are allocated.
- (Comment out slot lock acquisition if you have this patch.)
- Run `bin/drydock lease ...` in two windows, within 10 seconds of one another.
This will reliably double-allocate the binding because both blueprints see a view of the world where the binding is free.
To verify the lock works, un-comment it (or apply this patch) and run the same test again. Now, the lock fails in one process and only one resource is allocated.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Differential Revision: https://secure.phabricator.com/D14118
Summary: Fixes T9351. This is straightforward since this application is now relatively modern and doesn't have any bizarre craziness.
Test Plan:
{F787981}
{F787982}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9351
Differential Revision: https://secure.phabricator.com/D14093
Summary:
One of the migrations in rPa335004a91 (`20150730.herald.5.sql`) incorrectly swapped "add" and "add blocking" Differential Herald rules.
Swap any rules last modified before this patch was applied back. This is the best we can do without possibly overwriting more recent, intentional data. I'll issue some guidance on this in the changelog.
Test Plan:
- Made a rule, ran patch, no change.
- Changed rule modified time to a few months ago, ran patch, saw swap from non-blocking to blocking.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14061
Summary: Adds an additional field for questions, an answer wiki, should should usually be community editable.
Test Plan: New question, edit question, no wiki, lots of wiki.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14003
Summary: Until we have a proper close as duplicate workflow for Ponder, remove the option with something more sensible.
Test Plan: Closed a question as invalid, saw it closed and in feed.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14007
Summary:
Ref T8783. Sort out some relationships and fields:
- Make Items 1:1 with Queues: each item is always in exactly one queue. Minor discussion on T8783. I think this is easier to understand and reason about (and implement!) and can't come up with any real cases where it isn't powerful enough.
- Remove "QueueItem", which allowed items to be in multiple queues at once.
- Remove "dateNuanced", which is equivalent to "dateCreated" in all cases.
Then add really basic routing:
- Add "Default Queue" for Sources. New items from the source route into that queue.
- (Some day there will be routing rules, but for now the rule is "always route into the default queue".)
- Show queue on items.
- Show more / more useful edit history and transactions in several UIs.
Test Plan:
{F749445}
{F749446}
{F749447}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8783
Differential Revision: https://secure.phabricator.com/D13988
Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?
Test Plan: New package, edit package, archive package, run search queries.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T8428
Differential Revision: https://secure.phabricator.com/D13925
Summary:
Ref T8659. In the general case, this eventually allows build processes to do things like:
- Upload build results (like a ".app" or ".exe" or other binary).
- Pass complex results between build steps (e.g., build step A does something hard and build step B uses it to do something else).
Today, we're a long way away from having the infrastructure for that. However, it is useful to let third party build processes (like Jenkins) upload URIs that link back to the external build results.
This adds `harbormaster.createartifact` so they can do that. The only useful thing to do with this method today is have your Jenkins build do this:
params = array(
"uri": "https://jenkins.mycompany.com/build/23923/details/",
"name": "View Build Results in Jenkins",
"ui.external": true,
);
harbormaster.createartifact(target, 'uri', params);
Then (after the next diff) we'll show a link in Differential and a prominent link in Harbormaster. I didn't actually do the UI stuff in this diff since it's already pretty big.
This change moves a lot of code around, too:
- Adds PHIDs to artifacts.
- It modularizes build artifact types (currently "file", "host" and "URI").
- It formalizes build artifact parameters and construction:
- This lets me generate usable documentation about how to create artifacts.
- This prevents users from doing dangerous or policy-violating things.
- It does some other general modernization.
Test Plan:
{F715633}
{F715634}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8659
Differential Revision: https://secure.phabricator.com/D13900
Summary: Ref T9173, adds basic hide support for answers. Answer authors and Moderators can hide answers, unhide them.
Test Plan: Hide answer, log into other account, see hidden message. Mark as visible, see answer again.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9173
Differential Revision: https://secure.phabricator.com/D13894
Summary: Ref T6920, This removes the PonderVotableInterface from PonderQuestion and assocaited code. Also... never used?
Test Plan: Visit Ponder, See List, New Question, Add Answer.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6920
Differential Revision: https://secure.phabricator.com/D13833
Summary: This allows installs to essentially set a "moderator" for Ponder, who can clean up answers. Fixes T9098
Test Plan: Edit an answer I don't own.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9098
Differential Revision: https://secure.phabricator.com/D13818
Summary: This fails to apply on my second sandbox with incorrect DOUBLE value. Reran SQL, works as expected.
Test Plan: Rerun new SQL on ponder_question table
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13830
Summary: Ref T9096. This is a first cut at adding additional statuses, happy to add or subtract as needed... maybe even configurable? Also, the dialog doesn't seem to fire, I'll keep debugging.
Test Plan: Close and Reopen many questions. Test applicationSearch params by seeing resolved questions, all questions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9096
Differential Revision: https://secure.phabricator.com/D13826
Summary: Ref T9076, adds basic plumbing for setting the state of a Paste.
Test Plan: Archive Paste, Activate Paste, New Paste
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9076
Differential Revision: https://secure.phabricator.com/D13801
Summary: Ref T8493, Ref T3578. Adds spaces support to ponder.
Test Plan: Ask a question in a new space, see new question.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578, T8493
Differential Revision: https://secure.phabricator.com/D13792
Summary: Ref T3578, adds ability to set a default edit and view policy for questions. Not sure what to set viewPolicy to ?
Test Plan: Test an old question, edit policy still on myself. Test a new question, see new default.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D13791
Summary: Ref T3846. Adds mailkey generation and migration.
Test Plan: Ran the migration, see keys in mysql.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3846
Differential Revision: https://secure.phabricator.com/D13785
Summary: Ref T8726. Convert these to be modular.
Test Plan:
- Created rules using these actions.
- Upgraded them.
- Verified they still work.
{F659266}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13705
Summary: Ref T8726. No surprises.
Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.
{F658842}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13701
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.
Test Plan:
{F658709}
- Wrote some rules.
- Migrated them forward.
- Used a bunch of these rules.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13699
Summary:
Fixes T8958. I renamed this class and table (in D13644), but the migration `20150606.mlist.1.php` needs to load records from the table before the table rename applies.
Put the table back and accept a tiny bit of cruft here to fix this problem. We can no-op the migration after a while.
I'll cherry-pick this to `stable`.
Test Plan: Applied migration. Created Herald rules. Reviewed existing Herald rules.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T8958
Differential Revision: https://secure.phabricator.com/D13736
Summary: Drops the sql column.
Test Plan: View logged in and logged out badges, edited some too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13715
Summary: Still doesn't mail yet, but the settings now show up.
Test Plan: View email settings, see Badges options.
Reviewers: eadler, epriestley
Reviewed By: eadler, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13712
Summary: DRAFT - throw together Phurl skeleton.
Test Plan: The idea is that `some/long/url` will become `install/Udet4d` and can be viewed and edited at `install/Udet4d/view` and `install/Udet4d/edit`, respectively?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, chad, epriestley, Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D13681
Summary: Fixes T8949. Adds the ability to render honors on those who have fought and received badges of distinction and honor.
Test Plan: Write 'asdf'. See 'asdf'.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8949
Differential Revision: https://secure.phabricator.com/D13704
Summary:
Ref T8726. I want to modularize actions like fields, but the base class should be "HeraldAction".
Eventually, "HeraldCondition" should probably be "HeraldFieldRecord", and then both Action and Condition should just be rolled into Rule, probably, but that can wait and doesn't block anything.
Test Plan: Ran migration, poked around UI, used `git grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13644
Summary: Allows countdowns to have a description.
Test Plan: Use description, edit description. Check timeline, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13691
Summary:
Basic plumbing for Badges application.
- You can make Badges.
- You can look at a list of them.
- They can be edited.
- They can be assigned to people.
- You can revoke them from people.
- You can subscribe to them.
Test Plan: Make Badges with various options. Give them to people. Take them away from people.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: tycho.tatitscheff, johnny-bit, epriestley, Korvin
Maniphest Tasks: T6526
Differential Revision: https://secure.phabricator.com/D13626
Summary: Implements T8734
Test Plan: Verify that countdowns can have spaces associated with them. Verify that non-default spaces are displayed in the header of the countdowns in the result of the query.
Reviewers: lpriestley, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13634
Summary: Implements T8733
Test Plan: Verify that spaces policies can be implemented in the creation and editing of polls.
Reviewers: lpriestley, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13537
Summary: Ref T8726. The gruntwork part of this is finally over.
Test Plan:
- Made a huge rule with every field.
- Applied migration.
- Verified the rule was still the same.
- Pushed a bunch of commits and verified transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13601
Summary: Ref T8726. Just (pre-commit content) one more left.
Test Plan:
- Created a big rule with every field.
- Migrated it.
- Verified the rule was still the same.
- Pushed a bunch of changes and reviewed the transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13600
Summary: Ref T8726. Make all the DifferentialRevision stuff modular.
Test Plan:
- Created a rule with all fields.
- Ran upgrade.
- Saw all fields preserved with new modular versions.
- Used test console to run rule with all fields, verified field values as broadly sensible.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13598
Summary: Ref T8726. This deals with all the Differential diff fields, same deal as previous changes.
Test Plan:
- Wrote a rule with every field.
- Migrated it.
- Saw the same rule working.
- Rigged the hell out of transcripts (diffs normally do not generate transcripts, because the only action is "block" and they don't exist yet when Herald runs).
- Verified that all fields looked sensible in the transcript.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13590
Summary: Fixes T7604. This is the big scary change which drops the "arcanist project" fields from the database permanently.
Test Plan:
`grep`ped for the following to ensure that I had found all remaining references:
- `/arcanistProject/i`
- `/arcanist_project/i`
- `/projectName/i`
- `/project_name/i`
- `/project_id/i`
WARNING: Wait at least one month before landing this.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12899
Summary: Ref T8726.
Test Plan:
Created a giant rule with every commit field:
{F594686}
Ran the upgrade, got the same rule with new fields:
{F594688}
Used "Test Console" to run transcripts, saw all the fields populate correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13567
Summary:
Ref T8746. See discussion there. We could try harder to fix this but I suspect the number of affected installs is exceedingly small since it's ~18 months old. Requiring reconfiguration of credentials seems reasonable-ish.
Also fix a merge issue with Conpherence search.
Test Plan: N/A
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8746
Differential Revision: https://secure.phabricator.com/D13565
