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

Declarative scaler config #5676

Merged
merged 5 commits into from
May 13, 2024

Conversation

wozniakjan
Copy link
Member

@wozniakjan wozniakjan commented Apr 10, 2024

Various scaler configurations are inconsistent in implementation as discussed in #5037. There has been a great effort in improving the situation with getParameterFromConfigV2() in #5228 (with further enhancements in #5319 and #5314).

There are likely always going to be some situations that require imperative parsing of the provided configs and the helper getParameterFromConfigV2(), but for the majority of cases, I believe we can implement an even more generic, more convenient and declarative algorithm, similar for example to the API of well known json.Unmarshal().

My desired goal is to allow each scaler to define the configuration struct with proper field tags to fine-tune the parsing algorithm, call a single helper method TypedConfig() defined on ScalerConfig struct and consume unmarshalled typed config without the need for explicit typecasting.

Example:

type MyConfigXYZ struct {
  Flag    bool           `keda:"name=flag,    order=triggerMetadata"`
  Addr    string         `keda:"name=addr,    order=triggerMetadata;resolvedEnv, optional, default=d"`
  Headers map[string]int `keda:"name=headers, order=triggerMetadata,             optional, default=d"`
}

sc := &scalerconfig.ScalerConfig{ ... }
conf := MyConfigXYZ{}
if err := sc.TypedConfig(conf); err != nil {
   ...
}

Where the MyConfigXYZ is an arbitrary structure with fields containing properly formatted keda tag. Callers can control

  • where to obtain the value from - triggerMetadata, resolvedEnv, authParams, and which map has a precedence
  • whether it's optional or deprecated
  • what is default value
  • name as a key to the map
  • allowed values with enum
  • mutually exclusive values with exclusive

The algorithm is based on go reflections and dynamic parsing of a structure, natively supports slices and maps and url.Values so it might eventually supersede helpers in pkg/util/parse_string.go

PR structure

  • 4475519 core implementation
  • d5d0b10 sample conversion of prometheus scaler
  • 43b6c10 implementation of prometheus auth parsing

Checklist

Relates to #5037

@wozniakjan wozniakjan requested a review from a team as a code owner April 10, 2024 12:52
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.

love

@SpiritZhou
Copy link
Contributor

Could this be used to generate scalers' schema?

@wozniakjan
Copy link
Member Author

Could this be used to generate scalers' schema?

we may need another set of field tags but I think it should be possible

@wozniakjan
Copy link
Member Author

just fyi, I started refactoring auth params config so I will mark this as draft until it's ready for a review again

@wozniakjan wozniakjan marked this pull request as draft April 30, 2024 16:07
@wozniakjan wozniakjan force-pushed the declarative_scaler_config branch 3 times, most recently from 6a6c9b0 to a80d471 Compare May 8, 2024 10:23
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@wozniakjan wozniakjan marked this pull request as ready for review May 8, 2024 10:52
@wozniakjan
Copy link
Member Author

wozniakjan commented May 8, 2024

/run-e2e
Update: You can check the progress here

@wozniakjan
Copy link
Member Author

I think we can give it a try, ptal @kedacore/keda-core-contributors

after this merges, I will try to tackle the scalers one by one, would you like to divide and conquer @dttung2905?

@dttung2905
Copy link
Contributor

I think we can give it a try, ptal @kedacore/keda-core-contributors

after this merges, I will try to tackle the scalers one by one, would you like to divide and conquer @dttung2905?

Absolutely, since there are many scalers, we can divide amongst ourselves and other community members who are interested as well. We can create a separate issue to track completion progress once this PR is merged 🤘

Copy link
Contributor

@dttung2905 dttung2905 left a comment

Choose a reason for hiding this comment

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

Thanks @wozniakjan for spending times on making this changes, it's not an easy feat at all.
I have 1 minor comment in my first pass going through the PR. Will probably go through it during the weekend when I have time 😄

pkg/scalers/scalersconfig/typed_config.go Outdated Show resolved Hide resolved
pkg/scalers/scalersconfig/typed_config_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
* rename 'parsingOrder' tag to 'order'
* rename 'exclusive' tag to 'exclusiveSet'
* improve parsing 'order' tag behaviour + additional coverage

Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@wozniakjan
Copy link
Member Author

wozniakjan commented May 10, 2024

/run-e2e prometheus
Update: You can check the progress 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, great job!

@zroubalik
Copy link
Member

@dttung2905 let me know once you are done with reviews, so we can merge this :)

@dttung2905
Copy link
Contributor

Its all good for me now. Thanks @zroubalik

@zroubalik zroubalik merged commit 613919b into kedacore:main May 13, 2024
20 checks passed
@SpiritZhou SpiritZhou mentioned this pull request May 21, 2024
6 tasks
@wozniakjan wozniakjan mentioned this pull request Jun 24, 2024
6 tasks
@wozniakjan wozniakjan mentioned this pull request Aug 20, 2024
7 tasks
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.

4 participants