Reconfigurator: Add support for boundary NTP zones #6259
Conversation
|
Test setup on london:
After the rack comes back, we see the new chrony config line from #6050 in an internal NTP zone, but the new We can confirm the DNS contents via omdb: To get the new We can see the new DNS config was pushed out: And back in the internal NTP zone, the name resolves: (We'll need to do the above as part of the R10 upgrade, so we can assume moving forward that this name exists.) |
|
Continuing the test on london, I ran Importing that to Nexus and diffing shows the one zone expunged as well: I then made this new blueprint the target: On the relevant sled, we see that sled-agent removed the zone: Regenerating a new blueprint adds an internal NTP zone to the sled which now has no NTP zone at all, and promotes one of the preexisting internal NTP zones to a boundary NTP zone: After making this new blueprint the target, we can confirm that the new boundary NTP zone has connectivity: As does the new internal NTP zone, including to the new boundary NTP zone ( Checking one of the existing, untouched internal NTP zones, we see that chrony has picked up the new boundary NTP zone, but also still has a record of the now-expunged one. I'm not sure if this is bad or indicative of something missing in the chrony config, or something that will update if I wait long enough? (The high Checking the config and DNS, it's as we expect: we see the two original RSS boundary NTP zones, but the expunged one no longer resolves, and the new server is present in the I then repeated all of the above to expunge the other boundary NTP zone that RSS had created. By luck/chance, one of the original internal NTP zones remained, and we can see that it now knows about all four, but is only able to resolve the new ones (and only has recent We can also confirm that the external IP and vnic accounting in CRDB looks correct; we only have records for the two new boundary NTP zones: As a final test, I rebooted all the sleds of london concurrently to ensure we could come back up with our new boundary NTP zones. This appeared to work fine; the control plane came back up, and the four boundary NTP zones all have the expected sources (including the original internal NTP that still has config references to now-nonexistent zones, which only sees the two new sources now): |
|
Those are great tests -- especially the cold boot.
Do you know why we have these dups? Is it that we have three DNS servers in /etc/resolv.conf and host(1) asks all of them and prints all the results? I'm basically just trying to figure out if we have dups in the data coming from a single DNS server. |
I don't think it's duplicate data, but it might be a different problem? I'm seeing triplicate answers even on dogfood with one of the specific-zone names: Bumping up the verbosity, it looks like |
There was a problem hiding this comment.
If I'm reading this correctly, we don't ever convert a boundary NTP zone to an internal NTP one. (e.g., if we found ourselves with too many boundary NTP zones). I was surprised at first but I can't think of a reason we'd need to do that so I'm glad to avoid the complexity if we can.
| @@ -435,6 +436,8 @@ enum BlueprintEditCommands { | |||
| }, | |||
| /// add a CockroachDB instance to a particular sled | |||
| AddCockroach { sled_id: SledUuid }, | |||
| /// expunge a particular zone from a particular sled | |||
| ExpungeZone { sled_id: SledUuid, zone_id: OmicronZoneUuid }, | |||
| // We want to preserve the CockroachDB cluster settings from the parent | ||
| // blueprint. | ||
| new_blueprint | ||
| .cockroachdb_fingerprint | ||
| .clone_from(&blueprint.cockroachdb_fingerprint); | ||
| new_blueprint.cockroachdb_setting_preserve_downgrade = | ||
| blueprint.cockroachdb_setting_preserve_downgrade; |
There was a problem hiding this comment.
Should this be handled in BlueprintBuilder::new_based_on() (called at L739 above)?
There was a problem hiding this comment.
cockroachdb_setting_preserve_downgrade is handled in new_based_on(), yeah, but cockroachdb_fingerprint isn't. I removed cockroachdb_setting_preserve_downgrade and expanded the comment to add detail on why we're manually setting the fingerprint in 6cf3ab5
| @@ -341,8 +345,9 @@ impl<'a> BlueprintBuilder<'a> { | |||
| pub fn current_sled_zones( | |||
| &self, | |||
| sled_id: SledUuid, | |||
| filter: BlueprintZoneFilter, | |||
| // | ||
| // TODO-cleanup Is there ever a case where we might want to do some kind | ||
| // of graceful shutdown of an internal NTP zone? Seems unlikely... |
There was a problem hiding this comment.
I can't see why. Is there something you have in mind?
There was a problem hiding this comment.
Not really. Maybe if we were doing some kind of graceful cleanup of all the zones on a sled? But even then I don't think there's anything to actually do. I'll remove this comment.
| let mut unmatched = BTreeSet::new(); | ||
| unmatched.insert(zone_id); | ||
| BuilderZonesConfigError::ExpungeUnmatchedZones { unmatched } |
There was a problem hiding this comment.
rustfmt seems asleep at the switch here.
There was a problem hiding this comment.
I don't think so? If I change the formatting on this line, rustfmt puts it back to the way it is.
There was a problem hiding this comment.
... but it's definitely wrong, right? I would expect L84-86 to be indented because they're inside a block that ends at line 87. I'd also expect L87 to be indented because it closes the block and parens opened at L83.
There was a problem hiding this comment.
It's definitely weird and probably wrong, and possibly related to our rustfmt.toml settings:
max_width = 80
use_small_heuristics = "max"If I comment out use_small_heuristics, it doesn't change. If I comment out both lines, I get what you want:
let zone = self
.zones
.iter_mut()
.find(|zone| zone.zone.id == zone_id)
.ok_or_else(|| {
let mut unmatched = BTreeSet::new();
unmatched.insert(zone_id);
BuilderZonesConfigError::ExpungeUnmatchedZones { unmatched }
})?;If I comment out just max_width and keep use_small_heuristics, I get this (which also seems reasonable and more correct than what we're getting now):
let zone = self.zones.iter_mut().find(|zone| zone.zone.id == zone_id).ok_or_else(|| {
let mut unmatched = BTreeSet::new();
unmatched.insert(zone_id);
BuilderZonesConfigError::ExpungeUnmatchedZones { unmatched }
})?;I think I will just leave this alone - I'm not sure how to isolate it, or whether it's a rustfmt bug or just some interaction we don't understand.
Hmm. This looks similar to #4051 and #4258. It seems like maybe the DNS server is just ignoring the requested record type and reporting everything for the given name? If that's the behavior, that does seem like a bug, but I don't think it's a problem for this PR since this is the only kind of record that we have for this name. I guess the other possible problem would be if chrony also queries separately for different record types and gets confused by the dups. But it seems unlikely that dups would make it do the wrong thing. |
Today we don't handle any cases of "I have too many service of kind X"; we only ever add if we're below the policy count. That presumably needs to change at some point, and then we might need to convert boundary NTP back to internal. I think that would be pretty similar to this, though? Basically - expunge the boundary NTP zone, and add a new internal NTP? |
This also fixes the planner considering an expunged NTP zone to satisfy the "every sled must have an NTP zone" requirement.
jgallagher
force-pushed
the
john/reconfigurator-planning-boundary-ntp
branch
from
6cf3ab5 to
eefaaf3
Compare
This is almost entirely planner work; no executor work was required, because it already supports "zones with external networking", and there's nothing special to boundary NTP beyond that from an execution point of view.
I put this through a fairly thorough test on london; I'll put my notes from that in comments on the PR momentarily.
This builds on #6050 and should be merged after it.