Feature: Eventbridge v2: add schedule executor by maxhoheiser · Pull Request #10817 · localstack/localstack

maxhoheiser · 2024-05-14T16:12:31Z

Motivation

Rules can have a schedule that invokes the defined (max 5) targets for the rule, not based on incoming events that are matched via a pattern, but this deveined schedule.
https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-create-rule-schedule.html

The schedule can either be specified as a rate expression (https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-rate-expressions.html) or as a cron expression (https://docs.aws.amazon.com/eventbridge/latest/userguide/eb-cron-expressions.html)

Changes

Add validation of schedule cron or rate.
Add schedule function that creates default event or uses custom event input and uses TargetSender to dispatch event.
Fix JobScheduleExecttor to use UTC time, since default cron schedule is in UTC.
Add logic to convert rate to cron.
Expand test suite.

github-actions · 2024-05-14T18:16:35Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 44m 29s ⏱️ + 2m 56s
2 964 tests +10 2 662 ✅ +9 302 💤 +1 0 ❌ ±0
2 966 runs +10 2 662 ✅ +9 304 💤 +1 0 ❌ ±0

Results for commit 7a82594. ± Comparison against base commit 98dbcbc.

This pull request removes 22 and adds 32 tests. Note that renamed tests count towards both.

tests.aws.services.events.scheduled_rules.test_events_scheduled_rules_logs ‑ test_scheduled_rule_logs
tests.aws.services.events.scheduled_rules.test_events_scheduled_rules_sqs ‑ test_scheduled_rule_sqs
tests.aws.services.events.test_events_rules ‑ test_create_rule_with_one_unit_in_singular_should_succeed[rate(1 day)]
tests.aws.services.events.test_events_rules ‑ test_create_rule_with_one_unit_in_singular_should_succeed[rate(1 hour)]
tests.aws.services.events.test_events_rules ‑ test_create_rule_with_one_unit_in_singular_should_succeed[rate(1 minute)]
tests.aws.services.events.test_events_rules ‑ test_put_rule_invalid_rate_schedule_expression[ rate(10 minutes)]
tests.aws.services.events.test_events_rules ‑ test_put_rule_invalid_rate_schedule_expression[rate( 10 minutes )]
tests.aws.services.events.test_events_rules ‑ test_put_rule_invalid_rate_schedule_expression[rate()]
tests.aws.services.events.test_events_rules ‑ test_put_rule_invalid_rate_schedule_expression[rate(-10 minutes)]
tests.aws.services.events.test_events_rules ‑ test_put_rule_invalid_rate_schedule_expression[rate(0 minutes)]
…

tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ test_schedule_cron_target_sqs
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 10 * * ? *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 18 ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0 8 1 * ? *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/10 * ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/15 * * * ? *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/30 0-2 ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/30 20-23 ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(0/5 8-17 ? * MON-FRI *)]
tests.aws.services.events.test_events_schedule.TestScheduleCron ‑ tests_put_rule_with_schedule_cron[cron(15 12 * * ? *)]
…

♻️ This comment has been updated with latest results.

maxhoheiser · 2024-05-15T11:03:24Z

tests/aws/services/events/test_events_schedule.py

+        response = aws_client.events.list_targets_by_rule(Rule=rule_name)
+        snapshot.match("list-targets", response)
+
+        time.sleep(60)


required to wait until rate kicks in - minimum defineable rate duration is 1 minute

maxhoheiser · 2024-05-15T11:04:21Z

tests/aws/services/events/test_events_schedule.py

+            ],
+        )
+
+        time.sleep(120)  # required to wait for time delta 1 minute starting from next full minute


although cron tab fires in 1 minute, it is required to wait until start of next full minute to begin with due to cold start on AWS.

maxhoheiser · 2024-05-15T11:05:34Z

localstack/services/events/provider.py

        self._rule_services_store: RuleServiceDict = {}
        self._target_sender_store: TargetSenderDict = {}

+    def on_before_start(self):


required to start background thread of job scheduler

maxhoheiser · 2024-05-15T11:07:18Z

localstack/services/events/scheduler.py

+        delay_secs = schedule.next(
+            default_utc=True
+        )  # utc default time format for rule schedule cron
+        # TODO fix execute on exact cron time


the rework of the job scheduler in v2 needs to deal with executing a schedule cron at the exact specified time, not run the job if it should occur in less than 60 seconds from now.

maxhoheiser · 2024-05-15T11:07:49Z

localstack/services/events/scheduler.py

-                del self.jobs[i]
-            else:
-                i += 1
+        self.jobs = [job for job in self.jobs if job.job_id != job_id]


👍
💡 We still need to make such modifications thread-safe using locks in the scheduler rework.

maxhoheiser · 2024-05-15T11:10:18Z

tests/aws/services/events/test_events_schedule.py

+    @pytest.mark.xfail(
+        reason="This test is flaky is CI, might be race conditions"  # FIXME: investigate and fix
+    )
+    def test_scheduled_rule_logs(


this test needs to be completely reworked, due to the unknown time requirement this will take, it is being deferred to a later stage since the test case is covered by other tests mostly.

makes sense 👍
Remember to track it in a backlog item, so we don't forget about it.

joe4dev

LGTM 👍
This PR integrates the same scheduler as EventBridge v1 and improves test coverage and parity (especially related to schedule conversions).

We will still need to tackle the scheduler rework (based on Thomas's PR #9298) in a follow-up, but it makes sense to tackle this separately as part of the "EventBridge Scheduler" roadmap item and focus on EventBridge provider v1 parity here. Sensible decision 👍

localstack/services/events/scheduler.py

tests/aws/services/events/conftest.py

tests/aws/services/events/helper_functions.py

tests/aws/services/events/test_events_schedule.py

joe4dev · 2024-05-15T20:33:56Z

tests/aws/services/events/test_events_schedule.py

+    @pytest.mark.xfail(
+        reason="This test is flaky is CI, might be race conditions"  # FIXME: investigate and fix
+    )
+    def test_scheduled_rule_logs(


makes sense 👍
Remember to track it in a backlog item, so we don't forget about it.

localstack/services/events/provider.py

joe4dev · 2024-05-16T07:31:28Z

localstack/services/events/scheduler.py

-                del self.jobs[i]
-            else:
-                i += 1
+        self.jobs = [job for job in self.jobs if job.job_id != job_id]


👍
💡 We still need to make such modifications thread-safe using locks in the scheduler rework.

tests/aws/services/events/scheduled_rules/test_events_scheduled_rules_logs.py

localstack/services/events/scheduler.py

joe4dev · 2024-07-04T09:54:08Z

tests/aws/services/events/conftest.py

+        if not name:
+            name = f"test-log-group-{short_uid()}"
+
+        aws_client.logs.create_log_group(logGroupName=name)


@maxhoheiser I learned while testing pipes that log groups are not created instantly at AWS. Even worse was that a subsequent describe_log_groups call failed in this example:

log_group_name = f"test-log-group-{short_uid()}" aws_client.logs.create_log_group(logGroupName=log_group_name) def _log_group_created(): describe_log_groups_response = aws_client.logs.describe_log_groups( logGroupNamePattern=log_group_name ) log_groups = describe_log_groups_response.get("logGroups", []) return len(log_groups) > 0 # create_log_group does not happen instantly on AWS assert poll_condition(_log_group_created, timeout=5, interval=0.5) describe_log_groups_response = aws_client.logs.describe_log_groups( logGroupNamePattern=log_group_name ) log_group_arn = describe_log_groups_response["logGroups"][0]["arn"] cleanups.append(lambda: aws_client.logs.delete_log_group(logGroupName=log_group_name))

with the following error:

> log_group_arn = describe_log_groups_response["logGroups"][0]["arn"] E IndexError: list index out of range

I ended up returning the log_group_arn through the helper. Ideally, we could reuse a reliable fixture.

👉 It might be worth adding a comment to the fixture or implementing a waiter.

maxhoheiser added aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels May 14, 2024

maxhoheiser self-assigned this May 14, 2024

maxhoheiser requested a review from joe4dev May 14, 2024 16:12

maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from 424f308 to ef1d553 Compare May 14, 2024 17:07

maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from ef1d553 to 3e52613 Compare May 15, 2024 08:07

maxhoheiser added 13 commits May 15, 2024 11:39

feat: add job scheduler

71256af

feat: add test put rule with schedule

5348d7b

feat: add test invalid schedule expression

4902c9f

feat: remove old schedule tests

0867cae

feat: add test rule with schedule custom eventbus

16a75bd

feat: add test schedule sqs target

3218f3b

feat: add test time delta for rate

125e93d

feat: add test put rule with schedule cron

da734ee

feat: add test schedule cron target sqs

856a042

feat: add put rule cron all variations

57c7e66

feat: add test schedule rate custom input

cd97927

feat: add schedule rate target logs

a7d5534

feat: remove old schedule tests

410d0bf

maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from 5946c25 to e5aab49 Compare May 15, 2024 09:39

maxhoheiser added 3 commits May 15, 2024 12:59

feat: fix tests

a72d26d

fix: cron time not using utc

38bc697

fix: job scheduler not executing on exact cron schedule

933ed0a

maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from af03fb2 to 933ed0a Compare May 15, 2024 10:59

maxhoheiser commented May 15, 2024

View reviewed changes

maxhoheiser marked this pull request as ready for review May 15, 2024 11:10

maxhoheiser requested review from dfangl and dominikschubert as code owners May 15, 2024 11:10

fix: only create job for rule with schedule

5564762

joe4dev approved these changes May 16, 2024

View reviewed changes

maxhoheiser added 3 commits May 16, 2024 18:59

refactor: use f-string

8497a05

feat: add function detect old provider

08bf1ed

refactor: add docstring to cron helper method

7a82594

maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from b37f1b8 to 7a82594 Compare May 16, 2024 17:17

maxhoheiser merged commit 8d51830 into master May 17, 2024

maxhoheiser deleted the feature/eventbridge_v2-add-schedule-executor branch May 17, 2024 07:22

joe4dev reviewed Jul 4, 2024

View reviewed changes

Uh oh!

Conversation

maxhoheiser commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

github-actions bot commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxhoheiser commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading