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

Cannot use local time for cron schedules and UTC for solar schedules #57

Open
DDevine opened this Issue Jun 23, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@DDevine
Contributor

DDevine commented Jun 23, 2017

To make Solar scheduling work I had to set CELERY_TIMEZONE = 'UTC' - however this causes Crontab schedules to be offset by my timezone .

That means a crontab configured trigger in 1 hour's time does not end up triggering for 11 hours (because I am in +10 Australia/Brisbane).

Should django-celery-beat create crontab schedules with CrontabSchedule.tz = settings.TIME_ZONE instead to override CELERY_TIMEZONE?

@DDevine

This comment has been minimized.

Contributor

DDevine commented Jul 3, 2017

This is related to #49

Giving the ability to configure nowfun makes sense.

But in the meantime defaulting to using django.utils.timezone.now for nowfun by adding it to the arguments of django_celery_beat.schedules.ModelEntry.from_entry it will solve the vast majority of use-cases in the short term and it seems to me that migrating to a more flexible solution later should be trivial.

At the moment the existing behaviour is useless for the vast majority of people, so at least having a sane default is a considerable upgrade.

@RabidCicada

This comment has been minimized.

RabidCicada commented Jul 5, 2017

Hey there @DDevine , Thanks for the research. I'll be hacking around to implement your workaround until this gets fixed the right way. Could you tell me exactly where in the from_entry line to put the nowfun arg? I'll be working on it tonight so I may get to it before you respond...but I may also get lost:).

I just got bit by this messing with my crontab tasks when I turned the timezone to the proper timezone instead of UTC

@DDevine

This comment has been minimized.

Contributor

DDevine commented Jul 5, 2017

I've looked into it some more, I've found it a bit tricky to debug and to wrap my head around all the timezone stuff. There's a lot more problems, particularly where Solar is concerned - I'm not convinced it was ever tested before being added to Celery...

So firstly, from_entry was not the right place to put nowfun - I ended up adding it to models.py in CrontabSchedule.

    @property
    def schedule(self):
        nf = lambda: make_aware(now())
        return schedules.crontab(minute=self.minute,
                                 hour=self.hour,
                                 day_of_week=self.day_of_week,
                                 day_of_month=self.day_of_month,
                                 month_of_year=self.month_of_year, nowfun=nf)

This allows Celery to process the crontab in context of your local time - assuming that you have Django TIME_ZONE set to your local time. This is probably not the right way to do it - but I am just hacking at this problem for the moment.

TIME_ZONE = 'Australia/Brisbane' # Django TZ
CELERY_ENABLE_UTC = True # If false, when messages are sent they are kept in local time, if True they are converted to UTC.
CELERY_TIMEZONE = 'Australia/Brisbane'

Pytz is creating a LMT based timezone for Australia/Brisbane (+10:12 LMT) rather than a clean UTC offset (10:00 UTC) or tomezone abbreviation (... AEST). LMT does not cleanly convert to UTC and I found that when converting using pytz localize() it would drop the offset, so when converting you have to make sure you add (or subtract) the UTC offset.

For example, in the celery.schedules.solar.remaining_estimate():

        next = self.maybe_make_aware(next_utc.datetime())
        now = self.maybe_make_aware(self.now())
        next = localize(next, timezone.utc)
        offset = now.utcoffset()
        now = localize(now, self.tz)

        delta = next - now - offset
        return delta

It also seems that when there are Crontab schedules are not checked/executed while there are Solar schedules. Awesome...

Another problem is that I notice the delta goes negative. This issue is dealt with in the crontab class by using ffwd to only to positive deltas.

It also seems that upon restarting celerybeat solar tasks that have already been run are also run again... Maybe this is an issue with last_run_at? I'll have to look into specifics regarding last_run_at to know more.

The whole lot is a mess. I really didn't anticipate this trouble!

@RabidCicada

This comment has been minimized.

RabidCicada commented Jul 6, 2017

Holy Cow....You weren't kidding about tricky.

I dove in deep last night and...bleh.

So (separate bug), Celery is not properly taking django's TIME_ZONE setting like it says it will and it tries to do. It simply doesn't see django's timezone variable for some reason. So I had to explicitly set celey's timezone like you did.

You are right about the LMT thing. That would have driven me crazy had you not mentioned it. I wonder why it does that-->https://stackoverflow.com/questions/11473721/weird-timezone-issue-with-pytz.

I tracked a lot of the handling and just figured I could provide 'app=self.app' to some constructors since a lot of the is_due() stuff is mediated by ModelEntry.

