feat: add activation feature for CPU/Memory scaler #6231
Conversation
|
@JorTurFer You suggested that I query the metrics server directly instead of querying HPA. I have modified the code (#6174 (comment)). If approved, modification to the helm chart is needed since the operator's service account needs permission on pods.metrics.k8s.io APIs |
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
kunwooy
force-pushed
the
main
branch
3 times, most recently
from
c8f6e16
to
0e3f923
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kunwooy
force-pushed
the
main
branch
9 times, most recently
from
18293de
to
943fce1
Compare
|
Do you mind updating the CHANGELOG.md as well? |
|
@SpiritZhou I updated the CHANGELOG.md too. |
kunwooy
force-pushed
the
main
branch
3 times, most recently
from
2fb091a
to
df7e6f6
Compare
There was a problem hiding this comment.
Thanks, I quickly checked the code, I haven't read full the getAverage... functions.
But I would like to see more e2e tests (covering cases like scale 0->1, 1->0, activation with just a single cpu/mem scaler, etc ...to cover all possible cases).
I also think that this feature should be released first as and experimental one.
| err := config.TypedConfig(&meta) | ||
| func getScaleTarget(scalableObjectName, scalableObjectNamespace string, kubeClient client.Client) (string, string, error) { | ||
| scaledObject := &kedav1alpha1.ScaledObject{} | ||
| err := kubeClient.Get(context.Background(), types.NamespacedName{ |
There was a problem hiding this comment.
is there a specific reason to use context.Background()? If not, then we should pass ctx from the top level here.
There was a problem hiding this comment.
I used context.Background() because neither of its top level functions parseResourceMetadata() and NewCPUMemoryScaler has context. If there is any other way, I'd be happy to know
pkg/scalers/cpu_memory_scaler.go
Outdated
| func parseResourceMetadata(config *scalersconfig.ScalerConfig, logger logr.Logger) (cpuMemoryMetadata, error) { | ||
| meta := cpuMemoryMetadata{} | ||
| err := config.TypedConfig(&meta) | ||
| func getScaleTarget(scalableObjectName, scalableObjectNamespace string, kubeClient client.Client) (string, string, error) { |
There was a problem hiding this comment.
please add comment explaining this function
| default: | ||
| return meta, fmt.Errorf("unknown metric type: %s, allowed values are 'Utilization' or 'AverageValue'", string(meta.MetricType)) | ||
| } | ||
|
|
||
| if config.ScalableObjectType == "ScaledObject" { |
There was a problem hiding this comment.
we should fail for other types imho
There was a problem hiding this comment.
Are you suggesting that I do not check for config.ScalableObjectType in the current code, and instead return an error for other types? If that's so, should I parse the error string to see if the error is related to type? Because config.ScalableObjectType not being ScaledObject should not result in the error of the parseResourceMetadata() function.
|
|
||
| labelSelector = labels.SelectorFromSet(statefulSet.Spec.Selector.MatchLabels) | ||
| default: | ||
| return nil, nil, nil |
There was a problem hiding this comment.
What about other types?
See if this can be reused:
There was a problem hiding this comment.
I added GVK as the means to reference the scale target. However in the default case, there is still no way to fetch the MatchLabels field which is required to select the corresponding PodMetrics
@zroubalik I have added comments and made some changes you pointed out. However about the e2e test, you should note that this feature does not enable CPU/Memory scalers to activate itself and scale from zero because it is impossible to collect CPU/Memory data when there is no pod running. So the only difference this commit brings is the case where a CPU/Memory scaler is used with other external trigger scaler, and that external trigger is deactivated while the cpu/memory trigger is activated. When that happens, the deployment/statefulset should not scale to zero (while previously there were no activation feature for cpu/memory trigger and thus was scaled to zero). You can check out the e2e test for the above case here: Also, how do I move the feature to an experimental feature? |
kunwooy
force-pushed
the
main
branch
2 times, most recently
from
d5ad094
to
19d9a2b
Compare
Signed-off-by: kunwooy <[email protected]>
Currently, the cpu & memory scaler lacks the activation feature because it delegates the scaling responsibilities to the built-in Kubernetes HPA controller. As a result, even if the scale target is currently scaled-out by the cpu/memory metric being above the threshold value, if some other Keda scalers which use External Metrics are used in conjunction with the cpu/memory scaler, it will be deactivated (and thus scaled to zero) when all other scalers using External Metrics are deactivated.
Hence, my proposal is to introduce a way to check the activation of the cpu/memory scaler. Since the scaling behavior will be handled by the HPA controller, cpu/memory scaler only needs to feed in the activation value to the scaled object controller in its
GetMetricsAndActivity()method. Moreover to enable such feature, I introduceactivationValuefield in cpu/memory trigger's metadata.(I have accidentally closed the last pull request: #6174)
Checklist
Fixes #6057
Relates to #