-
Notifications
You must be signed in to change notification settings - Fork 295
CP-54476 Merge ntp disable/enable to ntp mode #6765
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
CP-54476 Merge ntp disable/enable to ntp mode #6765
Conversation
|
Test: # set NTP mode Custom, use custom NTP servers
session.xenapi.host.set_ntp_custom_servers(host_ref, ['time.google.com', 'time.windows.com', 'time.apple.com'])
session.xenapi.host.set_ntp_mode(host_ref, 'Custom')
time.sleep(5)
show('NTP mode Custom, use custom NTP servers')
# set NTP mode DHCP
session.xenapi.host.set_ntp_mode(host_ref, 'DHCP')
time.sleep(5)
show('set NTP mode DHCP')
# set NTP mode Factory
session.xenapi.host.set_ntp_mode(host_ref, 'Factory')
time.sleep(5)
show('set NTP mode Factory')
# disable NTP
session.xenapi.host.set_ntp_mode(host_ref, 'Disabled')
session.xenapi.host.set_servertime(host_ref, '20251125T14:11:55+08:00')
show('set NTP mode Disbaled and set server time')
# enable NTP and set to DHCP mode
session.xenapi.host.set_ntp_mode(host_ref, 'DHCP')
time.sleep(5)
show('set NTP mode DHCP')output: Exceptions example: try:
session.xenapi.host.set_ntp_mode(host_ref, 'Custom')
session.xenapi.host.set_ntp_custom_servers(host_ref, [])
except Exception as e:
print(e)
try:
session.xenapi.host.set_ntp_mode(host_ref, 'DHCP')
session.xenapi.host.set_ntp_custom_servers(host_ref, [])
session.xenapi.host.set_ntp_mode(host_ref, 'Custom')
except Exception as e:
print(e)
try:
session.xenapi.host.set_servertime(host_ref, '20251125T14:11:55+08:00')
except Exception as e:
print(e)output |
| if current_mode <> value then ( | ||
| let open Xapi_host_ntp in | ||
| let ensure_custom_servers_exist servers = | ||
| let ensure_servers_exist servers msg = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at least 3 NTP servers are required.
And it could be assert_sufficient_ntp_servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I find rfc8633 in chapter 3.2. Using Enough Time Sources recommends the four or more time sources. But here, I think it's just the best practice, not a hard requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 or 2 will fail definitely. This is for how NTP works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "1 or 2 will fail definitely" is too strong. They are risky and uncertain but not guaranteed failure.
One server: If it goes down or drifts significantly, there’s no backup.
Two servers: If they disagree, NTP can’t determine which one is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the description of what happens with one or two servers, it looks to me NTP is unable to work reliably :)
Because we want xapi to work reliably at all times, I think it's warranted to enforce at least 3 servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still insist we shouldn't strict the API settings. NTP service like chornyd will not limit the number of time sources. It's strange to encounter this error in our APIs if I am the user. If in user's internal network environment, there is only one NTP server deployed, it is hard to work.
@robhoes Will I get a vote from you on this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not restrict this in xapi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'd prefer if the standard recommending at least 3 servers was added in the datamodel field. This is something I'll also mention to XO people to avoid issues
406a201 to
4acb199
Compare
ocaml/idl/datamodel_host.ml
Outdated
| "Whether the host has booted in secure boot mode" | ||
| ; field ~qualifier:DynamicRO ~lifecycle:[] ~ty:host_ntp_mode | ||
| ~default_value:(Some (VEnum "ntp_mode_dhcp")) "ntp_mode" | ||
| ~default_value:(Some (VEnum "DHCP")) "ntp_mode" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is set to DHCP here, which is relevant for the update case. This implies that before this feature, hosts relied on DHCP to set up NTP. Is that right, as I expected Factory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we have to rely on the Dbsync_slave.update_env to get the info when xapi restart for the update application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Default value of ntp_mode changes to Factory.
| if current_mode <> value then ( | ||
| let open Xapi_host_ntp in | ||
| let ensure_custom_servers_exist servers = | ||
| let ensure_servers_exist servers msg = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not restrict this in xapi.
| ensure_servers_exist factory_servers | ||
| "Can't set ntp_mode to Factory when factory ntp servers is empty" ; | ||
| set_servers_in_conf factory_servers | ||
| | `Disabled -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot one thing. Should the system clock be synced to RTC hardware clock when NTP is about to be disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Enable kernel synchronization of the real-time clock (RTC).
rtcsync
In our dom0 host, chrony is configured to sync RTC after NTP synchronized. So there is no need to do it again in XAPI. I also have verified it.
Signed-off-by: Changlei Li <changlei.li@cloud.com>
Command timedatectl is used to set server time. However, if disable chronyd before set_servertime by systemctl, timedatectl may not sync the status in time, then the set-time will fail as `Failed to set time: Automatic time synchronization is enabled`. This is because systemd checks synchronization status periodically and update its own NTP flag (which is timedatectl replied on) To let timedatectl update NTP flag instantly, use timedatectl set-ntp true/false to enable/disable the NTP. Signed-off-by: Changlei Li <changlei.li@cloud.com>
The `/run/chrony-dhcp` dir can be created when chrony.sh is executable (i.e. DHCP mode) when host boot up. If host boots up with other NTP mode, the dir may not be created. Check the dir and create it if not exist. Signed-off-by: Changlei Li <changlei.li@cloud.com>
Signed-off-by: Changlei Li <changlei.li@cloud.com>
4acb199 to
1b698f0
Compare
02ee3af
into
xapi-project:feature/config-ntp-timezone-maxcstate
As the doc PR #6745 shows, the enable/disable ntp APIs and host.ntp_enabled field can be merged into host.ntp_mode.
Now, the ntp modes are DHCP, Custom, Factory and Disabled.
This PR also updates the datamodel lifecycle and bumps up the schema.