Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

internal NTP: resolve boundary NTP sources from DNS in addition to being told explicitly #6050

Merged
merged 8 commits into from
Aug 8, 2024
Merged

internal NTP: resolve boundary NTP sources from DNS in addition to being told explicitly #6050

merged 8 commits into from
Aug 8, 2024

Conversation

jgallagher
Copy link
Contributor

@jgallagher jgallagher commented Jul 11, 2024 •
edited
Loading

This PR adds a special internal DNS name boundary.ntp.control-plane.oxide.internal which resolves to a set of AAAA records, one for each boundary NTP zone. We pass this name to chrony via a pool directive in its config file, allowing it to find the boundary NTP servers via internal DNS.

Some context for this PR:

  • It's blocked behind zone-setup cleanup/refactoring #6015 which is itself blocked behind [sled-agent] Self assembling switch zone #5593 which is itself blocked waiting for R9 to go out. I don't expect many conflicts or changes when those dominos start to fall, but it can't land for a bit.
  • This PR does not remove the explicit boundary NTP server names from either the sled-agent -> zone-setup or the zone-setup -> chrony config paths. Assuming this PR ships as part of R10, we can come back and remove those after R10 is out the door. We can't do both in one release, because we need to establish the new DNS name (via setting a new blueprint) in R10, at which point we can remove the explicit server names in R11, because NTP has to sync before we get the opportunity to modify DNS.

Fixes #4791.

@davepacheco davepacheco mentioned this pull request Jul 12, 2024
Open
davepacheco
davepacheco reviewed Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question not covered in the PR: have we ensured somehow that the internal DNS zones will have DNS configured in /etc/resolv.conf? Last I checked, that was not reliably configured. It was only available in some zones. But that was a while ago.

internal-dns/src/names.rs Outdated Show resolved Hide resolved
smf/chrony-setup/manifest.xml Show resolved Hide resolved
zone-setup/src/bin/zone-setup.rs Outdated
@@ -281,10 +294,18 @@ maxslewrate 2708.333
.expect("write to String is infallible");
}
} else {
// TODO-john Replace with link to issue: remove specific boundary NTP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you want to update this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks! Fixed in fa58d40

zone-setup/src/bin/zone-setup.rs
for s in servers {
writeln!(&mut new_config, "server {s} iburst minpoll 0 maxpoll 4")
.expect("write to String is infallible");
}
writeln!(
&mut new_config,
"pool {boundary_pool} iburst maxdelay 0.1 maxsources 16",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I haven't reviewed the change in the chrony config

@jgallagher jgallagher force-pushed the john/zone-setup-cleanup branch from 26413d8 to 3a57dfc Compare July 29, 2024 15:43
@jgallagher jgallagher force-pushed the john/resolve-boundary-ntp-from-internal-dns branch 2 times, most recently from bd2cd29 to f973a52 Compare July 31, 2024 14:21
Base automatically changed from john/zone-setup-cleanup to main July 31, 2024 16:58
@jgallagher jgallagher force-pushed the john/resolve-boundary-ntp-from-internal-dns branch 3 times, most recently from 23c6b0e to 962da59 Compare August 5, 2024 16:02
@jgallagher jgallagher force-pushed the john/resolve-boundary-ntp-from-internal-dns branch from 962da59 to 1098aca Compare August 7, 2024 13:44
This was referenced Aug 7, 2024
Merged
Closed
@jgallagher
Copy link
Contributor Author

One question not covered in the PR: have we ensured somehow that the internal DNS zones will have DNS configured in /etc/resolv.conf? Last I checked, that was not reliably configured. It was only available in some zones. But that was a while ago.

Yes, sled-agent does this here for internal DNS, confirmed on a4x2 and london.

davepacheco
davepacheco approved these changes Aug 8, 2024
View reviewed changes
internal-dns/src/config.rs
@@ -407,6 +407,27 @@ impl DnsConfigBuilder {
(name, vec![DnsRecord::Aaaa(sled_ip)])
});

// Assemble the special boundary NTP name to support chrony on internal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence - the expanded test_blueprint_internal_dns_basic does check for this name. If you'd like a unit test specific to this crate, I can add one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth it, mainly because I think it should just be 1-2 lines of code in test_builder_output() below (plus regenerate the expectorate file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure enough! Added in 24b7562

@jgallagher jgallagher merged commit dda118a into main Aug 8, 2024
22 checks passed
@jgallagher jgallagher deleted the john/resolve-boundary-ntp-from-internal-dns branch August 8, 2024 21:24
jgallagher added a commit that referenced this pull request Aug 8, 2024
@jgallagher
Reconfigurator: Add support for boundary NTP zones (#6259)
5fb9029 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davepacheco davepacheco davepacheco approved these changes

@citrus-it citrus-it Awaiting requested review from citrus-it

Assignees

@jgallagher jgallagher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NTP zone config could be more dynamic
2 participants
@jgallagher @davepacheco