But it looks like you know the problem I ran into next....a lot of the other calculations (remaining_delta, remaining_time etc)don't use app.now() directly and so I end up with offsets that are off. It does look like nowfun is the way to go...but I was looking for a way to not touch the django model itself.

Perhaps I still can go with using app.now() properly....looking into it more later. I spent a good deal of time last night hacking around celery, django-celery-beat, and my app tracing exactly where and how things are called.

@RabidCicada

This comment has been minimized.

RabidCicada commented Jul 6, 2017

If you have any more insights let me know:).

@DDevine

This comment has been minimized.

Contributor

DDevine commented Jul 7, 2017

Yes using app.now() should be the right way to do it - because it abstracts away from Django by selecting nowfun or self.app.now() - and I can't see why it shouldn't work.

I adjusted my debug statements a little and found that crontab was using nowfun, but solar was not... This made me face palm because I realised that I had not told solar to use nowfun. Once I got that fixed in django-celery-beat (quite simple) it all came together, though I had to remove some of that localization/delta code I showed above and return to the default. I assume nowfun() is not using a LMT timezone and thus avoids those issues.

django-celery-beat SolarSchedule

    @property
    def schedule(self):
        nf = lambda: make_aware(now())
        return schedules.solar(self.event, self.latitude, self.longitude, nowfun=nf)

AND!!!
django-celery-beat CrontabSchedule

    @property
    def schedule(self):
        nf = lambda: make_aware(now())
        return schedules.crontab(minute=self.minute,
                                 hour=self.hour,
                                 day_of_week=self.day_of_week,
                                 day_of_month=self.day_of_month,
                                 month_of_year=self.month_of_year,
                                 nowfun=nf)

The question is - what is the real issue here? Is there more than one? Do we need to detect funky LMT datetimes and handle them over in celery? Does Celery try to use Django's TIME_ZONE? I was under the impression that Celery did not try to directly use Django's TIME_ZONE variable - and from what I can tell CELERY_TIMEZONE from within Django does seem to work. Were you getting confused between TIME_ZONE and CELERY_TIMEZONE?

@RabidCicada

This comment has been minimized.

RabidCicada commented Jul 7, 2017

From Celery's Periodic Tasks Notes

Django Users
Celery recommends and is compatible with the new USE_TZ setting introduced in Django 1.4.

For Django users the time zone specified in the TIME_ZONE setting will be used, or you can specify a custom time zone for Celery alone by using the timezone setting.

But it lies:).

Btw thanks for this investigation and the work to fix.

@zh-h

This comment has been minimized.

zh-h commented Jul 15, 2017

@RabidCicada Because that is old style project djcelery. Manually setting CELERY_TIMEZONE resolved my problem too. #5 (comment)

@JoshYuJump

This comment has been minimized.

JoshYuJump commented Aug 20, 2017

In the celery/app/base.py

    def now(self):
        """Return the current time and date as a datetime."""
        from datetime import datetime
        # return datetime.utcnow().replace(tzinfo=self.timezone)
        print(datetime.now())
        return datetime.now()
@DDevine

This comment has been minimized.

Contributor

DDevine commented Aug 21, 2017

@JoshYuJump ...you put this ticket in the wrong repo? I thought I may have left that debug code in there but looking at my pull requests and celery master that code isn't there.

@DDevine

This comment has been minimized.

Contributor

DDevine commented Sep 7, 2017

I was forced to look back into this - I have no idea why my other code was working in my development machine but is not working in new installs!

I traced it down to what you alluded to @JoshYuJump - your fix is not the most correct, but it is on the right track.

celery.app.base.now() was returning a PyTZ timezone, which Celery should use - however it does not make sense in the Django Celery Beat context. I think that's what the PyTZ Introduction section is alluding to (http://pytz.sourceforge.net/#introduction) however whenever I look at the PyTZ documentation my eyes instantly glaze over.

Anyway - for Django Celery Beat we need to localize self.app.now() in schedulers.ModelEntry._default_now().

I'll look testing my fix further tomorrow and submit a PR.

Without this #68 doesn't work.

@2ps

This comment has been minimized.

2ps commented Mar 11, 2018

This was really goofy, but what I did was override the celery base app's now to return the proper time, and it started working. There is a bug in celery v4.1.0 where Celery.now does not calculate the current time properly for non-UTC timezones.

class MyCelery(Celery)
    def now(self):
        """Return the current time and date as a datetime."""
        from datetime import datetime
        return datetime.now(self.timezone)

app = MyCelery()

And this fixed the issue for us. n.b., this issue may be fixed in the master branch, but it may not be the wisest decision to rely upon version 4.X getting timezones right anytime soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment