-
Notifications
You must be signed in to change notification settings - Fork 207
fix: ambiguity about time #478
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.
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
base: master
Are you sure you want to change the base?
Conversation
If the user defines the time zone of the container and uses cron at the same time, it will produce wrong results: cron.ParseStandard() defaults to using the local time zone to interpret cron expressions, but the time passed in Next is UTC, which will cause cron to change the original time zone to the time zone corresponding to the Next parameter, resulting in misunderstanding.
|
Hi @akkuman ; thank you for the PR. If I understood you correctly, the issue is that the cron assumes UTC (hence all of the For example, if you set it in cron to run every night at midnight, and the host/container is in timezone US Eastern Time, then it is not clear if it will run at 19:00 Eastern Time (midnight UTC) or 24:00 Eastern Time (local midnight, 05:00 UTC). Is that it? According to your examples, the current code will run at 0300 UTC, and your change will run it at 0300 local time. If so, your proposed solution is to make everything local. I am not sure that is the approach to take. In order to avoid confusion - I am currently involved in a project whose nightly releases run at midnight on GitHub Action runners, and there was a lot of confusion as to whose "midnight" that is - I think everything should be UTC. Why would we not instead parse the cron as relevant to UTC? We certainly can clarify the docs so that it says, "it all is UTC", or perhaps we can have an option to configure it to pick UTC vs local time? But I think forcing everything to local time increase confusion. |
|
I think the problem isn't there. The problem lies in the fact that the code uses local time when parsing the cron expression, but UTC is used during subsequent calculations. These two times are inconsistent. Either both should be unified to UTC, or both should be unified to Local time. cron.ParseStandard() defaults to using the local time zone to interpret cron expressions |
|
Then this change makes it all local? I think it would be cleaner if all UTC. |
|
Let me explain my use case. version: '3'
services:
mysql-backup:
image: databack/mysql-backup:1.2.2
command: dump
restart: always
user: "0"
environment:
DB_SERVER: test.com
DB_PORT: 3306
DB_USER: root
DB_PASS: 'xxx'
DB_DUMP_INCLUDE: testdb
DB_DUMP_CRON: '0 3 * * *'
#DB_DUMP_ONCE: true
DB_DUMP_TARGET: '/data'
#DB_DUMP_SAFECHARS: true
NICE: true
DB_DEBUG: true
DB_DUMP_RETENTION: 3c
volumes:
- '/etc/localtime:/etc/localtime:ro'
- '/etc/timezone:/etc/timezone:ro'
- '/usr/share/zoneinfo:/usr/share/zoneinfo:ro'
- './data:/data'Here, I've mounted the host machine's timezone, so I subconsciously assumed that DB_DUMP_CRON would be based on my host machine's timezone, just like crontab. However, in reality, regardless of whether I've mounted a timezone, DB_DUMP_CRON is always calculated based on timezone zero, but the final backup file is named according to the host machine's timezone, which is a bit confusing. Therefore, I found another alternative as follows: Earlier I mentioned that "this will cause cron to change the original timezone to the timezone corresponding to the Next parameter". Using
|
This is a good catch. Is this the heart of the issue? cron evaluation happens based on UTC, while file naming is based on whatever the timezone of the environment in which the backup process runs (machine, container, etc.)?
This works? I agree that in principle this should be consistent. I don't think it is terrible if it is not consistent - plenty of systems use UTC for one thing and local TZ for another - but it is confusing, especially if not cleanly documented. Let's try to figure out what makes sense. We have a few options:
I like UTC overall, especially for infrastructure things, and especially for file naming. But already we are not doing that, as you pointed out. I have no issues changing it, though. It would be backwards compatible anyways, since the filenames include either Nevertheless, I recognize that many people like their software to respect local timezones. If I were doing this today from scratch, I think I would reverse what we have today: cron based on local timezone, filenames 100% UTC. Long-term artifacts benefit much more from consistent timestamps. It makes it easier for people to compare them, look across TZs, look in the past, etc. If I understood your PR, you want to make cron be local timezone instead of UTC, i.e. half of the reverse I have in mind? |
If the user defines the time zone of the container and uses cron at the same time, it will produce wrong results: cron.ParseStandard() defaults to using the local time zone to interpret cron expressions, but the time passed in Next is UTC, which will cause cron to change the original time zone to the time zone corresponding to the Next parameter, resulting in misunderstanding.
example:
current time: 2025-10-31T10:36:00+08:00
current timezone: +08:00
output: 22m55.403517847s
output: 16h22m28.330960802s
Clearly, the second one is more intuitive.