Conversation
|
I'm a bit torn on this one, mostly because of not wanting to change things that currently work as intended. I would definitely think this is something to include in a v3, yes. One big question in here would be: considering this would be merged and users that currently mount their TZ data as it's done right now updated without changing their config, would their setups still continue to work? |
|
I just tested it by using my custom image (only adding environment:
- TZ=${TZ}
image: offen/docker-volume-backup:v2.47.2-tzdata-test
volumes:
# - /etc/timezone:/etc/timezone:ro
# - /etc/localtime:/etc/localtime:ro
# - /usr/share/zoneinfo:/usr/share/zoneinfo:roI also tried the custom image with environment:
# - TZ=${TZ}
image: offen/docker-volume-backup:v2.47.2-tzdata-test
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roI then tried using the custom environment:
- TZ=${TZ}
image: offen/docker-volume-backup:v2.47.2-tzdata-test
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roAll cases are working as expected, producing the expected time with the Perhaps it is better saved for v3, but I think this functionality could be enabled without issue immediately. |
|
From #28 (comment) /usr/share/zoneinfo:/usr/share/zoneinfo:roPage it's missing from: set container timezone services:
backup:
image: offen/docker-volume-backup:v2
volumes:
- data:/backup/my-app-backup:ro
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
volumes:
data:I can edit and open a PR to fix |
That'd be grand 🎩 |
|
Continuing our conversation here...
No worries. I can do some more testing and report back. I was doing some research on ChatGPT and it agrees that it's a low-risk change. I think the testing I did above will cover users that upgrade to a new image that includes |
|
Id like to throw my hat in the ring for this addition to the codebase. The current way of binding the OS host timezone did work for me as intended, but it feels a bit awkward as I am depending on an upstream config of my host system. Which could change. Being able to set the time zone explicitly (and also in only 1 line) would feel like a great ease of life feature. one question that comes to mind: Also, if this is added, it would be great if the time zone would be logged on startup (“Timezone set to “Europe/Berlin via environment variable.”) I also feel that the very slim increase in image size is worth it. |
@le-lenn great question. In short, if both are set, the timezone set by the env variable TZ (used by the
For configurations 2 and 3, I am using an image created with the following Dockerfile: FROM offen/docker-volume-backup:v2.47.2
RUN apk add tzdata curl jq --no-cacheConfiguration 1: environment:
# - TZ=America/Los_Angeles
image: offen/docker-volume-backup:v2.47.2
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roConfiguration 2: environment:
# - TZ=America/Los_Angeles
image: offen/docker-volume-backup:v2.47.2-tzdata-test
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roConfiguration 3: environment:
- TZ=America/Los_Angeles
image: offen/docker-volume-backup:v2.47.2-tzdata-test
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roConfiguration 1 represents users today, and configuration 2 represents a new image with Configuration 3 shows that when I would argue that there is no need to print the current timezone upon container start, it is UTC by default. If a user modifies it from the default, they should know what they are updating it to. Worst case users can run the I like your point about the dependency created by the upstream host. The Let me know if there's anything I can clarify. |
|
I guess it makes sense to do this, yes. A few more remarks from my end:
|
|
My line of thinking with printing the configured time zone on startup was to have a way to catch accidental misconfigurations and warn the user about it. |
|
@le-lenn the time zone defaults (without warning) to UTC if you do not provide a valid tz database time zone @m90 we should be able to add a script that does something similar to the following: A possible script could look something like this: #!/bin/sh
# List of deprecated bind-mount targets
DEPRECATED_MOUNTS="/etc/timezone /etc/localtime /usr/share/zoneinfo"
FOUND_DEPRECATED=false
for mnt in $DEPRECATED_MOUNTS; do
# Check if the path is explicitly listed as a mount point
if grep -q " $mnt " /proc/self/mountinfo; then
echo "WARNING: Bind-mounting '$mnt' is DEPRECATED."
FOUND_DEPRECATED=true
fi
done
if [ "$FOUND_DEPRECATED" = true ]; then
echo "-------------------------------------------------------------------"
echo "Use the 'TZ' environment variable to set the time zone"
echo "Please see the documentation for more details"
echo "-------------------------------------------------------------------"
fi
# Continue to main application
exec /usr/bin/backup -foreground "$@"This script would become the new entrypoint in the Dockerfile, and it could then call your current entrypoint. I will test this out. |
|
The logic in the script looks good to me, but I'd prefer to have this written in Go just like everything else. Would you be capable of doing this? Happy to help out if advice is needed. |
|
I am not an expert in Go, I had to get AI to help me with this one. Feel free to suggest changes or modifications you think would be helpful. I think this does a good job of capturing what we need to do, which is to detect any of the bind mounts, print a deprecation message, then hand execution off to the main binary. We could add a direct link to the page in the documentation, to the deprecation warning. Could this possibly be included in the existing main.go file, so that there is no need to modify the container's entrypoint? That would simplify the script, and the Dockerfile would remain the same after this functionality is added. package main
import (
"bufio"
"fmt"
"os"
"strings"
"syscall"
)
func main() {
deprecatedMounts := []string{"/etc/timezone", "/etc/localtime", "/usr/share/zoneinfo"}
foundDeprecated := false
file, err := os.Open("/proc/self/mountinfo")
if err == nil {
defer file.Close()
scanner := bufio.NewScanner(file)
for scanner.Scan() {
line := scanner.Text()
for _, mnt := range deprecatedMounts {
if strings.Contains(line, " "+mnt+" ") {
fmt.Printf("WARNING: Bind-mounting '%s' is DEPRECATED.\n", mnt)
foundDeprecated = true
}
}
}
}
if foundDeprecated {
fmt.Println("%%%%% DEPRECATION WARNING %%%%%")
fmt.Println("Use the 'TZ' environment variable to set the container's time zone")
fmt.Println("Please see the documentation for more details")
fmt.Println("%%%%% END DEPRECATION WARNING %%%%%")
}
binary := "/usr/bin/backup"
args := []string{"backup", "-foreground"}
// syscall.Exec replaces this Go process with the backup process (PID 1)
err = syscall.Exec(binary, args, os.Environ())
if err != nil {
fmt.Printf("Failed to execute %s: %v\n", binary, err)
os.Exit(1)
}
} |
|
Looks good. Right now, deprecations warnings are printed from within the docker-volume-backup/cmd/backup/config.go Lines 232 to 236 in 842ad97 Maybe this check can go here or we introduce a plain helper function that is called right afterwards: |
|
My vote is for where existing deprecation warnings are already being handled/printed |
|
@mmichalak-swe Do you need any help in getting that deprecation check/notice in? In case you're just busy, that's perfectly fine, just wanted to make sure you're not awaiting any kind of feedback from my end? |
|
@m90 sorry for the delayed response, I have been busy, but will be able to complete this in the next few days. |
|
No worries, please proceed as it fits your schedule and just ping me when you need help or you're done. |
|
@m90 please review my latest changes when you get a chance and let me know what you think. I built the image locally and tested with the following docker-compose file: services:
volume-backup:
cap_drop:
- ALL
container_name: volume-backup
environment:
- BACKUP_CRON_EXPRESSION=0 0 5 31 2 ?
- TZ=America/New_York
hostname: volume-backup
image: <local test image>
read_only: true
restart: unless-stopped
security_opt:
- no-new-privileges:true
tmpfs:
- /tmp
- /var/lock
user: "911:911"
volumes:
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
- /usr/share/zoneinfo:/usr/share/zoneinfo:roFor reference, my current local timezone is When I comment out the TZ variable, the time zone in the container is still set (as expected) to my host's time zone via the bind mounts. The logs change to the following: |
m90
left a comment
m90
left a comment
There was a problem hiding this comment.
This looks good, thank you. I left a couple of comments inline, but nothing major. Let me know what you think.
| > data: | ||
| > ``` | ||
|
|
||
| ## Recommended approach (using `TZ`) |
There was a problem hiding this comment.
I would guess we can move the recommended approach up? Ideally, users will only read this far and move on without even knowing the old approach still exists?
| @@ -1,2 +1,3 @@ | |||
| _site | |||
| .jekyll-cache | |||
| .DS_Store | |||
There was a problem hiding this comment.
Can we remove this? I would argue OS-specific files like this should go in a global gitignore on individual machines.
| } | ||
| } | ||
|
|
||
| func (c *command) logStartupWarnings(config *Config) { |
There was a problem hiding this comment.
Is this method really needed? I'm a bit confused this is used in here, but then it's not used when printing the config. Maybe it'd be better to just loop over the warnings in place above, even if it's a slight repetition?
| func (c *Config) timezoneDeprecationWarnings() []string { | ||
| mounts, err := mountedPaths("/proc/self/mountinfo") | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
I would think this means something went really wrong and we might not even want to continue at all? I.e. I think the method should return []string, error
| return mounts, nil | ||
| } | ||
|
|
||
| func (c *Config) timezoneDeprecationWarnings() []string { |
There was a problem hiding this comment.
Maybe this can have a more descriptive name? Something like checkTimezoneConfiguration. What do you think?
3 participants
Changes
tzdatapackage to Docker imageReason
Installing
tzdataprovides the system timezone database and allows the container timezone to be configured via theTZenvironment variable.