Not creating app deployments #78

arschles · 2021-03-01T21:19:19Z

Fixes #35

Previous to this PR, the operator created an application Deployment and Service. This pull request removes that functionality and requires that a user creates those things themself. In doing so, we're focusing the HTTP add on directly on:

Autoscaling
The minimum amount of traffic routing and distribution functionality needed to achieve autoscaling

This PR also changes the schema of the CRD. The new CRD needs to allow the user to specify what to scale and how to route. To achieve that, I've added a scaleTargetRef field to match the pattern that KEDA core has. It looks similar to this:

    scaleTargetRef:
        deployment: xkcd
        service: xkcd
        port: 8080

The above would instruct the operator to scale a Deployment called xkcd and route traffic through a Service called xkcd on port 8080.

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Regenerate the CRD and deepcopy code
Remove the option to create a new deployment/service - the http-add-on should only scale existing deployments

Fixes #35
Fixes #74

Fixes kedacore#35 Signed-off-by: Aaron Schlesinger <[email protected]>

Signed-off-by: Aaron Schlesinger <[email protected]>

Also generalizing test "infrastructure" code Signed-off-by: Aaron Schlesinger <[email protected]>

Signed-off-by: Aaron Schlesinger <[email protected]>

tomkerkhove

Can we add some docs please?

Users provide a scale target ref, which is the name of the deployment to scale and the service to route to. They are required to have already deployed these things already Signed-off-by: Aaron Schlesinger <[email protected]>

Signed-off-by: Aaron Schlesinger <[email protected]>

arschles · 2021-03-05T19:59:38Z

@tomkerkhove yup, documentation added here. I also removed the functionality that creates application Deployments and Services, as discussed in the March 2, 2021 community meeting.

tomkerkhove

This is looking good, thanks for making the change!

Added a few suggestions and was wondering why we are adding the xkcd Helm chart? Maybe we should leave that one out?

docs/ref/http_scaled_object.md

tomkerkhove · 2021-03-08T06:43:32Z

docs/walkthrough.md

+As said above, you need to route your HTTP traffic to the `Service` that the add on has created. If you have existing systems - like an ingress controller - you'll need to anticipate the name of these created `Service`s. Each one will be named consistently like so, in the same namespace as the `HTTPScaledObject` and your application (i.e. `$NAMESPACE`):
+
+```shell
+<deployment name>-interceptor-proxy


Let's also make the interceptor service configurable?

kind: HTTPScaledObject apiVersion: http.keda.sh/v1alpha1 metadata: name: xkcd spec: scaleTargetRef: deployment: xkcd service: xkcd port: 8080 service: name: my-interceptor-proxy port: 9999 type: LoadBalancer

Or just surface it like this:

kind: HTTPScaledObject apiVersion: http.keda.sh/v1alpha1 metadata: name: xkcd spec: scaleTargetRef: deployment: xkcd service: xkcd port: 8080 service: name: my-interceptor-proxy spec: selector: app: MyApp ports: - protocol: TCP port: 80 targetPort: 9376

Happy to open an issue if you want.

@tomkerkhove yea, can you create an issue for that? One thing I wonder is, should we not have it create a Service at all? By not doing so, we won't impose limitations on how they're configured.

We could then add to the documentation that users can create a Service with a specific label selector if they want.

What do you think @zroubalik?

I think having it part of the spec might be useful? Users and reading docs doesn't always go that well 😁

Very good point 😆. Would it be possible to merge this and then either add the service field or remove Service creation altogether in a follow up (depending on what you and @zroubalik decide)?

Also, if this helps:

In the scope doc, it says the following is in scope: Route HTTP traffic from a given source to an arbitrary HTTP server, as far as we need to efficiently accomplish (1). That could help the decision. I'd like to prefer having the operator create less things, but I'll defer to what you and @zroubalik think.

Moving to another PR is fine for me, but this feels like something we need to have to be able to work imo since this is for the interceptor.

If they have to create the service for KEDA, then it's too much infrastructure imo.

It's all about how you approach it. When I create an HTTPScaledObject you could add a field that surfaces the port + IP (if need be) so that it's all in that CRD when doing k get HTTPScaledObject and users don't even know about the service that was created. (not saying we should but a possibility).

Ok, yea I tend to agree with that @tomkerkhove . I'm happy to add the fields that surface port, IP, and maybe other fields. This PR is blocking some other work (since it makes such big fundamental changes to the operator). If there's nothing else you'd like to change in it, I'd like us to merge it and I will concurrently open a follow-up PR that adds those fields. What do you think @tomkerkhove @zroubalik ?

Fine by me!

#107 is the follow-up issue

Signed-off-by: Aaron Schlesinger <[email protected]>

zroubalik

Looking good

zroubalik · 2021-03-19T12:13:50Z

operator/api/v1alpha1/httpscaledobject_types.go

+// ScaleTargetRef contains all the details about an HTTP application to scale and route to
+type ScaleTargetRef struct {
+	// The name of the deployment to scale according to HTTP traffic
+	Deployment string `json:"deployment"`


Just a nit. Do you want to target just Deployments, what about StatefulSet or CustomResources with /scale subresoure? Might be worth checking how is this handled in ScaledObject: https://github.com/kedacore/keda/blob/6dc4c3524ba6278276cd1338cc7e4e9a7dd88963/api/v1alpha1/scaledobject_types.go#L75-L81

I would like to target Deployments for now, and open it up to other HTTP workloads that folks might want to scale in the future. Does that sound ok to you @zroubalik ?

@zroubalik I am going to merge this PR now, and we can create a follow-up if you think we need to support StatefulSet or other resources with a /scale subresource right now.

Create resource

Allowing users to scale an existing deployment

9db7295

Fixes kedacore#35 Signed-off-by: Aaron Schlesinger <[email protected]>

arschles added the enhancement New feature or request label Mar 1, 2021

arschles added this to the Alpha 2 milestone Mar 1, 2021

arschles added 3 commits March 1, 2021 14:37

checking custom deployment info

98a343c

Signed-off-by: Aaron Schlesinger <[email protected]>

Adding tests for new deployment logic

4df998e

Also generalizing test "infrastructure" code Signed-off-by: Aaron Schlesinger <[email protected]>

regenerating code

c8a3632

Signed-off-by: Aaron Schlesinger <[email protected]>

arschles marked this pull request as ready for review March 1, 2021 23:06

arschles requested review from ahmelsayed, tomkerkhove and zroubalik as code owners March 1, 2021 23:06

tomkerkhove reviewed Mar 2, 2021

View reviewed changes

arschles added 2 commits March 5, 2021 11:15

Remove functionality to auto-create deployments and services

fefe06b

Users provide a scale target ref, which is the name of the deployment to scale and the service to route to. They are required to have already deployed these things already Signed-off-by: Aaron Schlesinger <[email protected]>

Adding more docs

533bc14

Signed-off-by: Aaron Schlesinger <[email protected]>

arschles changed the title ~~Allowing users to scale an existing deployment~~ Not creating app deployments Mar 5, 2021

arschles mentioned this pull request Mar 5, 2021

Creating ingress resources when apps are created #73

Closed

5 tasks

tomkerkhove reviewed Mar 8, 2021

View reviewed changes

arschles mentioned this pull request Mar 15, 2021

Add documentation for the HTTPScaledObject schema #74

Closed

arschles and others added 7 commits March 15, 2021 16:41

merging main into this branch

ff23a4a

Signed-off-by: Aaron Schlesinger <[email protected]>

regenerating with 0.5.0 controller-gen

4e5bebe

Signed-off-by: Aaron Schlesinger <[email protected]>

Merge branch 'main' into existing-deployment

f73e9f4

fixing compile err

6463b9f

Signed-off-by: Aaron Schlesinger <[email protected]>

merging from main

55f25a7

Signed-off-by: Aaron Schlesinger <[email protected]>

Merge branch 'main' into existing-deployment

c283305

merging from main

0ae5751

Signed-off-by: Aaron Schlesinger <[email protected]>

tomkerkhove assigned zroubalik Mar 17, 2021

Merge branch 'main' into existing-deployment

1143945

arschles mentioned this pull request Mar 17, 2021

Deploy helm charts to a repository #104

Closed

1 task

tomkerkhove approved these changes Mar 18, 2021

View reviewed changes

zroubalik approved these changes Mar 19, 2021

View reviewed changes

Merge branch 'main' into existing-deployment

1acb0bc

arschles mentioned this pull request Mar 19, 2021

Add configuration parameters for operator-created services #107

Closed

1 task

arschles merged commit fd4cdfe into kedacore:main Mar 19, 2021

arschles deleted the existing-deployment branch March 19, 2021 19:58

kingdonb pushed a commit to kingdonb/http-add-on that referenced this pull request Jun 12, 2023

Merge pull request kedacore#78 from kingdon-ci/wge-create-branch-zzfwbi

8744208

Create resource

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Not creating app deployments #78

Not creating app deployments #78

arschles commented Mar 1, 2021 •

edited

Loading

tomkerkhove left a comment

arschles commented Mar 5, 2021

tomkerkhove left a comment

tomkerkhove Mar 8, 2021 •

edited

Loading

arschles Mar 15, 2021

tomkerkhove Mar 15, 2021

arschles Mar 15, 2021

tomkerkhove Mar 16, 2021

arschles Mar 18, 2021

tomkerkhove Mar 18, 2021

arschles Mar 19, 2021

zroubalik left a comment

zroubalik Mar 19, 2021

arschles Mar 19, 2021

arschles Mar 19, 2021

Not creating app deployments #78

Not creating app deployments #78

Conversation

arschles commented Mar 1, 2021 • edited Loading

Checklist

tomkerkhove left a comment

Choose a reason for hiding this comment

arschles commented Mar 5, 2021

tomkerkhove left a comment

Choose a reason for hiding this comment

tomkerkhove Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Mar 1, 2021 •

edited

Loading

tomkerkhove Mar 8, 2021 •

edited

Loading