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.

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

Use server local time for everything, not UTC #69

Closed
wants to merge 6 commits into from

Conversation

thieman
Copy link
Owner

@thieman thieman commented May 10, 2014

Closes #46

@rclough any objections?

@Creator11 can you give this another go? I had no problems creating jobs with this branch, are you able to do so with current master? I think maybe you've got a separate config issue.

@thieman thieman changed the title Tnt fix cron format Fix cron format May 10, 2014
@thieman thieman changed the title Fix cron format Use server local time for everything, not UTC May 10, 2014
@thieman
Copy link
Owner Author

thieman commented May 10, 2014

I'd imagine this will probably introduce weird behavior around daylight savings time changes on servers not running UTC. Worth it?

@rclough
Copy link
Collaborator

rclough commented May 12, 2014

Most things I've used have been server time.

Sending UTC to front end makes more sense though because it allows local time representation if you or the user prefers that.

In which way will this solve the "Time until next run" issue? ie if I have a cron for "18 00 * * *" and my server is West coast, I'm east coast, will it tell me "Next run at 9:00pm" (my local eastern time) or will it show 6 (server time)? If we're doing crons on server time it might be useful to see the current server time in the front end.

Whatever gets chosen, as long as its documented should be fine!

@rclough
Copy link
Collaborator

rclough commented May 12, 2014

Could be weird on DST I suppose, like when a server loses an hour and jobs are scheduled during that hour.....

@surbas
Copy link
Contributor

surbas commented May 13, 2014

I deal with Timezones a lot in the stuff i deal with at work. In my experience you always store time in utc and store the time zone that you want that utc interpreted by separately. The time zone should almost always be based of a location as opposed to the actual time zone. i.e I would store "America/New York" as opposed to "Eastern" because Eastern can be ambiguous with daylight savings. This is how the Olson database is setup. pytz is the python implementation of it.

I think in the UI you should be able to specify the Timezone (based off pytz name) you want the cron to be interpreted from. In the database you store the run time based of the cron string and that timezone, but store the time as UTC. We don't rely on server time at all except maybe as the default in the time zone drop down.

At least in my case I always want to run my processes at starting at 1 PM new york time (whether that is UTC-5 or UTC-4) It would be nice not to have change that twice a year, like i do now.

@thieman
Copy link
Owner Author

thieman commented May 13, 2014

Alright, I think I've confused myself a bit on this, so I'm going to start from scratch. I observe the following:

  1. The important element here is the cron string. This is what the user actually inputs. At no point does the user enter a time.
  2. Given 1 and Dagobah's intent as a cron replacement, Dagobah's handling of this cron string should mirror what you would expect from cron.
  3. Given 2, Dagobah should only care about the cron string and the system time (before timezone handling) on the server. Everything else is purely cosmetic.

An important implication of this is that scheduled jobs should fail to run during a DST lost hour if you are running a server that uses a daylight savings-sensitive timezone. In my understanding, this is the behavior you would expect from cron itself. Additionally, I don't like the idea of being able to set a cron based on the browser's timezone. I think the server should be the one true source of time.

Given these goals, I think we should do the following:

  1. Use server time, e.g. datetime.datetime.now(), for all operations on the server side.
  2. If easy, send local and UTC times to the UI so that times can be displayed in both server and local time.
  3. If not 2, then just indicate in the UI that everything is in server time.

Thoughts?

@surbas
Copy link
Contributor

surbas commented May 14, 2014

I think that the most flexible solution with not that much pain is to decouple Dagobah from server time zone. On various linux distributions it is possible to tell cron what time zone to use when interpreting a cron string by setting the TZ environmental variable. To be a true cron replacement then Dagobah should also allow a user to override server timezone when setting up jobs. This eliminates edge cases like not having the ability to set your servers time zone, and being forced to do mental arithmetic; or shenanigans like a server admin changing time zones on you in between job runs.

If you agree with the above the an argument can be made to set the timezone as a config, and force all jobs to use that but i really think to offer maximum flexibility that it should be a job specific thing. It fits in with the current plans to make jobs very flexible.

In either case in the schedule method of Job you would do something like, assuming you have a self.timezone or config.timezone (let just call it job_tz)

#Get the utc timezone aware datetime (needed for astimezone to work)
base_utc_datetime = pytz.utc.localize(datetime.utcnow())
base_datetime = base_utc_datetime.astimezone(job_tz)
self.cron_schedule = cron_schedule
self.cron_iter = croniter(cron_schedule, base_datetime)
#get the next run as utc here to minimize code changes else where.
self.next_run = self.cron_iter.get_next(datetime).astimezone(pytz.utc)

@thieman thieman mentioned this pull request Dec 9, 2014
@thieman thieman closed this Jul 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crontab format not correct
3 participants