Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow searching $TZDIR for timezone data #21063

Closed
wants to merge 1 commit into from
Closed

Allow searching $TZDIR for timezone data #21063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/source/markdown/options/tz.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
####> are applicable to all of those.
#### **--tz**=*timezone*

Set timezone in container. This flag takes area-based timezones, GMT time, as well as `local`, which sets the timezone in the container to match the host machine. See `/usr/share/zoneinfo/` for valid timezones.
Set timezone in container. This flag takes area-based timezones, GMT time, as well as `local`, which sets the timezone in the container to match the host machine. See `/usr/share/zoneinfo/` for valid timezones. Podman honors the `$TZDIR` environment variable to modify the `/usr/share/zoneinfo` search path for valid timezones.
Remote connections use local containers.conf for defaults
8 changes: 8 additions & 0 deletions libpod/container_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1708,6 +1708,14 @@ func (c *Container) mountStorage() (_ string, deferredErr error) {
tz := c.Timezone()
if tz != "" {
timezonePath := filepath.Join("/usr/share/zoneinfo", tz)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// Allow using TZDIR per:
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if tz == "local" {
timezonePath, err = filepath.EvalSymlinks("/etc/localtime")
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions test/system/030-run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

}

# run with --runtime should preserve the named runtime
@test "podman run : full path to --runtime is preserved" {
skip_if_remote "podman-remote does not support --runtime option"
Expand Down