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

Cron trigger does not support valid ranges #2034

Closed
jyaworski opened this issue Aug 13, 2021 · 10 comments · Fixed by #2038 or kedacore/keda-docs#514
Closed

Cron trigger does not support valid ranges #2034

jyaworski opened this issue Aug 13, 2021 · 10 comments · Fixed by #2038 or kedacore/keda-docs#514
Assignees
Labels
bug Something isn't working

Comments

@jyaworski
Copy link

Report

Hello:

The fix for #1809 appears to have broken our cron setup. Checking for - removes some valid cron entries.

Expected Behavior

Using the following, I would expect it to scale to 5 replicas in January, October, November, and December from 8:30 AM to 1:00 PM in the America/New_York TZ:

  - metadata:
      desiredReplicas: "5"
      end: 0 13 * 1,10-12 *
      start: 30 8 * 1,10-12 *
      timezone: America/New_York
    type: cron

Actual Behavior

Warning  KEDAScalerFailed         10m (x504 over 42h)  keda-operator  error parsing cron metadata: error parsing start schedule. map[desiredReplicas:5 end:0 13 * 1,10-12 * start:30 8 * 1,10-12 * timezone:America/New_York]: range or hyphenated inputs are not allowed

Steps to Reproduce the Problem

Apply any cron trigger with a range.

Logs from KEDA operator

No response

KEDA Version

2.4.0

Kubernetes Version

No response

Platform

Amazon Web Services

Scaler Details

Cron

Anything else?

No response

@jyaworski jyaworski added the bug Something isn't working label Aug 13, 2021
@JorTurFer
Copy link
Member

JorTurFer commented Aug 16, 2021

I think that the change required to support ranges again could be small. The package that it's in use supports the creation of a Parser. Maybe it's possible to validate the values trying to parse them instead evaluate if they contain -.
Maybe something like this:

parser := cron.NewParser(cron.SecondOptional | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow | cron.Descriptor)
if val, ok := config.TriggerMetadata["start"]; ok && val != "" {
	_, err := parser.Parse(val)
	if err != nil {
		return nil, fmt.Errorf("error parsing start schedule. %s: range or hyphenated inputs are not allowed", config.TriggerMetadata)
	}
	meta.start = val
} else {
	return nil, fmt.Errorf("no start schedule specified. %s", config.TriggerMetadata)
}

Reviewing the code, apart from this validation, all should work without any other problem. As I can see, the process (in high level) is basically query the next start and the next end. start < now < end. If the package supports ranges (and I think that it does), I can't see any problem to support them in KEDA.
I can take a look and open a PR if it's supported and you agree @zroubalik @tomkerkhove @ahmelsayed

@zroubalik
Copy link
Member

@JorTurFer that sounds like a nice approach! Opening a PR is a good start 🙏

@Ritikaa96 FYI^

@JorTurFer
Copy link
Member

JorTurFer commented Aug 17, 2021

Cool! You can assign it to me if you want, I will do it during this week :)

@mboutet
Copy link
Contributor

mboutet commented Aug 31, 2021

Any idea when the next release with this fix will be available? Thanks 😄

@tomkerkhove
Copy link
Member

We typically ship every 2 months so that would be early October.

@JorTurFer
Copy link
Member

If you need to try it before, you can use tag main to get the latest changes until the next release. For example, I have done that to use the latest changes related with RabbitMQ

@zroubalik
Copy link
Member

Yeah, but bear in mind that main is not stable :)

@mboutet
Copy link
Contributor

mboutet commented Aug 31, 2021

Thanks for the quick reply. I've built the images from main and I'm using this in the meantime. No issues so far.

@akshaygupta2208
Copy link

Is this feature included in 2.5.0 release ?

@JorTurFer
Copy link
Member

yes it is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants