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

Feature: Eventbridge v2: add schedule executor #10817

Merged
merged 20 commits into from
May 17, 2024

Conversation

maxhoheiser
Copy link
Member

@maxhoheiser maxhoheiser commented May 14, 2024

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.

@maxhoheiser 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 maxhoheiser self-assigned this May 14, 2024
@maxhoheiser maxhoheiser requested a review from joe4dev May 14, 2024 16:12
@maxhoheiser maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from 424f308 to ef1d553 Compare May 14, 2024 17:07
Copy link

github-actions bot commented May 14, 2024

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 maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from ef1d553 to 3e52613 Compare May 15, 2024 08:07
@maxhoheiser maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from 5946c25 to e5aab49 Compare May 15, 2024 09:39
@maxhoheiser maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from af03fb2 to 933ed0a Compare May 15, 2024 10:59
response = aws_client.events.list_targets_by_rule(Rule=rule_name)
snapshot.match("list-targets", response)

time.sleep(60)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

],
)

time.sleep(120) # required to wait for time delta 1 minute starting from next full minute
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -151,6 +154,12 @@ def __init__(self):
self._rule_services_store: RuleServiceDict = {}
self._target_sender_store: TargetSenderDict = {}

def on_before_start(self):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required to start background thread of job scheduler

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

del self.jobs[i]
else:
i += 1
self.jobs = [job for job in self.jobs if job.job_id != job_id]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaner

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
localstack/services/events/scheduler.py Show resolved Hide resolved
tests/aws/services/events/conftest.py Show resolved Hide resolved
tests/aws/services/events/helper_functions.py Show resolved Hide resolved
@pytest.mark.xfail(
reason="This test is flaky is CI, might be race conditions" # FIXME: investigate and fix
)
def test_scheduled_rule_logs(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

localstack/services/events/provider.py Show resolved Hide resolved
del self.jobs[i]
else:
i += 1
self.jobs = [job for job in self.jobs if job.job_id != job_id]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

localstack/services/events/scheduler.py Show resolved Hide resolved
@maxhoheiser maxhoheiser force-pushed the feature/eventbridge_v2-add-schedule-executor branch from b37f1b8 to 7a82594 Compare May 16, 2024 17:17
@maxhoheiser maxhoheiser merged commit 8d51830 into master May 17, 2024
30 checks passed
@maxhoheiser maxhoheiser deleted the feature/eventbridge_v2-add-schedule-executor branch May 17, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:events Amazon EventBridge semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants