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

Add PredictKube scaler #2418

Merged
merged 30 commits into from
Jan 26, 2022
Merged

Add PredictKube scaler #2418

merged 30 commits into from
Jan 26, 2022

Conversation

daniel-yavorovich
Copy link
Contributor

@daniel-yavorovich daniel-yavorovich commented Dec 28, 2021

This PR adds a new PredictKube scaler, ready-to-use for the community.
PredictKube - is a tool for proactive scaling based on the AI model’s prediction.

Related docs PR: kedacore/keda-docs#617

This is an example of what the TriggerAuthentication and the ScaledObject definitions would look like:

apiVersion: v1
kind: Secret
metadata:
  name: predictkube-secrets
type: Opaque
data:
  apiKey: # Required: base64 encoded value of PredictKube apiKey
---
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-trigger-auth-predictkube-secret
spec:
  secretTargetRef:
  - parameter: apiKey
    name: predictkube-secrets
    key: apiKey
---
apiVersion: keda.sh/v1alpha1
kind: ScaledObject
metadata:
  name: example-app-scaler
spec:
  scaleTargetRef:
    name: example-app
  pollingInterval: 60
  cooldownPeriod: 300
  minReplicaCount: 3
  maxReplicaCount: 50
  triggers:
  - type: predictkube
    metadata:
      predictHorizon: "2h"
      historyTimeWindow: "7d"  # We recomend to use minimum 7-14 days time window as historical data
      prometheusAddress: http://kube-prometheus-stack-prometheus.monitoring:9090
      metricName: http_requests_total # Note: name to identify the metric, generated value would be `predictkube-http_requests_total`
      query: sum(irate(http_requests_total{pod=~"example-app-.*"}[2m]))
      queryStep: "2m" # Note: query step duration for range prometheus queries
      threshold: '2000' # Value to start scaling for
    authenticationRef:
      name: keda-trigger-auth-predictkube-secret

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Related to #197, #2401
Fixes #2458

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

First, thanks a lot for this contribution ❤️
There are some topics that I commented in the code and apart from those, there are 2 global points:

  • Add e2e tests for the scaler (we could add needed secrets if you need)
  • Use s for referring the scaler instead of pkg. The majority if the scalers use s and it gives consistency across them.

And again, thanks a lot for this ❤️

pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from a team January 3, 2022 20:58
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

+1 to the concerns raised by @JorTurFer
I will add one thing, would be nice to describe the architecture a little bit, ie. what is happenning under the hood, since this is not a standard scaler. The best thing is to open an issue and describe it there. And cover this in the docs as well.

.github/workflows/main-build.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/predictkube_scaler.go Outdated Show resolved Hide resolved
@alex60217101990 alex60217101990 force-pushed the predictkube-scaler branch 2 times, most recently from 9f374a8 to f48aeb8 Compare January 10, 2022 09:58
@alex60217101990 alex60217101990 force-pushed the predictkube-scaler branch 2 times, most recently from 8b7c520 to 1ba8a63 Compare January 10, 2022 13:44
Signed-off-by: Daniel Yavorovych <[email protected]>
@zroubalik
Copy link
Member

There are 20 vulnerabilities in various dependencies brought through github.com/searKing/[email protected].

@zroubalik
Copy link
Member

zroubalik commented Jan 25, 2022

There are 20 vulnerabilities in various dependencies brought through github.com/searKing/[email protected].

I think that a better way would be actually not implement enums via that library (so it doesn't have to be included) and use more standard/idiomatic golang way. If I am not mistaken, that is the only place where this dependency is being used.

@daniel-yavorovich
Copy link
Contributor Author

There are 20 vulnerabilities in various dependencies brought through github.com/searKing/[email protected].

I think that a better way would be actually not implement enums via that library (so it doesn't have to be included) and use more standard/idiomatic golang way. If I am not mistaken, that is the only place where this dependency is being used.

We have implemented enum with golang standard way, thank you.

Signed-off-by: alex60217101990 <[email protected]>
@zroubalik
Copy link
Member

zroubalik commented Jan 26, 2022

/run-e2e predictkube*
Update: You can check the progres here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution, it is looking great!

@zroubalik
Copy link
Member

@daniel-yavorovich @alex60217101990 could you please resolve the outstanding issues in docs pr so we can go ahead and merge this one? Thanks!

@zroubalik zroubalik merged commit 59f8abb into kedacore:main Jan 26, 2022
markrzasa pushed a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>
markrzasa pushed a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>
Signed-off-by: Mark Rzasa <[email protected]>
markrzasa pushed a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>
markrzasa added a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
author Mark Rzasa <[email protected]> 1642970493 -0500
committer Mark Rzasa <[email protected]> 1643317483 -0500

app insights scaler

Signed-off-by: Mark Rzasa <[email protected]>

Add PredictKube scaler (kedacore#2418)

Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>

bump deps (kedacore#2563)

Signed-off-by: Zbynek Roubalik <[email protected]>

Clear scalers cache correctly both in Operator and Metrics Server (kedacore#2564)

Signed-off-by: Zbynek Roubalik <[email protected]>

test: Provide e2e secrets for Azure Application Insights scaler (kedacore#2568)

Signed-off-by: Tom Kerkhove <[email protected]>
markrzasa added a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
author Mark Rzasa <[email protected]> 1642970493 -0500
committer Mark Rzasa <[email protected]> 1643317483 -0500

app insights scaler

Signed-off-by: Mark Rzasa <[email protected]>

Add PredictKube scaler (kedacore#2418)

Signed-off-by: Daniel Yavorovych <[email protected]>
Signed-off-by: alex60217101990 <[email protected]>

bump deps (kedacore#2563)

Signed-off-by: Zbynek Roubalik <[email protected]>

Clear scalers cache correctly both in Operator and Metrics Server (kedacore#2564)

Signed-off-by: Zbynek Roubalik <[email protected]>

test: Provide e2e secrets for Azure Application Insights scaler (kedacore#2568)

Signed-off-by: Tom Kerkhove <[email protected]>
Signed-off-by: Mark Rzasa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an PredicetKube scaler
4 participants