Account for change in UTC offset when performing next schedule calculations #30083

timkpaine · 2023-03-13T23:02:17Z

Closes: #7999

When performing a next value calculation using a cron schedule with a non-fixed interval (e.g. * 1,2,3 * * *), changes in timezone are not properly accounted for. The functions _get_next and _get_prev do not account for the delta between the current and scheduled values' respective timezones. This PR fixes that, and accordingly fixes the closed but unfixed #7999 issue.

Note that this may want to be considered a breaking change, even though it is a fix in the behavior.

joshzana · 2023-03-14T04:32:44Z

@timkpaine I just logged #30088 and happened to find this PR right after logging. Does your fix resolve my issue as well?

timkpaine · 2023-03-14T20:49:56Z

@joshzana technically my PR makes the dag run an hour earlier during e.g. the recent DST transition, since the current code is 1 hour delayed. But I ran your repro and it passed without error, so I'm going to go with "yes"

airflow/timetables/_cron.py

potiuk

LGTM (wiht the small nit - @uranusjr - I think second opinion would be great.

uranusjr · 2023-03-16T06:21:25Z

Many things to fix. Actually the test failure is DST related, which strongly indicates the logic is wrong (in the sense of not being backward compatible).

timkpaine · 2023-03-16T12:33:37Z

Many things to fix. Actually the test failure is DST related, which strongly indicates the logic is wrong (in the sense of not being backward compatible).

Yep I think that's expected, current behavior does not respect DST, new behavior will shift cron schedules to continue to run on the correct hours. Let me add a second test for the other side of DST just to be sure, but if there are tests that assume the old behavior I'll need to adjust those as well, and presumably some docs (which I think I looked at and they were already slightly wrong)

uranusjr · 2023-03-16T19:53:18Z

I want to be cautious breaking compatibility here; since there are tests ensuring the previous behaviour, there is a high chance someone is depending on it, and their work should not be interrupted.

I would suggest the new DST behaviour should be implemented in a new Timetable class, instead of changing how the cron syntax works.

timkpaine · 2023-03-16T22:13:46Z

changing how the cron syntax works

*fixing the cron parsing. If your cron says "* 9,10" but once a year runs on 10,11 and 8,10, it's not "changing how the syntax works", it's fixing a long-standing bug (possibly breaking users that rely on that bug). It also isn't an undoable change: if you want absolute time, use an absolute time timezone like UTC.

I don't think a separate timetable is necessary, and certainly having the default timetable when using cron be a broken one is not preferable. But maybe a flag on the existing cron timetable is acceptable? Something obvious like adjust_for_dst and possibly even a warning if you have a schedule that will be affected and don't set this flag. Most existing workarounds that I'm familiar with involve running +/- an hour and checking the real time to see if you should run, which would continue to work.

uranusjr · 2023-03-17T08:18:01Z

But maybe a flag on the existing cron timetable is acceptable? Something obvious like adjust_for_dst and possibly even a warning if you have a schedule that will be affected and don't set this flag.

The thing here is the semantic of schedule="<cron expr>" (or schedule_interval, they are the same) cannot change (until Airflow 3.0) due to compatibility policies. And you can’t dismiss the current behaviour as a bug; the fact it is ensured by test means the behaviour is delibrately designed by someone, and likely relied on someone as a feature.

potiuk · 2023-03-17T08:24:09Z

But maybe a flag on the existing cron timetable is acceptable? Something obvious like adjust_for_dst and possibly even a warning if you have a schedule that will be affected and don't set this flag.

The thing here is the semantic of schedule="<cron expr>" (or schedule_interval, they are the same) cannot change (until Airflow 3.0) due to compatibility policies. And you can’t dismiss the current behaviour as a bug; the fact it is ensured by test means the behaviour is delibrately designed by someone, and likely relied on someone as a feature.

Yeah. agree it's something we should be careful about, but having "adjust_standard_cron_spec_for_dst" (or smth like that) flag in the DAG and raising deprecation warning in case if someone uses "regular" cron and has this flag "false" sounds completely doable and way better as a path for potential breaking change in case we have Airflow 3 than having a separate timetable IMHO.

timkpaine · 2023-03-23T16:17:31Z

I agree completely that the bugged behavior is asserted by tests, and that we should be careful with a fix. I think a minor version with a new flag to restore the current behavior would be best, followed by a minor version with a new flag to get the fixed behavior would be next best, followed by waiting until a 3.0, but that's just my personal opinion.

But, I do think the behavior is bugged or at the very least unexpected. Its unclear to me what the code for fixing DST does (@uranusjr you wrote this code so I would've thought by "fix for DST" you meant "if I want to run at 9am and 10am local time, run at 9am and 10am local time even after a DST change, but I could be wrong, in which case im not sure what "fix DST" means. Maybe "if you wanted to run at 9am local time before DST, run at 10am local time after"? but in either case, it seems like either "0 9 *" and "0 9,10 *" should both run at 9am local always, or run at 9am/10am in the same way. Right now one will adjust by 1 hour, and the other will not).

The thing here is the semantic of schedule="<cron expr>" (or schedule_interval, they are the same)

This is incorrect. schedule="<cron expr>" is sometimes schedule interval, and sometimes schedule fixed. This is the crux of the usage problem, certain cron schedules (given non-fixed hours like 1,2 or 1-3) behave as interval, others with single hours behave as fixed.

And you can’t dismiss the current behaviour as a bug

I think it's almost certainly a bug. From the airflow docs themselves:

This is incorrect, it only applies to non-fixed cron schedules not fixed cron schedules. Also this is not what "respect daylight savings time" means to most readers I would assume.

From astronomer.io docs:

Also incorrect, similar reasons.

Some other samplings from the internet:
Incorrect post:

It does behave like that depending on the type of cron schedule you use.

StackOverflow showing how to account for the bug by running extra tasks on either side of the intended tasks, clearly not the expected or desired behavior:
https://stackoverflow.com/questions/43662571/how-to-properly-handle-daylight-savings-time-in-apache-airflow

the fact it is ensured by test means the behaviour is delibrately designed by someone, and likely relied on someone as a feature.

Completely agree, it is always annoying when bugs (or if we want to agree to disagree, "unexpected behavior") become relied on as features but given that common understanding would expect airflow to account for DST, a minority of users relying on certain behavior should be deprioritized over a majority of users expecting another, with obvious requirements of documenting/announcing and versioning so that all users have a path forward.

github-actions · 2023-05-08T00:11:08Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

ashb · 2023-07-25T10:20:28Z

Not respecting local TZ around DST change was a bug that was introduced with our switch to timetables.

Prior to that change the DST was meant to be reflected. So this is fixing a bug that will bite some people -- it's just that using local timezones isn't all that common.

@timkpaine If you are able and willing could you add a news fragment to warn users of this change, phrased something as "this reverts the TZ behavoiur around DST transitions to the pre $VER behaviour", then we can get this in.

timkpaine · 2023-07-25T13:16:41Z

after a few months away, I think all this code is unnecessary. You can simply force _should_fix_dst to False. The code that I'm "fixing" here was deliberately introduced in order to make sure something that runs at e.g. 9am runs at 10am during DST. To revert it, you don't need to add additional adjustment code, you just need to undo it:

    @cached_property
    def _should_fix_dst(self) -> bool:
        # Deprecated
        return False

    def _get_next(self, current: DateTime) -> DateTime:
        """Get the first schedule after specified time."""
        naive = make_naive(current, self._timezone)
        cron = croniter(self._expression, start_time=naive)
        scheduled = cron.get_next(datetime.datetime)
        return convert_to_utc(make_aware(scheduled, self._timezone))

    def _get_prev(self, current: DateTime) -> DateTime:
        """Get the first schedule before specified time"""
        naive = make_naive(current, self._timezone)
        cron = croniter(self._expression, start_time=naive)
        scheduled = cron.get_prev(datetime.datetime)
        return convert_to_utc(make_aware(scheduled, self._timezone))

I'm happy to make this change if desired.

newsfragments/30083.significant.rst

This is private interface anyway so we don't need to keep it around. Also move the logic into DAG.is_fixed_time_schedule() so it is not broken by the removal of the logic.

airflow/utils/timezone.py

newsfragments/30083.significant.rst

dstandish · 2023-11-14T22:10:28Z

newsfragments/30083.significant.rst

+Given a cron expression resulting in an interval-based schedule, such as
+``0 9-16 * * *``, job after a timezone change (e.g. entering or exiting daylight
+saving time) used to be scheduled with an offset to the change in timezone. For
+example, when operating in *America/New_York*, the above schedule would result
+in a run at 10am on Monday morning after entering DST, instead of the previous
+9am.
+
+This was intentional to account for the change in timezone introduced in Airflow
+2.2, but caused confusion to several use cases. The functionality is now
+reverted to its pre-2.2 state, and timetables will now always run at the same
+*clock time*, e.g. 9am in the above example.


It seems we should state that this was a bug that we are fixing, not simply a change that we are reverting. We have observed that it's not simply that the schedule doesn't run at the correct clock time, but that the dag actually gets stuck and fails to run.

Additionally I'm a bit confused by the 9am / 10am example. Are you saying that the behavior before this fix would be that the dag would run at 10am until DST transitions back, 6 months later? Or only the first run?

dstandish · 2023-11-14T22:12:35Z

tests/models/test_dag.py

+    # def test_following_previous_schedule_daily_dag_cet_to_cest(self):
+    #     """
+    #     Make sure DST transitions are properly observed
+    #     """
+    #     local_tz = pendulum.timezone("Europe/Zurich")
+    #     start = local_tz.convert(datetime.datetime(2018, 3, 25, 2), dst_rule=pendulum.PRE_TRANSITION)

-        utc = timezone.convert_to_utc(start)
+    #     utc = timezone.convert_to_utc(start)

-        dag = DAG("tz_dag", start_date=start, schedule="0 3 * * *")
+    #     dag = DAG("tz_dag", start_date=start, schedule="0 3 * * *")

-        prev = dag.previous_schedule(utc)
-        prev_local = local_tz.convert(prev)
+    #     prev = dag.previous_schedule(utc)
+    #     prev_local = local_tz.convert(prev)

-        assert prev_local.isoformat() == "2018-03-24T03:00:00+01:00"
-        assert prev.isoformat() == "2018-03-24T02:00:00+00:00"
+    #     assert prev_local.isoformat() == "2018-03-24T03:00:00+01:00"
+    #     assert prev.isoformat() == "2018-03-24T02:00:00+00:00"

-        _next = dag.following_schedule(utc)
-        next_local = local_tz.convert(_next)
+    #     _next = dag.following_schedule(utc)
+    #     next_local = local_tz.convert(_next)

-        assert next_local.isoformat() == "2018-03-25T03:00:00+02:00"
-        assert _next.isoformat() == "2018-03-25T01:00:00+00:00"
+    #     assert next_local.isoformat() == "2018-03-25T03:00:00+02:00"
+    #     assert _next.isoformat() == "2018-03-25T01:00:00+00:00"

-        prev = dag.previous_schedule(_next)
-        prev_local = local_tz.convert(prev)
+    #     prev = dag.previous_schedule(_next)
+    #     prev_local = local_tz.convert(prev)



seems something needs to be addressed here. Should these tests be removed? Or uncommented? Or do we have a decision to make about what the behavior should be?

This test actually almost passes, only fails in the final previous_schedule call. Since previous_schedule is deprecated anyway, I think it is OK to just remove that part.

I’m also converting it into a timetable test to not rely on following_schedule, which is also deprecated.

dstandish · 2023-11-14T22:15:20Z

tests/timetables/test_interval_timetable.py

+    assert cron_interval._get_prev(last_run).timestamp() == cron_fixed._get_prev(last_run).timestamp()
+
+
+def test_cron_interval_dst_next_leaving():


we should probably verify the DST transition behavior with the CronTriggerTimetable class also. perhaps parameterizing would be an easy way to do that.

dstandish · 2023-11-15T17:10:25Z

@timkpaine you still interested in working on this or you want someone else to take over?

timkpaine · 2023-11-18T17:05:09Z

I'm still working on it but I don't mind others pushing to my fork, the big outstanding items are make sure the two blocks of tests are fully uncommented, delete the redundant tests I added, and switch from pendulum internals to date time fold.

Co-authored-by: Daniel Standish <[email protected]>

Pendulum 3 will remove the dst_rule flag altogether, so this changes the code to use the fold flag instead, which will be the only way moving forward, and has been supported in Pendulum 2 for a long time anyway.

uranusjr · 2023-11-27T08:35:39Z

Hmm, after some more tests I’m not sure we have the right fix here. Using Switzerland as an example (it is +1/+2, making the mental arithmetic a bit easier), DST was entered in 2023 at 1am UTC (2→3am local) on March 26, so for 0 3 * * *` (every day at 3am), the intervals should be

UTC 2023-03-24T02:00:00 to 2023-03-25T02:00:00 (no DS so +1 for both)
UTC 2023-03-25T02:00:00 to 2023-03-26T01:00:00 (entering DS, the interval ends one hour early!)
UTC 2023-03-26T01:00:00 to 2023-03-27T01:00:00 (inside DST)

It is important for the second interval to end on 1am UTC since the wall clock in Switzerland goes to 3am at this moment. But from my testing the second interval does not end correctly with the current implementation in this PR. We need more work here.

timkpaine · 2023-11-27T12:20:51Z

"0 3 * * *" shouldn't be affected by this PR as that's a fixed timetable not an interval timetable I thought? For that schedule it should run at 3am local throughout, so in UTC it should shift by 1 hour

timkpaine · 2023-11-27T13:33:54Z

tests/timetables/test_interval_timetable.py

+        "0 9-16 * * 1-5", timezone=pendulum.timezone("America/New_York")
+    )
+    # Fixed cron
+    cron_fixed = CronDataIntervalTimetable("0 9 * * 1-5", timezone=pendulum.timezone("America/New_York"))


This and the test below need to get rid of 1-5 so that they're fixed not interval

uranusjr · 2023-11-27T16:53:31Z

"0 3 * * *" shouldn't be affected by this PR as that's a fixed timetable not an interval timetable I thought?

A cron expression can be either fixed or interval—those are orthogontal concepts. Using cron as an interval timetable specifically means intervals are generated from adjacent times. This change breaks CronDataIntervalTimetable.

uranusjr · 2023-12-03T03:32:36Z

I posted some details in #35887. Unfortunately I don’t think it is technically possible to fix this in Airflow.

uranusjr · 2023-12-04T10:57:31Z

Closing in favour of #35887. That PR now passes test cases in this one.

timkpaine · 2023-12-04T12:36:54Z

I don't think that's sufficient, it looks like the commit above when I disabled the second block of code in get next which didn't end up working in all cases. I'm going to keep on this one as I think it will be more complete, please reopen for now.

uranusjr · 2023-12-05T08:22:48Z

Sure, although I am relatively sure this would not work well without support from croniter.

timkpaine · 2023-12-05T19:21:23Z

See #35887 (comment)

timkpaine requested a review from uranusjr as a code owner March 13, 2023 23:02

timkpaine mentioned this pull request Mar 13, 2023

Incorrect DAG scheduling after DST #7999

Closed

timkpaine changed the title ~~fixes #7999 - account for change in UTC offset when performing next schedule calculations~~ Account for change in UTC offset when performing next schedule calculations - fixes #7999 Mar 14, 2023

potiuk reviewed Mar 14, 2023

View reviewed changes

airflow/timetables/_cron.py Outdated Show resolved Hide resolved

potiuk approved these changes Mar 14, 2023

View reviewed changes

potiuk linked an issue Mar 15, 2023 that may be closed by this pull request

AssertionError: next schedule shouldn't be earlier around DST change #30088

Closed

2 tasks

uranusjr changed the title ~~Account for change in UTC offset when performing next schedule calculations - fixes #7999~~ Account for change in UTC offset when performing next schedule calculations Mar 16, 2023

eladkal mentioned this pull request Mar 16, 2023

AssertionError: next schedule shouldn't be earlier around DST change #30088

Closed

2 tasks

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 8, 2023

github-actions bot closed this May 14, 2023

ashb reopened this Jul 25, 2023

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 26, 2023

eladkal added this to the Airflow 2.7.0 milestone Jul 26, 2023

eladkal added the type:improvement Changelog: Improvements label Jul 26, 2023

ephraimbuddy modified the milestones: Airflow 2.7.0, Airflow 2.7.1 Aug 3, 2023

pankajkoti self-requested a review August 7, 2023 06:48

uranusjr reviewed Aug 9, 2023

View reviewed changes

newsfragments/30083.significant.rst Outdated Show resolved Hide resolved

uranusjr and others added 3 commits November 14, 2023 21:44

Revise news fragment grammar

7f2038d

Remove _should_fix_dst entirely

9cd808b

This is private interface anyway so we don't need to keep it around. Also move the logic into DAG.is_fixed_time_schedule() so it is not broken by the removal of the logic.

Properly account for transitioning across DST bounds

41de224

dstandish reviewed Nov 14, 2023

View reviewed changes

airflow/utils/timezone.py Outdated Show resolved Hide resolved

Update airflow/utils/timezone.py

d04e8c3

dstandish reviewed Nov 14, 2023

View reviewed changes

newsfragments/30083.significant.rst Outdated Show resolved Hide resolved

dstandish reviewed Nov 14, 2023

View reviewed changes

nclaeys mentioned this pull request Nov 22, 2023

Airflow scheduler stopped working #35272

Open

2 tasks

uranusjr and others added 3 commits November 24, 2023 14:29

Better wording in changelog

72d7ff4

Co-authored-by: Daniel Standish <[email protected]>

Remove in_timezone() to rely on datetime.fold

4b9ee9d

Pendulum 3 will remove the dst_rule flag altogether, so this changes the code to use the fold flag instead, which will be the only way moving forward, and has been supported in Pendulum 2 for a long time anyway.

Allow make_aware caller to explicitly disable fold

3a123df

uranusjr mentioned this pull request Nov 27, 2023

Account for change in UTC offset when calculating next schedule #35887

Merged

Uncomment second set of tests

ebfbbfa

timkpaine commented Nov 27, 2023

View reviewed changes

uranusjr closed this Dec 4, 2023

uranusjr reopened this Dec 5, 2023

timkpaine closed this Dec 5, 2023

ephraimbuddy removed this from the Airflow 2.8.0 milestone Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for change in UTC offset when performing next schedule calculations #30083

Account for change in UTC offset when performing next schedule calculations #30083

timkpaine commented Mar 13, 2023

joshzana commented Mar 14, 2023

timkpaine commented Mar 14, 2023

potiuk left a comment

uranusjr commented Mar 16, 2023

timkpaine commented Mar 16, 2023

uranusjr commented Mar 16, 2023

timkpaine commented Mar 16, 2023 •

edited

Loading

uranusjr commented Mar 17, 2023

potiuk commented Mar 17, 2023 •

edited

Loading

timkpaine commented Mar 23, 2023 •

edited

Loading

github-actions bot commented May 8, 2023

ashb commented Jul 25, 2023

timkpaine commented Jul 25, 2023 •

edited

Loading

dstandish Nov 14, 2023

dstandish Nov 14, 2023

uranusjr Nov 27, 2023

dstandish Nov 14, 2023

dstandish commented Nov 15, 2023

timkpaine commented Nov 18, 2023

uranusjr commented Nov 27, 2023 •

edited

Loading

timkpaine commented Nov 27, 2023 •

edited

Loading

timkpaine Nov 27, 2023

uranusjr commented Nov 27, 2023

uranusjr commented Dec 3, 2023

uranusjr commented Dec 4, 2023

timkpaine commented Dec 4, 2023 •

edited

Loading

uranusjr commented Dec 5, 2023

timkpaine commented Dec 5, 2023

		assert cron_interval._get_prev(last_run).timestamp() == cron_fixed._get_prev(last_run).timestamp()


		def test_cron_interval_dst_next_leaving():

Account for change in UTC offset when performing next schedule calculations #30083

Account for change in UTC offset when performing next schedule calculations #30083

Conversation

timkpaine commented Mar 13, 2023

joshzana commented Mar 14, 2023

timkpaine commented Mar 14, 2023

potiuk left a comment

Choose a reason for hiding this comment

uranusjr commented Mar 16, 2023

timkpaine commented Mar 16, 2023

uranusjr commented Mar 16, 2023

timkpaine commented Mar 16, 2023 • edited Loading

uranusjr commented Mar 17, 2023

potiuk commented Mar 17, 2023 • edited Loading

timkpaine commented Mar 23, 2023 • edited Loading

github-actions bot commented May 8, 2023

ashb commented Jul 25, 2023

timkpaine commented Jul 25, 2023 • edited Loading

dstandish Nov 14, 2023

Choose a reason for hiding this comment

dstandish Nov 14, 2023

Choose a reason for hiding this comment

uranusjr Nov 27, 2023

Choose a reason for hiding this comment

dstandish Nov 14, 2023

Choose a reason for hiding this comment

dstandish commented Nov 15, 2023

timkpaine commented Nov 18, 2023

uranusjr commented Nov 27, 2023 • edited Loading

timkpaine commented Nov 27, 2023 • edited Loading

timkpaine Nov 27, 2023

Choose a reason for hiding this comment

uranusjr commented Nov 27, 2023

uranusjr commented Dec 3, 2023

uranusjr commented Dec 4, 2023

timkpaine commented Dec 4, 2023 • edited Loading

uranusjr commented Dec 5, 2023

timkpaine commented Dec 5, 2023

timkpaine commented Mar 16, 2023 •

edited

Loading

potiuk commented Mar 17, 2023 •

edited

Loading

timkpaine commented Mar 23, 2023 •

edited

Loading

timkpaine commented Jul 25, 2023 •

edited

Loading

uranusjr commented Nov 27, 2023 •

edited

Loading

timkpaine commented Nov 27, 2023 •

edited

Loading

timkpaine commented Dec 4, 2023 •

edited

Loading