KEDA should check whether scale target is already managed by another ScaledObject

[zroubalik]

Proposal

When a new ScaledObject is reconciled by Operator, it should be checked whether the scale target referenced by the new ScaledObject is being already managed by another ScaledObject, if so an error should be raised.

Use-Case

No response

Anything else?

No response

[JorTurFer]

Nice idea!
What are you thinking about? Maybe an admission controller?

[zroubalik]

As a start, an error while creating the new ScaledObject is enough imho.

[JorTurFer]

BTW, this is duplicated I think #3087

[yuvalweber]

I think you can implement this using GateKeeper policy if you would like to

[JorTurFer]

We prefer to manage it internally better than using a 3rd party software. Something like checking it during the HPA creation or an admission hook.
Are you willing to contribute? 😄

[yuvalweber]

I actually want to contribute but I think I prefer to contribute in areas that I am more familiar with 😄

[JorTurFer]

makes sense totally, don't worry :)

[JorTurFer]

I have been thinking about this, and I guess that an admission controller is the best option, because the person who deploys the SO could not be the person with access to KEDA logs. In companies with SRE teams managing the clusters, developers maybe don't have access to KEDA logs because it's in other namespace and they don't have access to check the logs. With an admission hook, we can give the feedback to those users during the deployment process.
IMO this component should be optional, WDYT? Maybe we can just add another component inside keda repo, or maybe in another split repo we can extend in the future with new features for this hook (like checking not only other SOs but also HPAs).

If we agree with the admission hook, I'm open to do a PoC to check it

[zroubalik]

+1

Admissions controller inside this repo is the best approach imho. We can then extend this with another checks (find out whether there is HPA targeting the same resource) and we can also move some validation from the controller to admission controller.

[tomkerkhove]

Given we already have a discussion on #3087 and this is a duplicate, can we merge the conversation please?

[tomkerkhove]

As per the issues on https://github.com/kedacore/keda/issues?q=is%3Aopen+label%3Aprevention+sort%3Aupdated-desc, I believe that blocking creation is the only user-friendly way as error logs will not be consumed indeed.

The resource should be prevented from being created.

@JorTurFer I see you started working on this; would you mind making sure that the approach is extensible so that the other scenarios can be easily added in the future please?

As per #3087 we discussed to make it configurable and make it an opt-in. However, I think we can make it an opt-out instead as this would reduce the support cases given the nature of using multiple SOs and how of an anti-pattern that is.

[JorTurFer]

The resource should be prevented from being created.

Exactly, the webhooks' server logs the error, but the k8s api blocks the resource creation with a clear message like:
the workload 'XXX' of type 'YYY/ZZZ' is already managed by the ScaledObject 'foo' or the workload 'XXX' of type 'YYY/ZZZ' is already managed by the hpa 'foo'

@JorTurFer I see you started working on this; would you mind making sure that the approach is extensible so that the other scenarios can be easily added in the future please?

I thought in this and that's why I split the admission hook in other (the 3rd) deployment instead of including it in the operator. We will have the webhooks' server and we can include all new validating (or mutating) webhooks in the future, just adding the new code and registering them in the cluster, but without any "major change" as now.

As per #3087 we discussed to make it configurable and make it an opt-in. However, I think we can make it an opt-out instead as this would reduce the support cases given the nature of using multiple SOs and how of an anti-pattern that is.

Using kubectl, I have to install the webhook because we use those manifest for e2e tests (and obviously, I'm covering it with e2e test), but in this case, I don't block the resource if the webhook isn't alive, only if it is ready and blocks the validation (which I think is correct, because there is an error indeed).
My idea for helm chart is to make it configurable, deployed as default in the same way as the manifest, but supporting the restrictive mode (the webhook MUST validate the resource and k8s blocks it if the webhook isn't available) and also not installing the webhook (for multi-tenant scenarios where you can only have just 1 webhook for all the tenants due to k8s webhook api is a single endpoint registration at cluster scope)

[tomkerkhove]

Sounds good to me, thanks. For Helm, I would make it on by default though since it has it's own cycle and they can just turn it off when need be.

[yuvalweber]

I have question regarding the implementation.
I looked at this feature on gatekeeper and I see that in gatekeeper you need to cache all of the hpas and scaledobject in the cluster and then you can filter only by namespace to check if there is HPA or scaledobject that points to your scaleTarget (cause both are namespaced object).

In real production environment the amount of hpas and scaledobjects can get up to thousands of objects.
How do you solve the slowliness in your implementation??

[tomkerkhove]

Would you mind moving this to the PR to keep the conversation close to that please?

[tomkerkhove]

Just to be sure; are we effectively checking this today or was it closed by accident? (I think it's fine but just checking)

[JorTurFer]

It was closed automatically because the feature is fully implemented.
Feature + Docs + Helm chart changes are merged (helm changes merge closed this)

[hanna-liashchuk]

hi,
Is there a reason why it's not allowed to have 2 or more ScaledObjects that control one Deployment/etc?
We are using keda to scale our database servers and since one Scaled object can go either UP or DOWN, we have two, because we scale up on one condition and down on another. this is not optimal in terms of resource utilization ofc, but we cannot afford to kill user connects.
and after 2.10 it doesn't work anymore

[JorTurFer]

Is there a reason why it's not allowed to have 2 or more ScaledObjects that control one Deployment/etc?

Yes, there is 😄
Under the hood, KEDA generates an HPA for each ScaledObject, and the HPA controller doesn't take care about if a deployment is scaled by different HPA. It treats each HPA as independent on each cycle and 2 HPAs could produce (and produce) cases where the HPA 1 requires X instances and the HPA 2 requires Y instances, so every 15 seconds you'd see your workload doing X->Y->wait 15 sec->X->Y->.....

In fact, Kubernetes (1.26) has added a check in HPA Controller that disables the autoscaling if the HPA detects multiple deployments (based on labels). I mean, even if we'd remove the validation, it'll fail in k8s >= 1.26 because the HPA Controller disables the HPA

Could you share your user case more detailed? Maybe there is other option to address it

[hanna-liashchuk]

sure, gladly
so we have database servers that (let's say) can handle 10 users each. we have a ScaledObject that is triggered by Prometheus metric and if the average number of connections across servers is higher than the threshold - keda scales the servers up. if there is no connection left - another scaledObject scales them down.

[JorTurFer]

I'm quite sure that both ScaledObjects requires the same replicas or you should see the workload flapping between the required replicas for ScaledObject A and the required replicas for ScaledObject B.
Could you share them? Maybe I'm missing something 🤔

[hanna-liashchuk]

Yeah, they are having the same number of target replicas. I forgot to mention one thing - The downscaling object has upscale disabled and vice versa.

…

On Wed, Jul 19, 2023, 7:36 PM Jorge Turrado Ferrero < ***@***.***> wrote: I'm quite sure that both ScaledObjects requires the same replicas or you should see the workload flapping between the required replicas for ScaledObject A and the required replicas for ScaledObject B. Could you share them? Maybe I'm missing something 🤔 — Reply to this email directly, view it on GitHub <#3755 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALNTT466VYL5F3HJJIF77H3XRAEKXANCNFSM6AAAAAARHCFPVM> . You are receiving this because you commented.Message ID: ***@***.***>

[JorTurFer]

You're right, I was missing something xD
But in any case, it won't work on kubernetes >= 1.26

In fact, Kubernetes (1.26) has added a check in HPA Controller that disables the autoscaling if the HPA detects multiple deployments (based on labels).

I don't think that we should remove the validation if the upstream doesn't roll back that feature.
For the moment, you can disable the webhooks because they are optional, there is a helm value to disable them :)

[yuvalweber]

But if you specify a threshold and minReplicas you should be good.

If for example your threshold is 10 and you minReplicas is 4 then if the threshold is not met then it will scaleDown to minimum 4 replicas but if the threshold is met than it will upload more pods.

You can use same scaledObject for both upward and downward

[hanna-liashchuk]

The problem is that there is a chance that there is a connection on the 5th server and scaling down to 4 will disconnect the user. Sure, you shouldn't remove the check, especially cause k8s no longer supports it. I'll work around the Prometheus query, maybe something can be done there. Thanks for your prompt reply :)

…

On Wed, Jul 19, 2023, 8:47 PM yuval weber ***@***.***> wrote: But if you specify a threshold and minReplicas you should be good. If for example your threshold is 10 and you minReplicas is 4 then if the threshold is not met then it will scaleDown to minimum 4 replicas but if the threshold is met than it will upload more pods. You can use same scaledObject for both upward and downward — Reply to this email directly, view it on GitHub <#3755 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALNTT47VNHTMKUENRBPFLSDXRAMTBANCNFSM6AAAAAARHCFPVM> . You are receiving this because you commented.Message ID: ***@***.***>

[yuvalweber]

If the connections of the users are not super long then you can add a preStop hook to the pod which will maybe wait for termination or will terminate the connection gracefully.

If you want you can provide a bit more details regarding your use-case and we will try to help

[tomkerkhove]

Even if HPA would support it, I don't think having multiple autoscaling definitions tell 1 app to scale is a good idea.

[yuvalweber]

I know but there are some things I would do differently.
For example it would be nice if you could configure different scaling policies for different metrics.

Cause maybe to cpu I want to bring 2 more pods at a time but for memory I want to bring 4 pods at a time

[JorTurFer]

I know but there are some things I would do differently. For example it would be nice if you could configure different scaling policies for different metrics.

Cause maybe to cpu I want to bring 2 more pods at a time but for memory I want to bring 4 pods at a time

I think that here is where KEDA could try to extend the HPA for bringing that capabilities somehow and where KEDA can add value. I mean, KEDA is who is exposing the metrics to the HPA Controller and who manages the HPA, nothing blocks that KEDA modifies the HPA based on some conditions and modify the exposed values.

I mean, IMO that's a good idea and it's something that we can try to address in KEDA side, more than going against the upstream with multiple HPAs. In fact, didn't we created an issue for adding dynamic scaling rules @tomkerkhove ?

[tomkerkhove]

Yes - #2614