kedacore / keda

KEDA is a Kubernetes-based Event Driven Autoscaling component. It provides event driven scale for any container running in Kubernetes
https://keda.sh
Apache License 2.0
8.33k stars 1.05k forks source link

Cron trigger does not support valid ranges #2034

Closed jyaworski closed 3 years ago

jyaworski commented 3 years ago

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

JorTurFer commented 3 years ago

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 commented 3 years ago

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

@Ritikaa96 FYI^

JorTurFer commented 3 years ago

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

mboutet commented 3 years ago

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

tomkerkhove commented 3 years ago

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

JorTurFer commented 3 years ago

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 commented 3 years ago

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

mboutet commented 3 years ago

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 commented 2 years ago

Is this feature included in 2.5.0 release ?

JorTurFer commented 2 years ago

yes it is