feat: propagate host timezone to guest #1931

lobshunter · 2023-10-20T14:18:30Z

This PR closes #1921, propagates host timezone to guest VM.

This doesn't work for template://alpine for it doesn't have pre-installed tzdata. And this PR doesn't use timedatectl because some distros might not be using systemd.

balajiv113 · 2023-10-20T14:33:40Z

@lobshunter Thanks for the PR.

Can you try out setting timezone via cloud-init (via pkg/cidata/)
https://cloudinit.readthedocs.io/en/latest/reference/modules.html#timezone

This way it will set for all distro supporting cloud-init. Also will be set early in boot

jandubois · 2023-10-20T16:16:38Z

This doesn't work for template://alpine for it doesn't have pre-installed tzdata.

We can add tzdata to the image; it is just 3MB or so.

If you change the timezone via cloud-init, then we also have to update lima-init to support it, as we currently don't use cloud-init in our Alpine image.

lobshunter · 2023-10-21T03:31:45Z

Can you try out setting timezone via cloud-init (via pkg/cidata/)

Hi, I updated the cloud-init version in the second commit. PTAL which one do you prefer.

We can add tzdata to the image; it is just 3MB or so.

It's 3068 KiB for alpine:3.18.3. Should I update it here?

as we currently don't use cloud-init in our Alpine image

I am confused by this actually. Alpine doesn't have cloud-init installed, but it did run the new added 60-propagate-timezone.sh script.

jandubois

I'm not happy that this PR cannot support timezones with fractional-hours offsets from GMT. E.g. India is GMT+5:30, and there are plenty more.

I wonder if there isn't a way to get an exact match to the host timezone?

pkg/cidata/cidata.TEMPLATE.d/boot/60-propagate-timezone.sh

pkg/cidata/cidata.TEMPLATE.d/lima.env

pkg/cidata/cidata.go

jandubois · 2023-10-21T03:51:48Z

Please also address linter errors!

jandubois · 2023-10-21T03:58:51Z

I wonder if there isn't a way to get an exact match to the host timezone?

macOS sets /etc/localtime to a symlink too, so we could scrape it from there:

$ readlink /etc/localtime | sed 's#.*zoneinfo/##'
America/Vancouver

jandubois · 2023-10-21T04:10:01Z

Hi, I updated the cloud-init version in the second commit.

I think this is not what @balajiv113 meant. cloud-init has support for setting timezone built-in. You just have to add a line to user-data, e.g.

timezone: US/Eastern

You would not need the 60-propagate-timezone.sh script because cloud-init would already do the work for you.

Please ignore my earlier review and use the proper cloud-init mechanism instead.

Alpine doesn't have cloud-init installed, but it did run the new added 60-propagate-timezone.sh script.

It uses lima-init, which implements just a subset of cloud-init using shell and awk. It doesn't have support for the timezone key in user-data, so that would have to be added. It does support running the boot script, which in turn runs the provisioning scripts.

lobshunter · 2023-10-21T04:35:45Z

use the proper cloud-init mechanism instead.

Yes, will do.

I'm not happy that this PR cannot support timezones with fractional-hours offsets from GMT

Me neither, but I couldn't find a proper way after digging for half an hour. This is the simplest one I got.

I wonder if there isn't a way to get an exact match to the host timezone?

