[PATCH] devconfig: fix setting node timezone

[PATCH] devconfig: fix setting node timezone

[Luis Chamberlain]

It turns out that just relying on the clock offset being set won't
ensure we have the right time set on the nodes, so when timectl is
enabled for NTP, we still need to set the timezone on the nodes based
on the hypervisor to be very sure we get the right time.

Below is an example where the clock offset XML settings were set on
guests with guestfs and yet the timezone on the nodes was set to
Eastern. To fix this we must keep a kconfig option which matches the
hypervisor, and use that value on the nodes to set the timezone.

mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ grep "clock offset" guestfs/base-xfs-nocrc/base-xfs-nocrc.xml
  <clock offset='localtime'>
mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ ssh base-xfs-reflink timedatectl
               Local time: Mon 2024-04-08 17:32:38 EDT
           Universal time: Mon 2024-04-08 21:32:38 UTC
                 RTC time: Mon 2024-04-08 21:32:38
                Time zone: US/Eastern (EDT, -0400)
System clock synchronized: yes
              NTP service: active
          RTC in local TZ: no
mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ timedatectl
               Local time: Mon 2024-04-08 21:32:42 UTC
           Universal time: Mon 2024-04-08 21:32:42 UTC
                 RTC time: Mon 2024-04-08 21:32:42
                Time zone: UTC (UTC, +0000)
System clock synchronized: yes
              NTP service: active
          RTC in local TZ: no

