using dedicated HTTP clients #1251

arschles · 2020-10-13T22:55:55Z

This attempts to use a dedicated, global HTTP client with a configurable global timeout for all scalers that do HTTP operations. This is a draft PR for now, but I'll update the docs too if the direction is acceptable.

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
A PR is opened to update the documentation on https://github.com/kedacore/keda-docs (see Adding docs for how to override the default HTTP client timeout keda-docs#324)
Changelog has been updated
Helm charts have been updated

Relates to #1133

Much of this was done with the help of @iennae

zroubalik · 2020-10-15T11:59:34Z

@aman-bansal PTAL^

aman-bansal · 2020-10-15T15:49:32Z

Sure I will take a look by EOD

aman-bansal · 2020-10-16T17:49:51Z

pkg/scalers/azure_blob_scaler.go

@@ -42,7 +44,7 @@ type azureBlobMetadata struct {
 var azureBlobLog = logf.Log.WithName("azure_blob_scaler")

 // NewAzureBlobScaler creates a new azureBlobScaler
-func NewAzureBlobScaler(config *ScalerConfig) (Scaler, error) {
+func NewAzureBlobScaler(httpClient *http.Client, config *ScalerConfig) (Scaler, error) {


Changing the contract for creating a new scaler seems unnecessary. I very much like the recent changes where we simplified the creation of a scaler object with ScalerConfig. It's a cleaner approach.

aman-bansal · 2020-10-16T17:52:59Z

adapter/main.go

@@ -106,9 +107,13 @@ func main() {
 cmd.Flags().AddGoFlagSet(flag.CommandLine) // make sure we get the klog flags
 cmd.Flags().IntVar(&prometheusMetricsPort, "metrics-port", 9022, "Set the port to expose prometheus metrics")
 cmd.Flags().StringVar(&prometheusMetricsPath, "metrics-path", "/metrics", "Set the path for the prometheus metrics endpoint")
+
+ var globalHTTPTimeoutMS int
+ cmd.Flags().IntVar(&globalHTTPTimeoutMS, "http-timeout", 300, "The default timeout of all HTTP requests (unless you explicitly set it otherwise)")


I don't think http-timeout is a critical parameter. It shouldn't be part of command flags.

I guess having something of say 3 sec as constant is fine. If a client wants to specify a different timeout he can simply add it to trigger metadata of config. It seems neat to me.

@zroubalik @turbaszek do you have any thoughts in this regard?

I am not 100% against this, but on the other hand, I can see your point. I honestly don't have a strict opinion on this 🤷‍♂️ I am open to hear others' ideas.

My thought process was that this would give a single timeout for all triggers. Would you rather go with a per-trigger timeout?

okay here are the possible solutions I see. I might be missing few things so do let me know if I am wrong anywhere.

How I view keda is something like this, we start keda with few flags that manages the state of keda operators and keda metric server. Each scaler manages its own state based on trigger metadata(including auth meta). I don't see any information that passes from keda to scalers as such. ScalerConfig is the example of that. Scaler config is just basic name and metadata information. Keda manages which scaler to run, after that each scaler is independent.

Now with this issue what we are trying is pass some control parameters to each scaler, and that's where the problem is we don't have any implementation regarding this (atleast i don't see anything).

So I see three different solutions for this:

Introduce global variable. Though I don't see any global variables in the code right now. This would introduce complexity while testing and all the know drawbacks of global variables.

Expand Scaler Config to have some parameters of Keda. This will help to extend more control parameters that might needed in future. But this is increasing the scope. I don't think this issue needs that much of rework. But yes something to think of and maybe build use cases around this.

Introduce a new env variable. We will pick it from the env variable. This is easy, neat and std industry practice. We could have something like this KEDA_HTTP_DEFAULT_TIMEOUT. This will do the trick.

@zroubalik @arschles What do you think?

Apologies for the delay, I had to give a talk yesterday.

@aman-bansal I agree regarding 1, so I would vote against that personally.

I think I'm on the same page as you regarding 2 and 3. Assuming we have a global timeout variable that you pass into KEDA when you start it up, you could also allow people to override that on a per-trigger basis by passing a timeout in the manifest you submit.

Specific to 3, I'm happy to change it to read from an environment variable rather than a flag. Let me know

I like the environment variable approach as well.

arschles · 2020-10-22T18:35:18Z

@aman-bansal @zroubalik I commited a change for both reading the timeout from an env var and also putting the HTTP client into the ScalerConfig (instead of changing the signatures for each scaler constructor).

One question that I still have is, in the Metrics API scaler, how should we configure the TLS transport? Currently, I have a TODO on it because if you configure it in the constructor, you'd be setting it on the global HTTP client. See https://github.com/kedacore/keda/pull/1251/files#diff-c832e36c124014f31496fdbc15f77397f48b73855a7b7441fd95525ed6112f54R82

zroubalik · 2020-10-29T17:17:16Z

@arschles I think that we should expose the env variable in Deployment yaml file (bc Helm chart is not the only installation method).

Sorry I missed the TLS transport question, do you have any particular solution in mind?

aman-bansal · 2020-10-31T08:49:23Z

I don't think we can do this. TLS config is something which is scaler specific. It needs to be manager per scaler basis. One single http.Client can't handle multiple TLS configs.
How about this for an approach:
Just like pkg/scaling/resolver/scale_resolvers.go:110#resolveEnv resolves env variables for a container and add it to the scaler config, entertain one more case where env variable is read from keda container. It will become part of scaler config. Now for every scaler you have this information available. Not just initialise each client per scaler.

arschles · 2020-11-06T21:00:28Z

@aman-bansal the metrics API scaler was the only one I saw that needs to specify its own TLS config (see my comment in that file) - would it make sense to make that a special case for now, instead of creating a client per scaler? I'm happy to do either, but the connection pooling across scalers could be beneficial. I'm not familiar with common workloads for KEDA, so I'll let you all decide whether that is truly beneficial

aman-bansal · 2020-11-09T08:05:47Z

currently we have tls support in metric api scaler and kafka scaler. There is one open PR for rabbit mq scaler.

arschles · 2020-11-10T20:17:18Z

@aman-bansal got it. I'll change this to create one client per scaler like you said then.

arschles · 2020-11-13T22:09:06Z

@aman-bansal I've updated to create a new HTTP client for each scaler, instead of sharing one across them all. PTAL 😄

Also, I saw that there's a TODO item in the OP to update the documentation. Where should I update? I saw the deployment section, does it make sense to do it there?

Finally, for the changelog TODO item, where should I update in that file? The latest release, v2.0.0 is already released

arschles · 2020-11-13T22:19:42Z

Corresponding helm chart update: kedacore/charts#90

arschles · 2020-11-13T22:59:39Z

Just fixed all the golang-ci checks. Looks like tests are broken on a context deadline exceeded error, but I cannot repro locally. am I not setting something up locally to be able to talk to rabbit?

pkg/scalers/http_clients.go

pkg/scalers/metrics_api_scaler.go

zroubalik · 2020-11-16T10:39:01Z

Just fixed all the golang-ci checks. Looks like tests are broken on a context deadline exceeded error, but I cannot repro locally. am I not setting something up locally to be able to talk to rabbit?

That was just some intermittent failure caused by the environment, I've rescheduled the checks and it is passing now.

zroubalik · 2020-11-16T12:08:13Z

Could you please update Changelog as well?

arschles · 2020-11-24T09:44:12Z

@zroubalik I've been busy working on the HTTP scaling stuff but I haven't forgotten about this. I am going to be on vacation for the rest of the week and I'll get back to this early next.

arschles · 2020-11-30T22:44:29Z

~~@zroubalik ok, just made all the changes you requested and resolved conflicts. PTAL~~

Edit: I forgot to sign a commit and then messed up rebasing, so now this PR is a mess. sorry - I'll either figure out how to fix or re-open

arschles · 2020-12-01T19:03:12Z

@zroubalik alright, I had time to give this my full attention and it should be fixed up now (I foolishly didn't rebase the first time 😦). PTAL

fixes kedacore#1133 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

zroubalik · 2020-12-07T10:44:51Z

@arschles one more small rebase and fixing the static check and I think we are good to merge! Thanks 👍

We should document the KEDA_HTTP_DEFAULT_TIMEOUT env variable somewhere though, I am thinking about this section: https://keda.sh/docs/2.0/operate/ WDYT @tomkerkhove ?

tomkerkhove · 2020-12-07T13:05:33Z

We should document the KEDA_HTTP_DEFAULT_TIMEOUT env variable somewhere though, I am thinking about this section: keda.sh/docs/2.0/operate WDYT @tomkerkhove ?

Sounds like a good spot to me indeed!

arschles · 2020-12-09T19:23:29Z

Great, I'll fix this up today and add to the docs. Thanks @zroubalik @tomkerkhove 😄

Cannot call os.Exit(1) because it prevents the klog.Flush defer Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

zroubalik · 2020-12-10T18:04:29Z

Looking good! let's merge this once the docs are out :)

kedacore/keda#1251 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles · 2020-12-10T20:21:35Z

@zroubalik @tomkerkhove see kedacore/keda-docs#324 for the docs PR

zroubalik

LGTM, thanks a lot @arschles for this!

* adding docs for the httpclient PR kedacore/keda#1251 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

aman-bansal reviewed Oct 16, 2020

View reviewed changes

arschles force-pushed the httpclient branch from 7d43842 to 50cb279 Compare October 21, 2020 19:39

arschles mentioned this pull request Oct 21, 2020

Should scalers reuse opened connection/clients? #1121

Closed

arschles force-pushed the httpclient branch 4 times, most recently from edf672f to f24a432 Compare October 26, 2020 21:35

arschles marked this pull request as ready for review October 26, 2020 21:35

arschles requested a review from ahmelsayed as a code owner October 26, 2020 21:35

arschles mentioned this pull request Oct 26, 2020

Adding the ability to set the default HTTP timeout kedacore/charts#90

Merged

zroubalik changed the base branch from v2 to main October 30, 2020 13:23

arschles mentioned this pull request Nov 6, 2020

TLS client is build with InsecureSkipVerify #1263

Closed

zroubalik reviewed Nov 16, 2020

View reviewed changes

pkg/scalers/http_clients.go Outdated Show resolved Hide resolved

pkg/scalers/metrics_api_scaler.go Outdated Show resolved Hide resolved

arschles force-pushed the httpclient branch from 3c6c185 to 429684f Compare December 1, 2020 00:09

arschles force-pushed the httpclient branch from bf54c55 to 429684f Compare December 2, 2020 18:02

using dedicated HTTP clients

bc312d7

fixes kedacore#1133 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles force-pushed the httpclient branch from 429684f to bc312d7 Compare December 4, 2020 20:10

arschles and others added 7 commits December 4, 2020 12:22

restoring mysql scaler test

8f59db4

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

Merge branch 'main' into httpclient

83610c3

tidy deps

98c6cad

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

newline at EOF

662a065

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

fixing syntax error

366d9d8

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

simplify code, bring closer to main

1cf4ef6

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

removing obsolete todo

3909d33

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles and others added 2 commits December 9, 2020 11:28

Merge branch 'main' into httpclient

d3c0d04

Fixing static check error

f969a84

Cannot call os.Exit(1) because it prevents the klog.Flush defer Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles force-pushed the httpclient branch from 88083ce to f969a84 Compare December 9, 2020 19:48

arschles added a commit to arschles/keda-docs that referenced this pull request Dec 10, 2020

adding docs for the httpclient PR

b7d7758

kedacore/keda#1251 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles mentioned this pull request Dec 10, 2020

Adding docs for how to override the default HTTP client timeout kedacore/keda-docs#324

Merged

tomkerkhove mentioned this pull request Dec 11, 2020

Provide capability to configure HTTP timeouts kedacore/charts#109

Closed

Merge branch 'main' into httpclient

3ae5d05

zroubalik approved these changes Dec 18, 2020

View reviewed changes

zroubalik merged commit e05fb91 into kedacore:main Dec 18, 2020

zroubalik mentioned this pull request Jan 4, 2021

Add OpenStack Swift scaler #1462

Merged

4 tasks

zroubalik pushed a commit to kedacore/keda-docs that referenced this pull request Jan 19, 2021

Adding docs for how to override the default HTTP client timeout (#324)

80b5404

* adding docs for the httpclient PR kedacore/keda#1251 Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles deleted the httpclient branch February 8, 2021 21:35

ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021

using dedicated HTTP clients (kedacore#1251)

7bfc6ac

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>

arschles mentioned this pull request Apr 27, 2021

Adding HTTP timeout to the IBM MQ Scaler #1758

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

using dedicated HTTP clients #1251

using dedicated HTTP clients #1251

arschles commented Oct 13, 2020 •

edited by tomkerkhove

Loading

zroubalik commented Oct 15, 2020

aman-bansal commented Oct 15, 2020

aman-bansal Oct 16, 2020

aman-bansal Oct 16, 2020 •

edited

Loading

zroubalik Oct 19, 2020 •

edited

Loading

arschles Oct 19, 2020

aman-bansal Oct 20, 2020

arschles Oct 21, 2020

zroubalik Oct 22, 2020

arschles commented Oct 22, 2020

zroubalik commented Oct 29, 2020

aman-bansal commented Oct 31, 2020

arschles commented Nov 6, 2020

aman-bansal commented Nov 9, 2020

arschles commented Nov 10, 2020

arschles commented Nov 13, 2020 •

edited

Loading

arschles commented Nov 13, 2020

arschles commented Nov 13, 2020

zroubalik commented Nov 16, 2020

zroubalik commented Nov 16, 2020

arschles commented Nov 24, 2020

arschles commented Nov 30, 2020 •

edited

Loading

arschles commented Dec 1, 2020

zroubalik commented Dec 7, 2020 •

edited

Loading

tomkerkhove commented Dec 7, 2020

arschles commented Dec 9, 2020

zroubalik commented Dec 10, 2020

arschles commented Dec 10, 2020

zroubalik left a comment

using dedicated HTTP clients #1251

using dedicated HTTP clients #1251

Conversation

arschles commented Oct 13, 2020 • edited by tomkerkhove Loading

Checklist

zroubalik commented Oct 15, 2020

aman-bansal commented Oct 15, 2020

aman-bansal Oct 16, 2020

Choose a reason for hiding this comment

aman-bansal Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

zroubalik Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

arschles Oct 19, 2020

Choose a reason for hiding this comment

aman-bansal Oct 20, 2020

Choose a reason for hiding this comment

arschles Oct 21, 2020

Choose a reason for hiding this comment

zroubalik Oct 22, 2020

Choose a reason for hiding this comment

arschles commented Oct 22, 2020

zroubalik commented Oct 29, 2020

aman-bansal commented Oct 31, 2020

arschles commented Nov 6, 2020

aman-bansal commented Nov 9, 2020

arschles commented Nov 10, 2020

arschles commented Nov 13, 2020 • edited Loading

arschles commented Nov 13, 2020

arschles commented Nov 13, 2020

zroubalik commented Nov 16, 2020

zroubalik commented Nov 16, 2020

arschles commented Nov 24, 2020

arschles commented Nov 30, 2020 • edited Loading

arschles commented Dec 1, 2020

zroubalik commented Dec 7, 2020 • edited Loading

tomkerkhove commented Dec 7, 2020

arschles commented Dec 9, 2020

zroubalik commented Dec 10, 2020

arschles commented Dec 10, 2020

zroubalik left a comment

Choose a reason for hiding this comment

arschles commented Oct 13, 2020 •

edited by tomkerkhove

Loading

aman-bansal Oct 16, 2020 •

edited

Loading

zroubalik Oct 19, 2020 •

edited

Loading

arschles commented Nov 13, 2020 •

edited

Loading

arschles commented Nov 30, 2020 •

edited

Loading

zroubalik commented Dec 7, 2020 •

edited

Loading