-
Notifications
You must be signed in to change notification settings - Fork 350
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 support for KEDA #2838
Add support for KEDA #2838
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's awesome!
Maybe a section could also be added to the existing scaling documentation.
Auto *bool `property:"auto" json:"auto,omitempty"` | ||
// Convert metadata properties to camelCase (needed because Camel K trait properties use kebab-case from command line). Disabled by default. | ||
CamelCaseConversion *bool `property:"camel-case-conversion" json:"camelCaseConversion,omitempty"` | ||
// Set the spec->replicas field on the top level controller to an explicit value if missing, to allow KEDA to recognize it as a scalable resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure you've come to the conclusion it's necessary, but that smells for an outside reader. My understand from the Keda documentation is that the only requirement for custom resources to be Keda-scalable is to declare a scale
sub-resource. Also, the path to the replicas spec is variable, and abstracted by the scale
sub-resource. Does that mean Keda access the replicas spec directly, rather than via the scale
endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to investigate.. yeah I called it "hack" because it smells :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that the error is thrown by Kube: error 500 when KEDA tries to get the scale subresource: https://github.com/kedacore/keda/blob/9ac7a0a28e446335376ef8f5e82494cf05456ecb/controllers/keda/scaledobject_controller.go#L293.
I've already seen this in the past...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible that the Integration hasn't been reconciled yet and the .status.replicas
field hasn't been set when the Keda controller reconciles the ScaledObject? If that is the case, maybe we could have it set by default earlier, either when the Integration is created, or even possibly defaulted in the CRD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the .status.replicas
field is not considered. Here's a test I ran with a fully deployed klb (with Keda trait disabled).
$ kubectl get klb sqs-to-telegram -o "jsonpath={.status.replicas}"
1
$ curl --silent http://localhost:8088/apis/camel.apache.org/v1alpha1/namespaces/test/kameletbindings/sqs-to-telegram/scale | jq
{
"kind": "Status",
"apiVersion": "v1",
"metadata": {},
"status": "Failure",
"message": "Internal error occurred: the spec replicas field \".spec.replicas\" does not exist",
"reason": "InternalError",
"details": {
"causes": [
{
"message": "the spec replicas field \".spec.replicas\" does not exist"
}
]
},
"code": 500
}
So the error comes from the API server.
Then, if I set the .spec.replicas
everything goes well:
$ kubectl get klb sqs-to-telegram -o "jsonpath={.spec.replicas}"
$ kubectl scale klb sqs-to-telegram --replicas 1
kameletbinding.camel.apache.org/sqs-to-telegram scaled
$ kubectl get klb sqs-to-telegram -o "jsonpath={.spec.replicas}"
1
$ curl --silent http://localhost:8088/apis/camel.apache.org/v1alpha1/namespaces/test/kameletbindings/sqs-to-telegram/scale | jq
{
"kind": "Scale",
"apiVersion": "autoscaling/v1",
"metadata": {
"name": "sqs-to-telegram",
"namespace": "test",
"uid": "0b93916c-72f1-47ae-b2d1-74f9e3f7e5b6",
"resourceVersion": "512151",
"creationTimestamp": "2022-01-03T22:56:18Z"
},
"spec": {
"replicas": 1
},
"status": {
"replicas": 1,
"selector": "camel.apache.org/integration=sqs-to-telegram"
}
}
May it be my version of Kube... it's not latest, but it's a recent one:
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.2", GitCommit:"8b5a19147530eaac9476b0ab82980b4088bbc1b2", GitTreeState:"clean", BuildDate:"2021-09-15T21:38:50Z", GoVersion:"go1.16.8", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.1", GitCommit:"632ed300f2c34f6d6d15ca4cef3d3c7073412212", GitTreeState:"clean", BuildDate:"2021-08-19T15:39:34Z", GoVersion:"go1.16.7", Compiler:"gc", Platform:"linux/amd64"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this might be seen as an obvious approach, but unfortunately it doesn't work this way. There are certain states where we need to compare the actual number of replicas to multiple values in ScaledObject and act based on the result. It is not just assignig one value as a replica count.
I mean, everything could be done, right? :) So maybe if we refactor to code a lot it could be done, but I am not 100% sure about this. It would definitely make the code less logical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I thought about it more and it cannot be done, there are certain KEDA features that won't work. For example there's an optional setting to reset replica count back to original number, once ScaledObject is deleted. If we don't know the number of replicas in the moment when we create the ScaledObject, we don't know the value that should be used to restore the replicas count, once ScaledObject is deleted.
And there definitely other things, that would be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry to drag you on this. I'm totally OK with KEDA mandating the .spec.replicas
field to be set. I'm still trying to understand what's the best decision on this in the general case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't know the number of replicas in the moment when we create the ScaledObject, we don't know the value that should be used to restore the replicas count, once ScaledObject is deleted.
For that case, it was not set, so it could be unset. It may be a dubious parallel, but it's like a pointer in Golang, when it's needed to differentiate between the default value for the type, and not initialised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry to drag you on this. I'm totally OK with KEDA mandating the
.spec.replicas
field to be set. I'm still trying to understand what's the best decision on this in the general case.
Yeah, no worries :) It is always great to discuss stuff, it gives you another perspective.
For that case, it was not set, so it could be unset. It may be a dubious parallel, but it's like a pointer in Golang, when it's needed to differentiate between the default value for the type, and not initialised.
But behind those numbers (pointers), there are actuall replicas of a workload that needs to be set. I am not sure what unset means in this case :)
I forgot to mention that it seems you've also fixed #1616 in the process, maybe partially though? |
Yes, iirc those cases should be covered. I'll add some other tests to verify. |
I think there's something wrong in the new applier. Maybe we can fix it in another PR, wdyt @astefanutti ? |
@nicolaferraro sure, we can figure it out later. I was about to suggest you exactly that also for the point about the scale sub-resource detection. |
Fix #1107
This adds support for KEDA autoscalers. It supports both manual configuration and fully automated configuration based on Kamelet metadata. In the fully automated scenario, Kamelets are annotated with special KEDA markers to tell the KEDA trait how to configure properties. I've added documentation in the Kamelet dev and user guide.
Everything is developed under addons. The only bits that are added to the main codebase are the roles/bindings for which a major refactoring was needed.
I've not directly pointed to the KEDA repo to import the API since they use a single module. We can ask to provide APIs in a separate module in the future. Until then, we need to sync the APIs from time to time. We only use 2 objects and I've included only fields relevant for us.
The KEDA trait allows configuring multiple triggers for an integration. In order to do it, I had to extend the CLI to support slices of complex objects. KEDA also requires that the resources being scaled have a
spec
->replicas
field, that we don't put by default. So, when the KEDA trait is enabled there's a specific hack in place that sets that field explicitly to make KEDA work.Among manual configs, the user can indicate a secret per trigger that contains authentication options, other than plain metadata.
Auto-configuration works with Kamelet. By annotating Kamelets/properties with special markers, the trait is able to map Kamelet configuration to KEDA configuration, so that a KameletBinding can automatically create all KEDA resources without additional input from the user.
I've also created some KEDA-enabled Kamelets that I'll publish when this PR gets merged.
The trick is to add something like this in the Kamelet definition:
That tells to the trait that the
topic
Kamelet property corresponds to thetopic
KEDA parameter. There are more sophisticated configuration explained in the dev guide.So when you bind e.g. a Kafka Source Kamelet to a destination, you need to add a
trait.camel.apache.org/keda.enabled=true
annotation and nothing else and you'll have autoscaling and scaling to zero from Kafka.I've also added metadata to the AWS-SQS Kamelet and plan to do a demo with both.
The new Kamelet looks like this:
While developing I did a bit of refactoring in the serverside apply part, but later I didn't use it (but kept the refactoring).
Release Note