[semarie]

Change the way ntp-mode=DHCP integration is done.

Previously, xapi was parsing dhclient leases (internal format, DHCPv4 format only) and manually create sources file inside /run/chrony-dhcp directory.

Prefer relying on proper NTP and DHCP integration: only change chrony.conf file and add sourcedir /run/chrony-dhcp directive only when ntp-mode=DHCP, and let's the DHCP client to manage populating /run/chrony-dhcp directory.

It makes ntp-mode=DHCP to be independent of ISC dhclient.

[semarie]

Tests done:

• verify idempotence of ntp-mode changes (like Factory -> DHCP -> Factory give same configuration)
• verify NTP service is using the provided NTP servers from DHCP (chrony 4.6.1, ISC dhclient-4.4.3)
• verify that others modes are still working as expected

[semarie]

Last push:

• fix make check : ( // ) function is now unused and should be removed

[psafont]

I do like removing these kinds of dependencies. One thing that is not clear, is how is the synchronization between the writing of the configuration and reading of it by the NTP client happening, they just read it periodically, or is there some kind of trigger elsewhere that makes them read the configuration?

[semarie]

The /etc/dhcp/dhclient.d/chrony.sh script (from chrony package) is doing all the integration.

#!/usr/bin/bash

CHRONY_SOURCEDIR=/run/chrony-dhcp
SERVERFILE=$CHRONY_SOURCEDIR/$interface.sources

chrony_config() {
        # Disable modifications if called from a NM dispatcher script
        [ -n "$NM_DISPATCHER_ACTION" ] && return 0

        rm -f "$SERVERFILE"
        if [ "$PEERNTP" != "no" ]; then
                mkdir -p $CHRONY_SOURCEDIR
                for server in $new_ntp_servers; do
                        echo "server $server ${NTPSERVERARGS:-iburst}" >> "$SERVERFILE"
                done
                /usr/bin/chronyc reload sources > /dev/null 2>&1 || :
        fi
}

chrony_restore() {
        [ -n "$NM_DISPATCHER_ACTION" ] && return 0

        if [ -f "$SERVERFILE" ]; then
                rm -f "$SERVERFILE"
                /usr/bin/chronyc reload sources > /dev/null 2>&1 || :
        fi
}

The functions are called by /usr/sbin/dhclient-script (from dhcp-dhclient package).

On receiving IP from DHCP, chrony_config is called:

• /run/chrony-dhcp is created if not exist
• a $interface.sources file is created and populated with NTP servers received
• it is calling chronyc reload sources to make chronyd aware of the changes

On lease release, chrony_restore is called, and it is removing the file and make chronyd aware of the change

[psafont]

Thanks for the explanation!

[changlei-li]

Thanks @psafont involves me. I remember the implementation has some historical reasons. I will look into it.

[changlei-li]

Good change, but it breaks backward compatibility during update and upgrade.

[semarie]

I have added migration code path. At xapi start, it will look at the current NTP system layout and update it. I used an integer for keeping track of "NTP system layout version" instead of just a boolean in order to have a possibly different layout later.

On the first host startup after the upgrade, it will ensure that the DHCP hook file is executable and rewrite the NTP configuration file.

Regarding populating the NTP sources at switch time, I think it is not necessary:

• if IP-configuration-mode=DHCP before setting ntp-mode=DHCP, the hook has been already ran previously so /run/chrony-dhcp is already filled with a source file. Once ntp-mode=DHCP is setted, chrony is reloaded with it.
• if IP-configuration-mode=DHCP is set after ntp-mode=DHCP, the hook will be ran immediately (new lease) and it is the hook that will ask chrony to reload the sources.

But there is one edge-case that I need to review more deeply: the upgrade time. I think there is a possible race window where the hook isn't executable at DHCP time (so no sources will be recorded), and xapi updating it later.

[semarie]

Regarding the edge-case I mentioned, I confirm it is existing (and I am able to trigger it).

• host is running with previous system layout, IP-configuration-mode=DHCP and ntp-mode=Disabled (or anything not DHCP)
  • a DHCP lease has been acquired
  • the chrony.sh hook file is without executable bit (due to ntp-mode != DHCP)
  • no sources files present in /run/chrony-dhcp (due to -x on hook file)
• xapi is updated and toolstack restarted
  • due to toolstack restart, the chrony.sh` hook file is updated
  • but sources files will be only added at the next lease renewal
• if the user sets ntp-mode=DHCP during this window of time (between the xapi update and the next lease renewal), the NTP server will have no sources until the next lease renewal.

I dunno if the drawback of this one-time and not-immediately effective NTP configuration change is acceptable or not.

[changlei-li]

parse_ntp_conf gives the whole server_line. But write_ntp_conf needs bare server name. It should be wrong here. You may have a self test to verify this.

[semarie]

Thanks for pointing that. My tests doesn't expose it because I didn't done tests with manually edited chrony.conf file.

When you sets ntp-mode=DHCP (which it calling add_dhcp_ntp_servers function), the state were:

• ntp-mode=Factory : it calls clear_servers_in_conf first
• ntp-mode=Custom : it calls clear_servers_in_conf first
• ntp-mode=Disable : file was not touched before only the service is disabled
• ntp-mode=DHCP : set_ntp_mode skips the change

So you usually don't have server lines in the chrony.conf file at add_dhcp_ntp_servers time.

I will look to fix it. There is no reason to produce an invalid chrony.conf file.

[changlei-li]

Hi @semarie do you have any plans to backport it to lcm-26.1? There are a few more corner cases to consider there I think.

[semarie]

there is no backport intent for 26.1-lcm.

[semarie]

Hi @changlei-li I added a commit to do a small refactor of parse_ntp_conf / write_ntp_conf first.

[changlei-li]

Thank you. I have no more comments now. If you have manual tests, you can paste the test results here.

As I mentioned, this change needs to be released together with the corresponding changes in xsconsole and host-installer. I will keep the change request here to avoid it is merged unintentionally until the changes in those public repos are done.
I can handle the XenRT test cases on our side when all is settled.