I did try, but the problem is Lima currently supports multiple host platforms (Linux/MacOS/Windows(that's WSL2 IIUC)). Especially for Windows, I didn't find a portable way to get the host timezone. Using Golang's built-in function would be the most portable one, but it doesn't provide the get timezone in geographic location format.

I noticed location timezone can be retrieved in /etc/localtime on MacOS and /etc/timezone on Linux(Ubuntu & Manjaro). But didn't have a Windows setup for testing. Also using platform-specific approach reduces the testability of code. I'll do more study on this.

jandubois · 2023-10-21T05:13:19Z

Especially for Windows, I didn't find a portable way to get the host timezone.

Yeah, it is almost impossible to map Windows time zones to IANA (tzdata) time zone names. There is a Python app that seems to implement it.

However, I think WSL2 already sets this up by itself, so we may not need this. Well, I saw it set up correctly in Ubuntu, but not in Alpine. I wonder if this would work in Alpine if the tzdata package was installed, but my Windows machine is in a data centre in a different country and seems to have network issues connecting to the Alpine package CDN right now.

I don't have time to research this further right now, but if Windows doesn't need to be solved, then the solution might turn out to be simple.

jandubois · 2023-10-21T05:23:46Z

I wonder if this would work in Alpine if the tzdata package was installed

I gave it one more try, and it did indeed work. After apk add tzdata I shut down the distro. When I started it up again, the following 2 files appeared automatically:

/ # cat /etc/timezone
America/Los_Angeles
/ # ls -l /etc/localtime
lrwxrwxrwx    1 root     0               39 Oct 20 22:19 /etc/localtime -> /usr/share/zoneinfo/America/Los_Angeles

Which means we don't need to propagate the host timezone to the guest on WSL2.

lobshunter · 2023-10-29T09:30:21Z

pkg/cidata/cidata.go

+	logrus.Debugf("cannot get timezone from /etc/timezone: %s", err)
+
+	// 3. try to get timezone from /etc/localtime soft link
+	// base on the assumption that <zoneinfo_basedir>/zone.tab file exists


Haven't found any official documents about zoneinfo data file hierarchy, just assume zone.tab file exists.

I don't know either. Right now it seems like a safe assumption, but there are 2 comments in the file that might want us to look for safer alternatives:

# tzdb timezone descriptions (deprecated version) [...] # This file is intended as a backward-compatibility aid for older programs.

Maybe we should look for a directory named Antarctica (or a file named Etc/UTC) instead?

jandubois · 2023-10-31T06:45:45Z

@lobshunter I see you updated this PR, but it is still marked as a draft. Not sure if you are still working on it or not. When you think it is ready for another review, please move it out of draft mode and re-request a review!

Signed-off-by: pengyu <lobshunter86@gmail.com>

jandubois

I strongly feel that we don't want to look at TZ here, plus some additional comments.

jandubois · 2023-11-06T06:29:03Z

pkg/cidata/cidata.go

+	tzEnv := os.Getenv("TZ")
+	if tzEnv != "" &&
+		// skip if TZ is in "std offset dst offset, rule" format
+		!strings.Contains(tzEnv, " ") {
+		return tzEnv
+	}


I think trying to parse TZ is futile; there are just too many different possible formats, and different C runtime libraries implement them differently.

glibc specification

POSIX specification

Neither of them show any spaces in the "std offset dst offset" format, and I believe spaces are not even allowed optionally.

Furthermore, cloud-init only allows settings corresponding to zoneinfo entries:

timezone: (string) The timezone to use as represented in /usr/share/zoneinfo

If we want to support importing the TZ environment variable, then I think it should be optional, and added to /etc/environment. There is some precedent for doing this (we have logic for proxy settings), but this should be a separate issue, if we decide to do this at all. For this PR I think you should concentrate on synchronizing the /etc/localtime and /etc/timezone settings, and nothing else.

jandubois · 2023-11-06T06:36:38Z

pkg/cidata/cidata.go

+	logrus.Debugf("cannot get timezone from /etc/timezone: %s", err)
+
+	// 3. try to get timezone from /etc/localtime soft link
+	// base on the assumption that <zoneinfo_basedir>/zone.tab file exists


I don't know either. Right now it seems like a safe assumption, but there are 2 comments in the file that might want us to look for safer alternatives:

# tzdb timezone descriptions (deprecated version) [...] # This file is intended as a backward-compatibility aid for older programs.

Maybe we should look for a directory named Antarctica (or a file named Etc/UTC) instead?

jandubois · 2023-11-06T06:45:25Z

pkg/cidata/cidata.go

+			tz := strings.TrimPrefix(tzFilePath, baseDir+"/")
+			_, err = time.LoadLocation(tz)
+			if err == nil {
+				return tz
+			}


I don't think the check if time.LoadLocation succeeds is useful. It is the setting that has been configured, so if the datafile somehow got corrupted or deleted, it is not our problem. The interesting question would be if the data file exists inside the VM, and we can't answer that anyways.

If we kept the check, then I would still argue that we need to return "" in the failure case, and not to keep looking in higher levels of the directory tree; that seems to be wrong either way.

Suggested change

tz := strings.TrimPrefix(tzFilePath, baseDir+"/")

_, err = time.LoadLocation(tz)

if err == nil {

return tz

}

return strings.TrimPrefix(tzFilePath, baseDir+"/")

jandubois · 2023-11-06T06:47:09Z

pkg/cidata/cidata.go

+	logrus.Debugf("cannot get host timezone from /etc/localtime: %s", err)
+
+	logrus.Warnf("cannot get host timezone")


We should have only one log message, not 2 at different levels. Since we no longer have the err value, we should drop the first one.

AkihiroSuda · 2023-11-17T14:28:39Z

@lobshunter Could you check comments from @jandubois ?

AkihiroSuda · 2023-11-24T03:46:16Z

pkg/cidata/cidata.go

@@ -136,6 +137,7 @@ func GenerateISO9660(instDir, name string, y *limayaml.LimaYAML, udpDNSLocalPort
 		VMType:         *y.VMType,
 		VSockPort:      vsockPort,
 		Plain:          *y.Plain,
+		Timezone:       getHostTimezone(),


Timezone shouldn't be set when *y.Plain is true

lobshunter · 2023-11-30T11:47:12Z

Thanks for reviewing and sorry for late response. I've been too occupied to work on this PR. All review comments make sense but I won't be able to update and test the code in the short future. It should be better to close this PR :-).

AkihiroSuda · 2023-11-30T12:06:43Z

pkg/cidata/cidata.go

@@ -370,6 +372,54 @@ func GenerateISO9660(instDir, name string, y *limayaml.LimaYAML, udpDNSLocalPort
 	return iso9660util.Write(filepath.Join(instDir, filenames.CIDataISO), "cidata", layout)
 }

+// getHostTimezone tries to get host timezone from env or specific files
+func getHostTimezone() string {


Probably we could just use https://pkg.go.dev/time#Time.Zone

(Tried but it didn't work; time.Now().Zone() returns "JST" but it is missing in /usr/share/zoneinfo/)

lobshunter force-pushed the propagate-timezone branch 3 times, most recently from 1488564 to ca37cf0 Compare October 21, 2023 03:17

jandubois requested changes Oct 21, 2023

View reviewed changes

lobshunter marked this pull request as draft October 21, 2023 04:14

lobshunter force-pushed the propagate-timezone branch from fe1705f to b5dc0bc Compare October 29, 2023 09:26

lobshunter commented Oct 29, 2023

View reviewed changes

feat: propagate host timezone to guest

d86ad17

Signed-off-by: pengyu <lobshunter86@gmail.com>

lobshunter force-pushed the propagate-timezone branch from b5dc0bc to d86ad17 Compare November 5, 2023 06:35

lobshunter marked this pull request as ready for review November 5, 2023 13:27

lobshunter requested a review from jandubois November 5, 2023 13:27

jandubois requested changes Nov 6, 2023

View reviewed changes

AkihiroSuda added this to the v0.19.0 milestone Nov 17, 2023

AkihiroSuda reviewed Nov 24, 2023

View reviewed changes

AkihiroSuda modified the milestones: v0.19.0, v0.20.0 Nov 30, 2023

lobshunter closed this Nov 30, 2023

AkihiroSuda removed this from the v0.20.0 milestone Nov 30, 2023

AkihiroSuda reviewed Nov 30, 2023

View reviewed changes

jandubois mentioned this pull request Dec 22, 2023

Set guest timezone from host setting #2097

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: propagate host timezone to guest #1931

feat: propagate host timezone to guest #1931

lobshunter commented Oct 20, 2023

balajiv113 commented Oct 20, 2023

jandubois commented Oct 20, 2023

lobshunter commented Oct 21, 2023

jandubois left a comment

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

lobshunter commented Oct 21, 2023

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

lobshunter Oct 29, 2023

jandubois Nov 6, 2023

jandubois commented Oct 31, 2023

jandubois left a comment

jandubois Nov 6, 2023 •

edited

Loading

jandubois Nov 6, 2023

jandubois Nov 6, 2023 •

edited

Loading

jandubois Nov 6, 2023

AkihiroSuda commented Nov 17, 2023

AkihiroSuda Nov 24, 2023

lobshunter commented Nov 30, 2023

AkihiroSuda Nov 30, 2023

AkihiroSuda Nov 30, 2023

		logrus.Debugf("cannot get host timezone from /etc/localtime: %s", err)

		logrus.Warnf("cannot get host timezone")

feat: propagate host timezone to guest #1931

feat: propagate host timezone to guest #1931

Conversation

lobshunter commented Oct 20, 2023

balajiv113 commented Oct 20, 2023

jandubois commented Oct 20, 2023

lobshunter commented Oct 21, 2023

jandubois left a comment

Choose a reason for hiding this comment

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

lobshunter commented Oct 21, 2023

jandubois commented Oct 21, 2023

jandubois commented Oct 21, 2023

lobshunter Oct 29, 2023

Choose a reason for hiding this comment

jandubois Nov 6, 2023

Choose a reason for hiding this comment

jandubois commented Oct 31, 2023

jandubois left a comment

Choose a reason for hiding this comment

jandubois Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

jandubois Nov 6, 2023

Choose a reason for hiding this comment

jandubois Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

jandubois Nov 6, 2023

Choose a reason for hiding this comment

AkihiroSuda commented Nov 17, 2023

AkihiroSuda Nov 24, 2023

Choose a reason for hiding this comment

lobshunter commented Nov 30, 2023

AkihiroSuda Nov 30, 2023

Choose a reason for hiding this comment

AkihiroSuda Nov 30, 2023

Choose a reason for hiding this comment

jandubois Nov 6, 2023 •

edited

Loading

jandubois Nov 6, 2023 •

edited

Loading