libpod: fix timezone handling #18756
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
LGTM |
|
I'll be AFK for a few hours, will not be able to check back in until ~2pm MDT. |
|
From my long ago memories of this, java was particularly sensitive to the TZ setting. The TZ would be fine for other apps, but java would not concur. Can you add a test that includes java, similar to the original problem report in https://bugzilla.redhat.com/show_bug.cgi?id=1984251? |
The test already checks what java would check. It must be a valid symlink to the correct time zone. Can we add a java test? Sure but the problem is not what java does, you can reproduce this with any application that tries to get the timezone identifier, e.g. in a systemd container use Adding an image with java is around 500 MB in size which is IMO a big no we cannot ever test actual java in CI, the image size would be unacceptable big and cause way to much flakes considering that would also only use it for one test. |
| run_podman run --rm $SYSTEMD_IMAGE ls /usr/share/zoneinfo | ||
|
|
||
| run_podman run --rm --tz Europe/Berlin $SYSTEMD_IMAGE readlink /etc/localtime | ||
| assert "$output" == "../usr/share/zoneinfo/Europe/Berlin" "localtime is linked correctly" |
There was a problem hiding this comment.
I think you're going to need to add run_podman rmi $SYSTEMD_IMAGE here. (And maybe in teardown, but I'm less worried about that). I tried, when creating the systemd image, to fix tests so they would accept it in the image list, but obviously I didn't catch everything.
There was a problem hiding this comment.
Let's see if #18762 fixes these problems.
There was a problem hiding this comment.
Oh my, and I was going crazy over here because they pass when I ran them individually. I applied Tom nits and cherry-picked your commit to fix the tests.
The current way of bind mounting the host timezone file has problems. Because /etc/localtime in the image may exist and is a symlink under /usr/share/zoneinfo it will overwrite the targetfile. That confuses timezone parses especially java where this approach does not work at all. So we end up with an link which does not reflect the actual truth. The better way is to just change the symlink in the image like it is done on the host. However because not all images ship tzdata we cannot rely on that either. So now we do both, when tzdata is installed then use the symlink and if not we keep the current way of copying the host timezone file in the container to /etc/localtime. Also note that we need to rebuild the systemd image to include tzdata in order to test this as our images do not contain the tzdata by default. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2149876 Signed-off-by: Paul Holzinger <[email protected]>
We _usually_ have only one image in store, $IMAGE, but it's
perfectly fine to also have $SYSTEMD_IMAGE also. Fix a few
tests so they can handle that condition.
And, cleanup:
- remove a no-longer-useful test ("podman load NEWNAME",
functionality that was removed 2+ years ago in containers#8877)
- reorder some tests in the image-mount test, to make
them safer and easier to understand
- use no-such-image, not no-such-container, in image-mount test.
Computer don't care, but this human felt confused for a sec.
Signed-off-by: Ed Santiago <[email protected]>
|
My recollection is that Java demands a symlink, not a hardlink, so this PR is better/safer for Java. Regardless, symlinks are the new way (that's what I see on Fedora, Gentoo, Debian) so this LGTM. |
|
yes it must be in symlink in order for any tool to get the zimezone identifier, see localtime(5).
I should have mentioned that in the commit message. Java is only special because it just outright ignores incorrect localtime setup and uses UTC in this case instead of just reading the file content and still showing the right local time with an incorrect timezone like the other applications (i.e. systemd) do. And I don't think we can blame java for that. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
The current way of bind mounting the host timezone file has problems. Because /etc/localtime in the image may exist and is a symlink under /usr/share/zoneinfo it will overwrite the targetfile. That confuses timezone parses especially java where this approach does not work at all. So we end up with an link which does not reflect the actual truth.
The better way is to just change the symlink in the image like it is done on the host. However because not all images ship tzdata we cannot rely on that either. So now we do both, when tzdata is installed then use the symlink and if not we keep the current way of copying the host timezone file in the container to /etc/localtime.
Also note that we need to rebuild the systemd image to include tzdata in order to test this as our images do not contain the tzdata by default.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2149876
Does this PR introduce a user-facing change?