-
Notifications
You must be signed in to change notification settings - Fork 102
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
Multi-tenant interceptor and scaler #206
Conversation
@arschles when do you think this will be ready for review? |
@yaron2 this week sometime. I have to finish the routing table storage in the operator, then I can test it out, and from there I'll hit the button to make it not a draft PR. I'll for context, I just moved to a new house 600 miles away over the weekend and am getting back on my feet 😆 |
@yaron2 an update here - everything is built at this point. I have an M1 Mac and having problems (possibly qemu related) building images for amd64 architectures, so I am figuring that out and then I'll be ready to test in a cluster. Feel free to start reviewing this in the meantime if you like. The code is still rough, but it would be great to have more eyes on it sooner rather than later. |
Roger Roger. |
@arschles can you please add another to-do list item: |
I Will be responsible for the following:
If all goes right, should start working on it next Wed |
@khaosdoctor that TODO list item is in there but already checked off. would you like me to uncheck it? |
@khaosdoctor FYI I'm going to be working on adding e2e tests in this branch as well |
Oh haven't seen it! No worries, it's fine then :D |
Yep! I booked some time weekly starting next week to finish it ASAP |
@arschles I will start the new routing table strategy and merge it into your |
11899a1
to
cb9e61c
Compare
How to test this
|
15b47b4
to
b82dd44
Compare
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
b82dd44
to
43e8eff
Compare
…mponents Signed-off-by: Aaron Schlesinger <70865+arschles@users.noreply.github.com>
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.
lgtm
Reviewed offline with @arschles, looks good. |
Adding @zroubalik in case he wants to review as well |
Oh OK, nvm - Auto-merge was on. |
Huge improvement! |
🤣 |
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.
Looking good, though the PR is so huge, that I might have missed anything.
But I trust @arschles :) Great work!
thank you @tomkerkhove and @benjaminhuo @zroubalik sorry this got merged before you got a chance to look at it. if you'd like to review, please feel free to! I can make changes/fixes in follow-up PRs |
@arschles no worries, I went through the code after the merge and it was looking good :) |
Eh, unfortunately 2 pods per namespace still totally defeats the purpose of "multi-tenant scale to zero" as tenants usually live in namespaces and usually that's what is needed: scale each namespace to zero pods. |
@pkit there needs to be at least one pod running, though, to handle incoming requests to applications that are scaled to zero. or am I missing something? |
In knative, the activator pod is just for this purpose which is always on |
@arschles yup, 1 global deployment of request handlers is ok. But 1 per namespace is not that useful. |
@benjaminhuo @pkit we've scoped the interceptor to an individual namespace on purpose, because KEDA is also scoped to a single namespace. We could expand the interceptor to be cluster-global, but doing so to gain better economies of scale would only make sense if the external scaler were made cluster-global as well. @yaron2 - you raised the issue that prompted this PR in the first place. WDYT? Also, @tomkerkhove and @zroubalik WDYT as well? |
@arschles my idea was to use something other than full fledged knative installation to just have that "scale-to-zero" feature. |
@pkit not sure what you mean? |
@arschles there is no solution (other than knative) that provides "namespace scale to zero" functionality. |
Weren't we going to support both of the scenarios? I thought that was the case where you could have it cluster-wide if you want to centralize or have it namespaced if you want to isolate. We do the same with KEDA where you can deploy it cluster-wide or scoped if you want to. I think we should align, what are your thoughts @zroubalik @yaron2 ? |
got it, thanks for clarifying. stay tuned, we might be adding that functionality soon (see #206 (comment))
@tomkerkhove somebody asked about that, but we didn't go all the way to making it cluster-global. we did, however, decide to allow interceptors/scalers/operators to run in any arbitrary namespace. making them cluster-global would be different work. I wasn't aware that you could install KEDA at the cluster-wide level. can you point me to any docs on how to do that? I think if KEDA can be global, that makes it easier for the addon to do so as well. |
It's part of the helm chart/configuration - https://github.com/kedacore/charts/blob/master/keda/README.md#configuration It's called watchnamespace and is cluster-wide by default, but scopable if you need to |
👍 . Not sure how I didn't know this. I don't have the need to use KEDA across multiple namespaces much, I guess 😆 I think, then, that #240 can go on as planned (because it doesn't make much sense for KEDA to be cluster-global, but this project to not). @pkit your wish from #206 (comment) is going to be granted 😄 |
This is a large PR that makes the interceptor and external scaler multi-tenant.
See below for testing instructions.
Instead of starting up a single interceptor/scaler automatically per application, a fleet of interceptors/scalers run and can operate on any application in the same namespace. Interceptors can dynamically proxy requests based on the incoming request, scalers can dynamically report metrics on all applications, and the operator can provide routing information to the interceptor fleet. See https://hackmd.io/@arschles/mutitenant-keda-http-addon for more detailed design information.
In this pull request:
Pod
s into the cluster when anyHTTPScaledObject
is createdService
, Port, and
Deployment` of a backing applicationHTTPScaledObject
is created -- the operator pings all interceptor pods to refresh their copyThere are several follow-ups to this PR, including but not limited to:
Checklist
design.md
andwalkthrough.md
indocs/
directoryHTTPScaledObject
kedacore/charts
(feat: multi-tenant scaler and interceptor in the HTTP add-on charts#169)kedacore/charts
. Some status fields will need removing, and ahost
field will need to be added so that the operator can build the routing tableScaledObject
for interceptor. See above for details on work to make interceptor metrics availableREADME.md
docs/
directoryIsActive
method returns true always for interceptors (so that they don't scale down to 0)ConfigMap
ConfigMap
(to ensure they converge to the correct table, even if they miss events)Follow-Ups
Fixes #183
Fixes #214
Fixes #101
NOTE: since this is a large pull request, we've split up the work. To do so, we've created branches off of this branch and submitted PRs to this branch. Following are PRs that need to be merged into into this branch before this should be reviewed and merged:
cc/ @yaron2 @tomkerkhove