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

[BUG] Flytescheduler cron expression validation process #4953

Open
2 tasks done
Tracked by #5783
BroderPeters opened this issue Feb 26, 2024 · 6 comments
Open
2 tasks done
Tracked by #5783

[BUG] Flytescheduler cron expression validation process #4953

BroderPeters opened this issue Feb 26, 2024 · 6 comments
Assignees
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working documentation Improvements or additions to documentation flyteadmin Issue for FlyteAdmin Service hacktoberfest

Comments

@BroderPeters
Copy link

Describe the bug

Cron expressions like 0 6 * * MON#1 are currently able to being registered, display and executed, but flytescheduler produces the following error logs:

unable to register the schedule {BaseModel:{ID:1259 CreatedAt:2024-02-05 09:51:51.390164 +0000 UTC UpdatedAt:2024-02-05 09:51:51.390164 +0000 UTC DeletedAt:<nil>} SchedulableEntityKey:{Project:<project> Domain:development Name:<my>-launchplan Version:3622cca2ce009e394c1d193d43b6fe46c062afdc} CronExpression:0 6 * * MON#1 FixedRateValue:0 Unit:MINUTE KickoffTimeInputArg: Active:0xc000a042b4} due to failed to parse int from MON#1: strconv.Atoi: parsing "MON#1": invalid syntax

Which according to the flyte docs should also be the expected behavior, as the # symbol is not listed here. Generally, # is used for the nth occurrence of a day of a week in a month.

From a discussion in the community slack

Expected behavior

Either cron expression that don't fit the allowed symbols from the docs are properly validated while registering the launch plan or expressions with symbols like # are also supported in the flytescheduler validation.

Additional context to reproduce

Used Launchplan:

LaunchPlan.get_or_create(
    name="my_launchplan",
    workflow=wf,
    schedule=CronSchedule(
        schedule="0 6 * * MON#1",  # UTC, https://docs.flyte.org/en/latest/concepts/schedules.html#cron-expression-table
    )
)

Screenshots

image
image

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@BroderPeters BroderPeters added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Feb 26, 2024
Copy link

welcome bot commented Feb 26, 2024

Thank you for opening your first issue here! 🛠

Copy link

dosubot bot commented Feb 26, 2024

🤖

Hey @BroderPeters, back again to keep us on our toes, huh? 😄 Always a pleasure seeing your name pop up here!

The error you're encountering is likely due to the fact that the cron expression 0 6 * * MON#1 is not supported by the cron parser used in Flyte. The # symbol is not a standard cron expression and is not mentioned in the cron implementation described in the comments of the CronSchedule message in the schedule.proto file.

The cron implementation used by Flyte is based on the standard cron as described by Wikipedia and also supports nonstandard predefined scheduling definitions as described by Amazon CloudWatch, except @reboot.

To resolve this issue, you have two options:

  1. Modify your cron expression to fit the supported format. For example, if you want to schedule a job for the first Monday of every month at 6:00, you could use a cron expression like 0 6 1-7 * MON. This will schedule the job to run at 6:00 on Monday, but only if the date is between the 1st and the 7th, effectively ensuring that it runs on the first Monday of the month.

  2. If you need to use the # symbol or other non-standard cron expressions, you would need to modify the cron parser used by Flyte to support these expressions. This would involve changing the source code of Flyte and may not be feasible depending on your situation.

Please note that any changes to the cron parser would need to be thoroughly tested to ensure they don't introduce any new issues. You can find more information about the cron parser used by Flyte in the schedule.proto file.

Sources

About Dosu

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@dosubot dosubot bot added the documentation Improvements or additions to documentation label Feb 26, 2024
@pingsutw pingsutw added flyteadmin Issue for FlyteAdmin Service and removed untriaged This issues has not yet been looked at by the Maintainers labels Feb 29, 2024
@eapolinario eapolinario added the backlogged For internal use. Reserved for contributor team workflow. label Jul 1, 2024
@nitram509
Copy link

After some triaging I found out:

  • the file flyteadmin/scheduler/schedule_executor.go is delegating cron expression parsing to the library robfig/cron
  • the library robfig/cron is no more maintained since ca. Jan.2020
  • Flyte already is replacing the cron lib with unionai/cron (see go.mod file)
  • the original robfig/cron has plenty of bug-fixes as open PRs, e.g. there is one to add the 'day of week in a month' feature, see add '#' support for the day-of-week field robfig/cron#394

My recommendation:

  • fork the robfig/cron to flyteorg and apply all the patches
  • use flyteorg/cron in your code == this will also fix this issue.

Reasoning

  • robfig/cron is no more maintained, and the replacement used neither
  • core features of cron parsing and scheduling rely on that library
  • taking control over the source and applying patches is reliable and secure for future builds

@gagandeepp
Copy link

Interested please assign

@peterxcli
Copy link
Contributor

#take

@peterxcli
Copy link
Contributor

Close with #5951

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working documentation Improvements or additions to documentation flyteadmin Issue for FlyteAdmin Service hacktoberfest
Projects
None yet
Development

No branches or pull requests

7 participants