Repository navigation
Conversation
|
This PR should not be committed before xapi-project/xen-api#7332 . |
|
Cc @changlei-li |
|
Tests:
|
| self.data['ntp']['sourcedir'].append(match.group(1)) | ||
| continue | ||
|
|
||
| self.data['ntp']['othercontents'].append(line) |
There was a problem hiding this comment.
Can we just query the NTP mode from XAPI? Then we wouldn't need to maintain the logic in two places going forward.
Previously, NTP could only be configured through xsconsole. After XAPI added support for configuring NTP, the control path in xsconsole was changed to use the XAPI APIs to avoid breaking XAPI's state.
Now that the detection rule may change on the host (DHCP mode relies on the sourcedir line rather than the execute permission of chrony.sh here), I think it would be better to reuse the XAPI APIs in the query path as well. I think this should be doable, though I haven't gone through the related code yet.
There was a problem hiding this comment.
I refactored the whole NTP code to query XAPI instead of looking at system (chrony.conf, service status, etc...)
It makes xsconsole changes independent of xapi-project/xen-api#7332 : it could be committed before it.
a783a30 to
b272c17
Compare
It makes xsconsole independant of the XAPI implementation for NTP DHCP (which is changed by xapi-project/xen-api#7332). Tracking-reference: XCPNG-1094 Signed-off-by: Sebastien Rodot <sebastien.rodot@vates.tech>
b272c17 to
ea131c1
Compare
|
Last push:
|
| self.RequireSession() | ||
| if self.session is not None: | ||
| try: | ||
| self.data['ntp']['method'] = self.session.xenapi.host.get_ntp_mode(self.data['host']['opaqueref']) |
There was a problem hiding this comment.
xsconsole uses DHCP, Manual, Default and None (XSFeatureNTP.py line19-23). So a mapping is needed.
There was a problem hiding this comment.
In the write path (Commit function in XSFeatureNTP.py:383-390), a mapping is need because we are using the dialog state-machine choices and write it to XAPI mode.
Here, in the read path, the raw XAPI value is used. It has only 2 consumers:
StatusUpdateHandler(XSFeatureNTP.py:417) which compares"Disabled"and["DHCP", "Factory"]XSMenuLayout.py:79which displays the XAPI value
The NTP dialogue's INITIAL pane never reads ntp.method to preselect/highlight the current state, so the state machine values (DHCP/DEFAULT/MANUAL/NONE) are purely internal flow identifiers and never compared against data['ntp']['method'].
But I agree that the output in xsconsole isn't consistent as XSMenuLayout.py:79 displays the XAPI value instead of the historical DHCP, Manual, Default and None. I will change that.
| if self.session is not None: | ||
| try: | ||
| self.data['ntp']['method'] = self.session.xenapi.host.get_ntp_mode(self.data['host']['opaqueref']) | ||
| self.data['ntp']['servers'] = self.session.xenapi.host.get_ntp_custom_servers(self.data['host']['opaqueref']) |
There was a problem hiding this comment.
self.data['ntp']['servers'] meaning is changed. Before it is whatever in chrony.conf. Now it is host.ntp-custom-servers. I am not clear whether it is consistent in the new logic.
There was a problem hiding this comment.
I agree that self.data['ntp']['servers'] meaning is changed. It is now exactly host.ntp_custom_servers, i.e.
only meaningful when ntp_mode is Custom.
I don't think parsing chrony.conf is the right answer: the factory servers live in XAPI's product config (factory_ntp_servers), no XAPI field carries them, and deriving them from the conf file would duplicate in xsconsole the detection logic that XAPI owns.
After reviewing the logic, I will do some changes.
So the read path stays purely XAPI-based, split by purpose:
servers=host.ntp_custom_servers: the draft list for Custom mode. Consumed only by the edit flow (MANUAL pane add/remove), where it's the value actually written back.- Status display shows the effective sources via
host.get_ntp_servers_statusin all modes (Custom included), instead of shelling out tochronycdirectly. Note that in DHCP mode that's the DHCP-lease servers, in Factory the factory servers.
One consequence I'll fix in xsconsole: AdjustNTPForStaticNetwork infers the mode from whether the servers list is empty, which no longer holds since the field isn't cleared when leaving Custom. I'll make it branch on ntp_mode instead.
| self.SetNTPMode('Disabled') | ||
|
|
||
| self.DisableService("chrony-wait") | ||
| self.DisableService("chronyd") |
There was a problem hiding this comment.
The two disables are legacy behaviors. I think it need more consideration for the remove. What is chrony-wait and what is the impact.
Following xapi-project/xen-api#7332, this commit changes the way the DHCP mode of NTP is implemented.
Instead of relying on executable permission of
chrony.sh(dhclient hook), use the presence ofsourcedirline inchrony.conf.It makes the implementation to depend only on chrony instead of both chrony and dhclient.
Tracking-reference: XCPNG-1094