The name of networks should be unique.
Also adds support for exact-name queries for AlamanacNetworks.
Details
Details
- Reviewers
epriestley - Commits
- rP0a83f253ed03: Add unique constraint for Almanac network names
Applied migration with existing duplicates, saw networks renamed, attempted to add duplicates, got a nice error message.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Migrations like this are tricky. For example, if an install has two networks with the same name already, ADD UNIQUE KEY ... will fail and leave the user stuck with a broken install that can't upgrade and no clear way to continue.
I think the state of the art here is D18623, a PHP migration with:
- LOCK TABLES ... WRITE
- Modify the data so that we guarantee the constraint is not violated (e.g., by adding numbers on the end of existing, duplicate network names).
- ADD UNIQUE KEY ...
- UNLOCK TABLES
I think the Network change is reasonable, but should be accompanied by these changes:
- The column type should become sort128, not text128, so that abc and ABC are not different networks.
- The NetworkNameTransaction should raise a friendly error about this, similar to the errors that Devices and Services generate.
- We should probably (?) apply AlmanacNames::validateName(...) logic to network names, which may require some error messages to be updated to say "...service, device, or network name...". This will disallow network names like "Public Internet", which the documentation may currently recommend.
(I think the future of Almanac is very automation-heavy and that these constraints make sense looking forward.)
I'm somewhat less certain the Interface change is correct/desirable in the long run. Currently, we actually destroy Interface objects, but this is unusual and should probably stop at some point, and we should disable/archive them instead. When that happens, maybe it becomes valid to have an old disabled :80 interface and a newer enabled :80 interface?
But it's easy to remove this key later and will only get harder to add it, and I can't come up with any reason not to have it today. However, we need to do the migration dance to make sure we don't stick installs in a dead state:
- LOCK TABLES ... WRITE
- Identify interfaces which violate this constraint. Pick a survivor (lowest or highest ID), update all bindings on other copies to point at the survivor, then destroy the copies.
- ADD UNIQUE KEY ...
- UNLOCK TABLES
AlmanacInterfaceEditor may also need a didCatchDuplicateKeyException() implementation to provide a nicer error message.
(Maybe split this into two separate changes, too.)
Comment Actions
The column type should become sort128, not text128, so that abc and ABC are not different networks.
Does this require a migration? It looks like it's just a collation type change according to bin/storage upgrade.
We should probably (?) apply AlmanacNames::validateName(...) logic to network names, which may require some error messages to be updated to say "...service, device, or network name...". This will disallow network names like "Public Internet", which the documentation may currently recommend.
Do I need to create a migration to fix this as well? It looks pretty non-trivial to mangle arbitrary strings to conform to the AlmanacNames::validateName(...) rules.
| resources/sql/autopatches/20180418.almanac.network.unique.php | ||
|---|---|---|
| 25 | I just realized this name could also be a duplicate. Fix coming... | |
Comment Actions
The new migration logic might be too clever by half, but I tried pretty hard to break it and wasn't able to.
Comment Actions
Identify interfaces which violate this constraint.
Something like this?
$counts = queryfx_all( $conn_w, 'SELECT networkPHID, devicePHID, address, port, count(*) N FROM %T GROUP BY networkPHID, devicePHID, address, port', $interface->getTableName()); foreach ($counts as $row) { if ($row['N'] > 1) { //blah blah } }
Comment Actions
Does this [text128 -> sort128] require a migration?
In general, no. bin/storage adjust will do this kind of change automatically.
In this particular case, if you don't add an explicit migration, installs can technically do this:
- Run the other migrations with bin/storage upgrade --no-adjust for some reason.
- Start Phabricator.
- Create networks "AAA" and "aaa".
- bin/storage adjust now fails when run.
This is normally very hard to do (i.e., you basically have to willfully break Phabricator) and fairly easy to get out of (Phabricator runs fine without adjust, so you can just go rename one of the networks, although it may not be clear that renaming is what you should do) so I probably wouldn't bother with an explicit migration, but you could add one if you want.
(One note is that we use --no-adjust in the Phacility cluster to limit downtime (adjust runs in a separate, optional phase) so rough edges here are a little more likely to cut us than regular installs.)
Do I need to create a migration to fix [almanac names] as well?
I think these are fine to leave as-is since nothing else depends on them or references them. This change is mostly just for consistency and sanity in a more API-oriented world.
Something like this [to identify duplicate interfaces]?
That should work, but it's a little tricky to get the IDs out if you GROUP BY (at least, I don't know a good way to do it). I'd probably just do something dumb/simple like this:
$big_map = array();
foreach (new LiskMigrationIterator(new AlmanacInterface()) as $interface) {
$big_map["$network/$device/$address/$port"][] = $interface->getID();
}Then throw away all the entries with only 1 element, and you're left with the duplicates. But the GROUP BY approach is perfectly fine too if you find that simpler to write or work with.
Actual changes here look good to me. I don't see any way to break the rename migration, except the one edge case inline.
| resources/sql/autopatches/20180418.almanac.network.unique.php | ||
|---|---|---|
| 25 | An install that is very, very dedicated to making our lives difficult could name two networks the same 128 character name before this migration. We'd then try to rename the second one to "AAAA...AAAA-1", and the insert would fail because the name was too long (128 "A" and then "-1" for 130 characters total). You could avoid this by using PhutilUTF8StringTruncator to choose the name "<first 100 characters of the old name>-1" or similar. But it's also hard to imagine that any install would do this and I suspect we've done similar migrations in the past successfully without accounting for this possibility, so I'd probably not bother until someone complains. | |