Fixes: de233a386941 ("devconfig: Remove DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_TIMEZONE"
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 Makefile.min_deps                           | 1 +
 kconfigs/Kconfig.ansible_provisioning       | 4 ++++
 playbooks/roles/devconfig/defaults/main.yml | 1 +
 playbooks/roles/devconfig/tasks/main.yml    | 9 +++++++++
 scripts/systemd-timesync.Makefile           | 1 +
 5 files changed, 16 insertions(+)

diff --git a/Makefile.min_deps b/Makefile.min_deps
index 9d8f205799c4..ce0a82595074 100644
--- a/Makefile.min_deps
+++ b/Makefile.min_deps
@@ -10,6 +10,7 @@ BINARY_DEPS += sudo
 BINARY_DEPS += make
 BINARY_DEPS += nc
 BINARY_DEPS += ansible-playbook
+BINARY_DEPS += timedatectl
 
 BINARY_DEPS_VCHECKS := $(subst $(TOPDIR)/,,$(wildcard $(TOPDIR)/scripts/version_check/*))
 
diff --git a/kconfigs/Kconfig.ansible_provisioning b/kconfigs/Kconfig.ansible_provisioning
index 82a6f822c3f5..347992e24ab2 100644
--- a/kconfigs/Kconfig.ansible_provisioning
+++ b/kconfigs/Kconfig.ansible_provisioning
@@ -217,6 +217,10 @@ config DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD
 
 if DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD
 
+config DEVCONFIG_SYSTEMD_TIMESYNCD_TIMEZONE
+	string
+	default $(shell, timedatectl show -p Timezone --value)
+
 config DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_NTP
 	bool "Enable systemd-timesyncd NTP"
 	default y
diff --git a/playbooks/roles/devconfig/defaults/main.yml b/playbooks/roles/devconfig/defaults/main.yml
index b48b4cce8e00..3fe2605c87e8 100644
--- a/playbooks/roles/devconfig/defaults/main.yml
+++ b/playbooks/roles/devconfig/defaults/main.yml
@@ -50,6 +50,7 @@ devconfig_systemd_journal_remote_url: "http://192.168.124.1"
 devconfig_systemd_journal_remote_path: "/var/log/journal/remote/"
 
 devconfig_enable_systemd_timesyncd: False
+devconfig_systemd_timesyncd_timezone: "UTC"
 devconfig_enable_systemd_timesyncd_ntp: False
 devconfig_enable_systemd_timesyncd_ntp_google: False
 devconfig_enable_systemd_timesyncd_ntp_debian: False
diff --git a/playbooks/roles/devconfig/tasks/main.yml b/playbooks/roles/devconfig/tasks/main.yml
index fec6ea3b5939..a76106b2a997 100644
--- a/playbooks/roles/devconfig/tasks/main.yml
+++ b/playbooks/roles/devconfig/tasks/main.yml
@@ -526,6 +526,15 @@
   when:
     - devconfig_enable_systemd_timesyncd_ntp|bool
 
+- name: Override timezone on nodes
+  tags: [ 'timesyncd' ]
+  become: yes
+  become_flags: 'su - -c'
+  become_method: sudo
+  command: "timedatectl set-timezone {{ devconfig_systemd_timesyncd_timezone }}"
+  when:
+    - devconfig_enable_systemd_timesyncd_ntp|bool
+
 - name: Restart systemd-timesyncd.service on the client
   become: yes
   become_flags: 'su - -c'
diff --git a/scripts/systemd-timesync.Makefile b/scripts/systemd-timesync.Makefile
index 11950cd9e598..d11d135113d8 100644
--- a/scripts/systemd-timesync.Makefile
+++ b/scripts/systemd-timesync.Makefile
@@ -2,6 +2,7 @@
 
 ifeq (y,$(CONFIG_DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD))
 ANSIBLE_EXTRA_ARGS += devconfig_enable_systemd_timesyncd='True'
+ANSIBLE_EXTRA_ARGS_DIRECT += devconfig_systemd_timesyncd_timezone='$(subst ",,$(CONFIG_DEVCONFIG_SYSTEMD_TIMESYNCD_TIMEZONE))'
 
 ifeq (y,$(CONFIG_DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_NTP))
 ANSIBLE_EXTRA_ARGS += devconfig_enable_systemd_timesyncd_ntp='True'
-- 
2.43.0

Re: [PATCH] devconfig: fix setting node timezone

[Chuck Lever]

On Mon, Apr 08, 2024 at 03:25:15PM -0700, Luis Chamberlain wrote:
> It turns out that just relying on the clock offset being set won't
> ensure we have the right time set on the nodes, so when timectl is
> enabled for NTP, we still need to set the timezone on the nodes based
> on the hypervisor to be very sure we get the right time.
> 
> Below is an example where the clock offset XML settings were set on
> guests with guestfs and yet the timezone on the nodes was set to
> Eastern. To fix this we must keep a kconfig option which matches the
> hypervisor, and use that value on the nodes to set the timezone.

Hi Luis,

IMO you've demonstrated that the Ansible timezone plugin is not
working as expected, not that kdevops has to do something special.
I'd really like to see a root cause for this misbehavior, because
this is definitely not what I expect to see from that plug-in.

For example, does this happen with non-Debian target nodes? Does
it happen when timesyncd and NTP are not enabled?


> mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ grep "clock offset" guestfs/base-xfs-nocrc/base-xfs-nocrc.xml
>   <clock offset='localtime'>
> mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ ssh base-xfs-reflink timedatectl
>                Local time: Mon 2024-04-08 17:32:38 EDT
>            Universal time: Mon 2024-04-08 21:32:38 UTC
>                  RTC time: Mon 2024-04-08 21:32:38
>                 Time zone: US/Eastern (EDT, -0400)
> System clock synchronized: yes
>               NTP service: active
>           RTC in local TZ: no
> mcgrof@some-hypervisor-01 /xfs1/base/kdevops (git::main)$ timedatectl
>                Local time: Mon 2024-04-08 21:32:42 UTC
>            Universal time: Mon 2024-04-08 21:32:42 UTC
>                  RTC time: Mon 2024-04-08 21:32:42
>                 Time zone: UTC (UTC, +0000)
> System clock synchronized: yes
>               NTP service: active
>           RTC in local TZ: no
> 
> Fixes: de233a386941 ("devconfig: Remove DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_TIMEZONE"
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  Makefile.min_deps                           | 1 +
>  kconfigs/Kconfig.ansible_provisioning       | 4 ++++
>  playbooks/roles/devconfig/defaults/main.yml | 1 +
>  playbooks/roles/devconfig/tasks/main.yml    | 9 +++++++++
>  scripts/systemd-timesync.Makefile           | 1 +
>  5 files changed, 16 insertions(+)
> 
> diff --git a/Makefile.min_deps b/Makefile.min_deps
> index 9d8f205799c4..ce0a82595074 100644
> --- a/Makefile.min_deps
> +++ b/Makefile.min_deps
> @@ -10,6 +10,7 @@ BINARY_DEPS += sudo
>  BINARY_DEPS += make
>  BINARY_DEPS += nc
>  BINARY_DEPS += ansible-playbook
> +BINARY_DEPS += timedatectl
>  
>  BINARY_DEPS_VCHECKS := $(subst $(TOPDIR)/,,$(wildcard $(TOPDIR)/scripts/version_check/*))
>  
> diff --git a/kconfigs/Kconfig.ansible_provisioning b/kconfigs/Kconfig.ansible_provisioning
> index 82a6f822c3f5..347992e24ab2 100644
> --- a/kconfigs/Kconfig.ansible_provisioning
> +++ b/kconfigs/Kconfig.ansible_provisioning
> @@ -217,6 +217,10 @@ config DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD
>  
>  if DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD
>  
> +config DEVCONFIG_SYSTEMD_TIMESYNCD_TIMEZONE
> +	string
> +	default $(shell, timedatectl show -p Timezone --value)
> +
>  config DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_NTP
>  	bool "Enable systemd-timesyncd NTP"
>  	default y
> diff --git a/playbooks/roles/devconfig/defaults/main.yml b/playbooks/roles/devconfig/defaults/main.yml
> index b48b4cce8e00..3fe2605c87e8 100644
> --- a/playbooks/roles/devconfig/defaults/main.yml
> +++ b/playbooks/roles/devconfig/defaults/main.yml
> @@ -50,6 +50,7 @@ devconfig_systemd_journal_remote_url: "http://192.168.124.1"
>  devconfig_systemd_journal_remote_path: "/var/log/journal/remote/"
>  
>  devconfig_enable_systemd_timesyncd: False
> +devconfig_systemd_timesyncd_timezone: "UTC"
>  devconfig_enable_systemd_timesyncd_ntp: False
>  devconfig_enable_systemd_timesyncd_ntp_google: False
>  devconfig_enable_systemd_timesyncd_ntp_debian: False
> diff --git a/playbooks/roles/devconfig/tasks/main.yml b/playbooks/roles/devconfig/tasks/main.yml
> index fec6ea3b5939..a76106b2a997 100644
> --- a/playbooks/roles/devconfig/tasks/main.yml
> +++ b/playbooks/roles/devconfig/tasks/main.yml
> @@ -526,6 +526,15 @@
>    when:
>      - devconfig_enable_systemd_timesyncd_ntp|bool
>  
> +- name: Override timezone on nodes
> +  tags: [ 'timesyncd' ]
> +  become: yes
> +  become_flags: 'su - -c'
> +  become_method: sudo
> +  command: "timedatectl set-timezone {{ devconfig_systemd_timesyncd_timezone }}"
> +  when:
> +    - devconfig_enable_systemd_timesyncd_ntp|bool
> +
>  - name: Restart systemd-timesyncd.service on the client
>    become: yes
>    become_flags: 'su - -c'
> diff --git a/scripts/systemd-timesync.Makefile b/scripts/systemd-timesync.Makefile
> index 11950cd9e598..d11d135113d8 100644
> --- a/scripts/systemd-timesync.Makefile
> +++ b/scripts/systemd-timesync.Makefile
> @@ -2,6 +2,7 @@
>  
>  ifeq (y,$(CONFIG_DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD))
>  ANSIBLE_EXTRA_ARGS += devconfig_enable_systemd_timesyncd='True'
> +ANSIBLE_EXTRA_ARGS_DIRECT += devconfig_systemd_timesyncd_timezone='$(subst ",,$(CONFIG_DEVCONFIG_SYSTEMD_TIMESYNCD_TIMEZONE))'
>  
>  ifeq (y,$(CONFIG_DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_NTP))
>  ANSIBLE_EXTRA_ARGS += devconfig_enable_systemd_timesyncd_ntp='True'
> -- 
> 2.43.0
> 
> 

-- 
Chuck Lever

Re: [PATCH] devconfig: fix setting node timezone

[Luis Chamberlain]

On Mon, Apr 08, 2024 at 08:53:20PM -0400, Chuck Lever wrote:
> On Mon, Apr 08, 2024 at 03:25:15PM -0700, Luis Chamberlain wrote:
> > It turns out that just relying on the clock offset being set won't
> > ensure we have the right time set on the nodes, so when timectl is
> > enabled for NTP, we still need to set the timezone on the nodes based
> > on the hypervisor to be very sure we get the right time.
> > 
> > Below is an example where the clock offset XML settings were set on
> > guests with guestfs and yet the timezone on the nodes was set to
> > Eastern. To fix this we must keep a kconfig option which matches the
> > hypervisor, and use that value on the nodes to set the timezone.
> 
> Hi Luis,
> 
> IMO you've demonstrated that the Ansible timezone plugin is not
> working as expected,

Why is the timezone plugin involved here? I though the replacement
was:

-  <clock offset='utc'>
+  <clock offset='localtime'>

And so its not clear to me how the ansible plugin is involved?

I checked the docs for libvirt and the above just sets the time at boot,
if anything I think we need timezone setting like:

<clock offset="timezone" timezone="Europe/Paris" />

So one might think this is alternative:

diff --git a/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml b/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
index eb5118a105f0..b9ba971fa27c 100644
--- a/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
+++ b/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
@@ -12,7 +12,7 @@
     <apic/>
   </features>
   <cpu mode='{{ 'host-passthrough' if libvirt_host_passthrough else 'host-model' }}' migratable='off'/>
-  <clock offset='localtime'>
+  <clock offset="timezone" timezone="{{ kdevops_host_timezone.stdout }}">
     <timer name='rtc' tickpolicy='catchup'/>
     <timer name='pit' tickpolicy='delay'/>
     <timer name='hpet' present='no'/>
diff --git a/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml b/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
index 1061f2cc45d4..8dc962366319 100644
--- a/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
+++ b/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
@@ -17,7 +17,7 @@
     <gic version='3'/>
   </features>
   <cpu mode='{{ 'host-passthrough' if libvirt_host_passthrough else 'host-model' }}' migratable='off'/>
-  <clock offset='localtime'/>
+  <clock offset="timezone" timezone="{{ kdevops_host_timezone.stdout }}"/>
   <on_poweroff>destroy</on_poweroff>
   <on_reboot>restart</on_reboot>
   <on_crash>destroy</on_crash>

The kdevops_host_timezone is only used to set the vagrant timezone, for
guestfs the localtime is used currently. I thought that on commit
de233a386941 ("devconfig: Remove DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_TIMEZONE")
the above change would suffice for guestfs but it does not.

But the above is not sufficient, we end up in the same situaiton still.
I suspect its the guest image that has a timezone set. And so the
timezone needs to be set with virt-sysprep at least:

diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
index ed07c04d6b74..dbace8d5d502 100755
--- a/scripts/bringup_guestfs.sh
+++ b/scripts/bringup_guestfs.sh
@@ -113,7 +113,8 @@ do
 	# Copy the base image and prep it
 	ROOTIMG="$STORAGEDIR/$name/root.raw"
 	cp --reflink=auto $BASE_IMAGE $ROOTIMG
-	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
+	TZ="$(timedatectl show -p Timezone --value)"
+	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
 
 	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
 		lbs_idx=1

> For example, does this happen with non-Debian target nodes? Does
> it happen when timesyncd and NTP are not enabled?

I'm using debian and just enabling systemd-remote-journal, the time zone
was set correctly before but after commit de233a386941 we loose that
as the libvirt localtime does not actually do what we really want it
would seem.

The above two hunks are a decent replacement patch for this issue as well.

  Luis

Re: [PATCH] devconfig: fix setting node timezone

[Chuck Lever]

On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> On Mon, Apr 08, 2024 at 08:53:20PM -0400, Chuck Lever wrote:
> > On Mon, Apr 08, 2024 at 03:25:15PM -0700, Luis Chamberlain wrote:
> > > It turns out that just relying on the clock offset being set won't
> > > ensure we have the right time set on the nodes, so when timectl is
> > > enabled for NTP, we still need to set the timezone on the nodes based
> > > on the hypervisor to be very sure we get the right time.
> > > 
> > > Below is an example where the clock offset XML settings were set on
> > > guests with guestfs and yet the timezone on the nodes was set to
> > > Eastern. To fix this we must keep a kconfig option which matches the
> > > hypervisor, and use that value on the nodes to set the timezone.
> > 
> > Hi Luis,
> > 
> > IMO you've demonstrated that the Ansible timezone plugin is not
> > working as expected,
> 
> Why is the timezone plugin involved here? I though the replacement
> was:
> 
> -  <clock offset='utc'>
> +  <clock offset='localtime'>
> 
> And so its not clear to me how the ansible plugin is involved?

Ah. I misremembered: the plugin handles Vagrant timezones. You are
reporting the problem with guestfs. Got it.


> I checked the docs for libvirt and the above just sets the time at boot,
> if anything I think we need timezone setting like:
> 
> <clock offset="timezone" timezone="Europe/Paris" />

I'm looking at:

https://libvirt.org/formatdomain.html

> localtime
> The guest clock will be synchronized to the host's configured
> timezone when booted, if any. Since 0.9.11, the adjustment
> attribute behaves the same as in 'utc' mode.
> 
> timezone
> The guest clock will be synchronized to the requested timezone
> using the timezone attribute. Since 0.7.7

In my testing with Fedora guests, "localtime" always copied the
timezone from the hypervisor, as expected.


> So one might think this is alternative:
> 
> diff --git a/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml b/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
> index eb5118a105f0..b9ba971fa27c 100644
> --- a/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
> +++ b/playbooks/roles/gen_nodes/templates/guestfs_q35.j2.xml
> @@ -12,7 +12,7 @@
>      <apic/>
>    </features>
>    <cpu mode='{{ 'host-passthrough' if libvirt_host_passthrough else 'host-model' }}' migratable='off'/>
> -  <clock offset='localtime'>
> +  <clock offset="timezone" timezone="{{ kdevops_host_timezone.stdout }}">
>      <timer name='rtc' tickpolicy='catchup'/>
>      <timer name='pit' tickpolicy='delay'/>
>      <timer name='hpet' present='no'/>
> diff --git a/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml b/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
> index 1061f2cc45d4..8dc962366319 100644
> --- a/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
> +++ b/playbooks/roles/gen_nodes/templates/guestfs_virt.j2.xml
> @@ -17,7 +17,7 @@
>      <gic version='3'/>
>    </features>
>    <cpu mode='{{ 'host-passthrough' if libvirt_host_passthrough else 'host-model' }}' migratable='off'/>
> -  <clock offset='localtime'/>
> +  <clock offset="timezone" timezone="{{ kdevops_host_timezone.stdout }}"/>
>    <on_poweroff>destroy</on_poweroff>
>    <on_reboot>restart</on_reboot>
>    <on_crash>destroy</on_crash>
> 
> The kdevops_host_timezone is only used to set the vagrant timezone, for
> guestfs the localtime is used currently. I thought that on commit
> de233a386941 ("devconfig: Remove DEVCONFIG_ENABLE_SYSTEMD_TIMESYNCD_TIMEZONE")
> the above change would suffice for guestfs but it does not.

I would expect the above to behave exactly the same as

  <clock offset='localtime'>

And, I guess it does. ;-)


> But the above is not sufficient, we end up in the same situaiton still.
> I suspect its the guest image that has a timezone set.

Which is why I think this is related to the distro being provisioned
in the guest.

But I thought you had tested these patches before I merged them, on
what I assume was Debian. I wonder what changed.


> And so the timezone needs to be set with virt-sysprep at least:
> 
> diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> index ed07c04d6b74..dbace8d5d502 100755
> --- a/scripts/bringup_guestfs.sh
> +++ b/scripts/bringup_guestfs.sh
> @@ -113,7 +113,8 @@ do
>  	# Copy the base image and prep it
>  	ROOTIMG="$STORAGEDIR/$name/root.raw"
>  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> +	TZ="$(timedatectl show -p Timezone --value)"
> +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
>  
>  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
>  		lbs_idx=1
> 
> > For example, does this happen with non-Debian target nodes? Does
> > it happen when timesyncd and NTP are not enabled?
> 
> I'm using debian and just enabling systemd-remote-journal, the time zone
> was set correctly before but after commit de233a386941 we loose that
> as the libvirt localtime does not actually do what we really want it
> would seem.
> 
> The above two hunks are a decent replacement patch for this issue as well.

Two hunks? I thought just this last hunk was what actually fixed it.

I'm not convinced I understand exactly what's going on. I can try to
reproduce it here later today. But I would prefer not having the
extra Kconfig timezone noise, so I like this last hunk better than
the first patch you posted.


-- 
Chuck Lever

Re: [PATCH] devconfig: fix setting node timezone

[Chuck Lever]

On Tue, Apr 09, 2024 at 09:29:29AM -0400, Chuck Lever wrote:
> On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> > And so the timezone needs to be set with virt-sysprep at least:
> > 
> > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > index ed07c04d6b74..dbace8d5d502 100755
> > --- a/scripts/bringup_guestfs.sh
> > +++ b/scripts/bringup_guestfs.sh
> > @@ -113,7 +113,8 @@ do
> >  	# Copy the base image and prep it
> >  	ROOTIMG="$STORAGEDIR/$name/root.raw"
> >  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> > -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> > +	TZ="$(timedatectl show -p Timezone --value)"
> > +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
> >  
> >  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
> >  		lbs_idx=1
> > 
> > > For example, does this happen with non-Debian target nodes? Does
> > > it happen when timesyncd and NTP are not enabled?
> > 
> > I'm using debian and just enabling systemd-remote-journal, the time zone
> > was set correctly before but after commit de233a386941 we loose that
> > as the libvirt localtime does not actually do what we really want it
> > would seem.
> > 
> > The above two hunks are a decent replacement patch for this issue as well.
> 
> Two hunks? I thought just this last hunk was what actually fixed it.

I added the scripts/bringup_guestfs.sh hunk, and now I see:

[   5.2] Performing "utmp" ...
[   5.2] Performing "yum-uuid" ...
[   5.2] Performing "customize" ...
[   5.2] Setting a random seed
[   5.2] Setting the machine ID in /etc/machine-id
[   5.2] Setting the hostname: kdevops-nfs-v40
[   5.2] SSH key inject: kdevops
[   6.3] Setting the timezone: America/New_York   <<<<<<<
[   6.3] SELinux relabelling
[  18.6] Performing "lvm-uuids" ...

Which wasn't there before.


-- 
Chuck Lever

Re: [PATCH] devconfig: fix setting node timezone

[Chuck Lever]

On Tue, Apr 09, 2024 at 10:22:30AM -0400, Chuck Lever wrote:
> On Tue, Apr 09, 2024 at 09:29:29AM -0400, Chuck Lever wrote:
> > On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> > > And so the timezone needs to be set with virt-sysprep at least:
> > > 
> > > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > > index ed07c04d6b74..dbace8d5d502 100755
> > > --- a/scripts/bringup_guestfs.sh
> > > +++ b/scripts/bringup_guestfs.sh
> > > @@ -113,7 +113,8 @@ do
> > >  	# Copy the base image and prep it
> > >  	ROOTIMG="$STORAGEDIR/$name/root.raw"
> > >  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> > > -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> > > +	TZ="$(timedatectl show -p Timezone --value)"
> > > +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
> > >  
> > >  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
> > >  		lbs_idx=1
> > > 
> > > > For example, does this happen with non-Debian target nodes? Does
> > > > it happen when timesyncd and NTP are not enabled?
> > > 
> > > I'm using debian and just enabling systemd-remote-journal, the time zone
> > > was set correctly before but after commit de233a386941 we loose that
> > > as the libvirt localtime does not actually do what we really want it
> > > would seem.
> > > 
> > > The above two hunks are a decent replacement patch for this issue as well.
> > 
> > Two hunks? I thought just this last hunk was what actually fixed it.
> 
> I added the scripts/bringup_guestfs.sh hunk, and now I see:
> 
> [   5.2] Performing "utmp" ...
> [   5.2] Performing "yum-uuid" ...
> [   5.2] Performing "customize" ...
> [   5.2] Setting a random seed
> [   5.2] Setting the machine ID in /etc/machine-id
> [   5.2] Setting the hostname: kdevops-nfs-v40
> [   5.2] SSH key inject: kdevops
> [   6.3] Setting the timezone: America/New_York   <<<<<<<
> [   6.3] SELinux relabelling
> [  18.6] Performing "lvm-uuids" ...
> 
> Which wasn't there before.

Confirmed: without the above hunk, the guest timezone is not
set to the same timezone as the hypervisor.

I think the intent of f98ef7586c2 ("guestfs: Use libvirt to set
target node timezone") was to make guestfs copy the hypervisor
timezone, so I would argue that that is the real broken commit.


-- 
Chuck Lever

Re: [PATCH] devconfig: fix setting node timezone

[Luis Chamberlain]

On Tue, Apr 09, 2024 at 09:29:29AM -0400, Chuck Lever wrote:
> On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> 
> But I thought you had tested these patches before I merged them, on
> what I assume was Debian. I wonder what changed.

I thought I did too.

> > And so the timezone needs to be set with virt-sysprep at least:
> > 
> > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > index ed07c04d6b74..dbace8d5d502 100755
> > --- a/scripts/bringup_guestfs.sh
> > +++ b/scripts/bringup_guestfs.sh
> > @@ -113,7 +113,8 @@ do
> >  	# Copy the base image and prep it
> >  	ROOTIMG="$STORAGEDIR/$name/root.raw"
> >  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> > -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> > +	TZ="$(timedatectl show -p Timezone --value)"
> > +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
> >  
> >  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
> >  		lbs_idx=1
> > 
> > > For example, does this happen with non-Debian target nodes? Does
> > > it happen when timesyncd and NTP are not enabled?
> > 
> > I'm using debian and just enabling systemd-remote-journal, the time zone
> > was set correctly before but after commit de233a386941 we loose that
> > as the libvirt localtime does not actually do what we really want it
> > would seem.
> > 
> > The above two hunks are a decent replacement patch for this issue as well.
> 
> Two hunks? I thought just this last hunk was what actually fixed it.

Sure this last hunk quoted above would fix it.

> I'm not convinced I understand exactly what's going on. I can try to
> reproduce it here later today. But I would prefer not having the
> extra Kconfig timezone noise, so I like this last hunk better than
> the first patch you posted.

Sure I can just merge this small one hunk if you give me a Acked-by here.

  Luis

Re: [PATCH] devconfig: fix setting node timezone

[Chuck Lever]

On Wed, Apr 10, 2024 at 08:43:47AM -0700, Luis Chamberlain wrote:
> On Tue, Apr 09, 2024 at 09:29:29AM -0400, Chuck Lever wrote:
> > On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> > 
> > But I thought you had tested these patches before I merged them, on
> > what I assume was Debian. I wonder what changed.
> 
> I thought I did too.
> 
> > > And so the timezone needs to be set with virt-sysprep at least:
> > > 
> > > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > > index ed07c04d6b74..dbace8d5d502 100755
> > > --- a/scripts/bringup_guestfs.sh
> > > +++ b/scripts/bringup_guestfs.sh
> > > @@ -113,7 +113,8 @@ do
> > >  	# Copy the base image and prep it
> > >  	ROOTIMG="$STORAGEDIR/$name/root.raw"
> > >  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> > > -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> > > +	TZ="$(timedatectl show -p Timezone --value)"
> > > +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
> > >  
> > >  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
> > >  		lbs_idx=1
> > > 
> > > > For example, does this happen with non-Debian target nodes? Does
> > > > it happen when timesyncd and NTP are not enabled?
> > > 
> > > I'm using debian and just enabling systemd-remote-journal, the time zone
> > > was set correctly before but after commit de233a386941 we loose that
> > > as the libvirt localtime does not actually do what we really want it
> > > would seem.
> > > 
> > > The above two hunks are a decent replacement patch for this issue as well.
> > 
> > Two hunks? I thought just this last hunk was what actually fixed it.
> 
> Sure this last hunk quoted above would fix it.
> 
> > I'm not convinced I understand exactly what's going on. I can try to
> > reproduce it here later today. But I would prefer not having the
> > extra Kconfig timezone noise, so I like this last hunk better than
> > the first patch you posted.
> 
> Sure I can just merge this small one hunk if you give me a Acked-by here.

Acked-by: Chuck Lever <chuck.lever@oracle.com>

Tested-by: Chuck Lever <chuck.lever@oracle.com>

Pick one or both ;-)


-- 
Chuck Lever

Re: [PATCH] devconfig: fix setting node timezone

[Luis Chamberlain]

On Wed, Apr 10, 2024 at 11:45:52AM -0400, Chuck Lever wrote:
> On Wed, Apr 10, 2024 at 08:43:47AM -0700, Luis Chamberlain wrote:
> > On Tue, Apr 09, 2024 at 09:29:29AM -0400, Chuck Lever wrote:
> > > On Mon, Apr 08, 2024 at 08:36:57PM -0700, Luis Chamberlain wrote:
> > > 
> > > But I thought you had tested these patches before I merged them, on
> > > what I assume was Debian. I wonder what changed.
> > 
> > I thought I did too.
> > 
> > > > And so the timezone needs to be set with virt-sysprep at least:
> > > > 
> > > > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > > > index ed07c04d6b74..dbace8d5d502 100755
> > > > --- a/scripts/bringup_guestfs.sh
> > > > +++ b/scripts/bringup_guestfs.sh
> > > > @@ -113,7 +113,8 @@ do
> > > >  	# Copy the base image and prep it
> > > >  	ROOTIMG="$STORAGEDIR/$name/root.raw"
> > > >  	cp --reflink=auto $BASE_IMAGE $ROOTIMG
> > > > -	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub"
> > > > +	TZ="$(timedatectl show -p Timezone --value)"
> > > > +	virt-sysprep -a $ROOTIMG --hostname $name --ssh-inject "kdevops:file:$SSH_KEY.pub" --timezone $TZ
> > > >  
> > > >  	if [[ "$CONFIG_LIBVIRT_ENABLE_LARGEIO" == "y" ]]; then
> > > >  		lbs_idx=1
> > > > 
> > > > > For example, does this happen with non-Debian target nodes? Does
> > > > > it happen when timesyncd and NTP are not enabled?
> > > > 
> > > > I'm using debian and just enabling systemd-remote-journal, the time zone
> > > > was set correctly before but after commit de233a386941 we loose that
> > > > as the libvirt localtime does not actually do what we really want it
> > > > would seem.
> > > > 
> > > > The above two hunks are a decent replacement patch for this issue as well.
> > > 
> > > Two hunks? I thought just this last hunk was what actually fixed it.
> > 
> > Sure this last hunk quoted above would fix it.
> > 
> > > I'm not convinced I understand exactly what's going on. I can try to
> > > reproduce it here later today. But I would prefer not having the
> > > extra Kconfig timezone noise, so I like this last hunk better than
> > > the first patch you posted.
> > 
> > Sure I can just merge this small one hunk if you give me a Acked-by here.
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>
> 
> Tested-by: Chuck Lever <chuck.lever@oracle.com>
> 
> Pick one or both ;-)

Pushed.

  Luis