Skip to content

Use NTP admin API instead of sled agent, remove sled agent time_sync API #8597

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.

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

Open
wants to merge 1 commit into
base: ntp-admin
Choose a base branch
from

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jul 15, 2025

Builds on #8555

This PR:

  • Avoids the sled agent "zlogin to NTP zone" calls to chronyc, instead relying on the NTP admin server
  • Removes the "timesync_get" API exposed from the sled agent, and moves all callers (RSS only) to call the NTP admin server instead

Fixes #8590

@smklein smklein force-pushed the ntp-admin-use branch 2 times, most recently from c4faf10 to 8f65850 Compare July 15, 2025 17:51
@smklein smklein marked this pull request as ready for review July 15, 2025 19:53
@smklein smklein requested review from jgallagher and karencfv July 15, 2025 21:54
@smklein
Copy link
Collaborator Author

smklein commented Jul 15, 2025

FYI, I ran this on a4x2 - it didn't work, until I fixed an SMF typo, now it works. This is tested implicitly by the "helios / deploy" job, which relies on RSS, and also relies on the timesync API in the NTP admin server.

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@@ -1464,29 +1464,6 @@
}
}
},
"/timesync": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's any documentation that relies on checking the response from this endpoint in the logs to debug an NTP sync issue or something similar. I have a vague memory of something like that. If anyone remembers, that documentation should be updated. Perhaps an announcement on Matrix might be good as well? I've certainly used it before if an omicron deployment was taking too long to start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants