Allow searching $TZDIR for timezone data #21063

saschagrunert · 2023-12-20T08:54:44Z

glibc supports the environment variable as additional search path, means we allow to override the /usr/share/zoneinfo path by setting the variable.

Analogous change to containers/common#1772

Does this PR introduce a user-facing change?

Added `TZDIR` environment variable support to modify the global timezone search path of `/usr/share/zoneinfo`.

openshift-ci · 2023-12-20T08:54:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

glibc supports the environment variable as additional search path, means we allow to override the `/usr/share/zoneinfo` path by setting the variable. Analogous change to containers/common#1772 Signed-off-by: Sascha Grunert <[email protected]>

rhatdan · 2023-12-20T11:23:29Z

libpod/container_internal.go

@@ -1708,6 +1708,14 @@ func (c *Container) mountStorage() (_ string, deferredErr error) {
 	tz := c.Timezone()
 	if tz != "" {
 		timezonePath := filepath.Join("/usr/share/zoneinfo", tz)
+


Should all of this come from the similar code in containers/common?

I would say yes, especially since we have a clone of that code in cri-o/cri-o#7589. I stated that in cri-o/cri-o#7550 (comment). For CRI-O we would like to still maintain our own config field but reuse the helper functions per cri-o/cri-o#7550 (comment).

edsantiago · 2023-12-20T13:16:53Z

libpod/container_internal.go

+		// https://sourceware.org/git/?p=glibc.git;a=blob;f=time/tzfile.c;h=8a923d0cccc927a106dc3e3c641be310893bab4e;hb=HEAD#l149
+		tzdir := os.Getenv("TZDIR")
+		if tzdir != "" {
+			timezonePath = filepath.Join(tzdir, tz)


Wait.... this seems wrong to me, it's completely different from the append-path approach in your c-c PR. And I don't see any other basic utils if TZDIR is missing. I'm still investigating, but for now, let's block this

/hold

(Oh, also, what Dan said. We don't want two inconsistent implementations. But that might be a bigger problem)

Yeah so the question to me would be: Do we want to mimic the glibc behavior and use $TZDIR as override or do we want to place it as a higher priority path? I'm leaning towards the override.

The glibc code you linked to implements it as an override; the wording in tzset(3) seems to confirm:

TZDIR If this variable is set its value takes precedence over the system configured timezone database directory path.

Okay, so it's c-c that is wrong. It squicks me out to have podman actually error out, instead of just ignoring the error, but there's precedent (podman run --tz=Foo/Bar also fails), so okay.

Please fix it so it actually works though (my Abc/Def test above).

Thanks for your PR and for providing good reference sources.

Alright, following-up in c/common: containers/common#1774

edsantiago · 2023-12-20T13:18:14Z

test/system/030-run.bats

@@ -513,6 +513,11 @@ json-file | f
    assert "$output" == "../usr/share/zoneinfo/Europe/Berlin" "localtime is linked correctly"
 }

+@test "podman run --tz with zoneinfo and custom TZDIR env" {
+    # Setting the timezone should fail because it does not exist
+    TZDIR="$PODMAN_TMPDIR/zoneinfo" run_podman 127 run --rm --tz Europe/Berlin "$SYSTEMD_IMAGE"


Also, please don't just do error-status checks. Error messages must be confirmed:

assert "$output" =~ "Error: setting timezone for container .*: stat $PODMAN_TMPDIR/zoneinfo/Europe/Berlin: no such file or directory"

Also,

skip_if_remote "TZDIR not propagated remotely"

edsantiago · 2023-12-20T13:32:07Z

Also, it doesn't seem to be working:

$ mkdir -p /tmp/zoneinfo/Abc
$ cp /usr/share/zoneinfo/Pacific/Chatham /tmp/zoneinfo/Abc/Def

$ TZDIR=/tmp/zoneinfo TZ=Abc/Def date
Thu Dec 21 03:15:13 AM +1345 2023       <--- OK, it works on f39

$ TZDIR=/tmp/zoneinfo bin/podman run --rm --tz=Abc/Def quay.io/libpod/testimage:20221018 date
Error: running container create option: finding timezone: unknown time zone Abc/Def
^^^^^ but not with podman

saschagrunert · 2023-12-20T14:04:56Z

@sohankunkerkar @rhatdan @vrothberg @edsantiago @Luap99 considering cri-o/cri-o#7589, do we want to move the timezone related bits into c/common helper functions and reuse them in CRI-O and Podman?

sohankunkerkar · 2023-12-20T14:35:58Z

@sohankunkerkar @rhatdan @vrothberg @edsantiago @Luap99 considering cri-o/cri-o#7589, do we want to move the timezone related bits into c/common helper functions and reuse them in CRI-O and Podman?

Yeah, that's a good idea. We can reuse the setContainerTimeZone and copyTimezoneFile functions by placing them in c/common.

saschagrunert · 2023-12-21T08:08:30Z

Awesome, let me close this one then and vendor c/common code once it is there.

The TZDIR environment variable override the lookup paths and does not append them. This patch fixes that behavior and is a follow-up on: containers#1772 Ref: containers/podman#21063 (comment) Signed-off-by: Sascha Grunert <[email protected]>

openshift-ci bot added the release-note label Dec 20, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2023

saschagrunert force-pushed the tzdir branch from 42babd7 to addc398 Compare December 20, 2023 08:55

saschagrunert mentioned this pull request Dec 20, 2023

*: add support for specifying timezone for pod/container cri-o/cri-o#7589

Merged

saschagrunert changed the title ~~Allow searching $TZDIR for timezone data~~ WIP: Allow searching $TZDIR for timezone data Dec 20, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2023

saschagrunert force-pushed the tzdir branch from addc398 to 4d3b548 Compare December 20, 2023 09:49

saschagrunert changed the title ~~WIP: Allow searching $TZDIR for timezone data~~ Allow searching $TZDIR for timezone data Dec 20, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2023

Allow searching $TZDIR for timezone data

e916555

glibc supports the environment variable as additional search path, means we allow to override the `/usr/share/zoneinfo` path by setting the variable. Analogous change to containers/common#1772 Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert force-pushed the tzdir branch from 4d3b548 to e916555 Compare December 20, 2023 10:55

rhatdan reviewed Dec 20, 2023

View reviewed changes

edsantiago reviewed Dec 20, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2023

edsantiago reviewed Dec 20, 2023

View reviewed changes

saschagrunert closed this Dec 21, 2023

saschagrunert deleted the tzdir branch December 21, 2023 08:08

saschagrunert mentioned this pull request Dec 21, 2023

Use TZDIR as override lookup path containers/common#1774

Merged

sohankunkerkar mentioned this pull request Dec 21, 2023

pkg/timezone: add helper function to configure timezone for pod/containers containers/common#1775

Merged

TomSweeneyRedHat mentioned this pull request Jan 22, 2024

[0.57] backport: #1775 containers/common#1810

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Mar 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow searching $TZDIR for timezone data #21063

Allow searching $TZDIR for timezone data #21063

saschagrunert commented Dec 20, 2023 •

edited

Loading

openshift-ci bot commented Dec 20, 2023

rhatdan Dec 20, 2023

saschagrunert Dec 20, 2023

edsantiago Dec 20, 2023

saschagrunert Dec 20, 2023

edsantiago Dec 20, 2023

saschagrunert Dec 21, 2023

edsantiago Dec 20, 2023

edsantiago commented Dec 20, 2023

saschagrunert commented Dec 20, 2023

sohankunkerkar commented Dec 20, 2023

saschagrunert commented Dec 21, 2023

Allow searching $TZDIR for timezone data #21063

Allow searching $TZDIR for timezone data #21063

Conversation

saschagrunert commented Dec 20, 2023 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Dec 20, 2023

rhatdan Dec 20, 2023

Choose a reason for hiding this comment

saschagrunert Dec 20, 2023

Choose a reason for hiding this comment

edsantiago Dec 20, 2023

Choose a reason for hiding this comment

saschagrunert Dec 20, 2023

Choose a reason for hiding this comment

edsantiago Dec 20, 2023

Choose a reason for hiding this comment

saschagrunert Dec 21, 2023

Choose a reason for hiding this comment

edsantiago Dec 20, 2023

Choose a reason for hiding this comment

edsantiago commented Dec 20, 2023

saschagrunert commented Dec 20, 2023

sohankunkerkar commented Dec 20, 2023

saschagrunert commented Dec 21, 2023

saschagrunert commented Dec 20, 2023 •

edited

Loading