Add configMap informer

[tpiperatgod]

Use client-go's informer to replace the custom routing table config map updater.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo

Fixes #287

Additional information

When the configmap named "keda-http-routing-table" changes (e.g., a new httpscaledobjects is created), the external-scaler service will request keda to access the IsActive rpc of the http-add-on in the external-push mode.

However, since the minimum resynchronization period of the informer is 1s, the IsActive rpc request launched by keda will fail in the first 1s (this has no effect on the subsequent functionality, but there will be error log output).

Should we make StreamIsActive run after a 1 second delay? or just keep it.

And according to the optimization for the external-scaler(and interceptor) mentioned at https://hackmd.io/ttpSY5KQQtGIqbAzyKex3g, it will also solve this problem I think.

Signed-off-by: laminar [email protected]

[ajanth97]

@tpiperatgod thanks for this PR. Could you please share the error logs that you came across ?

[tpiperatgod]

Sure, the error log is as follows:

{"level":"error","ts":1637142750.0702565,"logger":"IsActive","caller":"scaler/handlers.go:72","msg":"Given host was not found in queue count map","host":"myhost.com","allCounts":{},"error":"host 'myhost.com' not found in counts","stacktrace":"main.(*impl).IsActive\n\t/go/src/github.com/kedacore/http-add-on/scaler/handlers.go:72\ngithub.jparrowsec.cn/kedacore/http-add-on/proto._ExternalScaler_IsActive_Handler\n\t/go/src/github.com/kedacore/http-add-on/proto/scaler_grpc.pb.go:139\ngoogle.golang.org/grpc.(*Server).processUnaryRPC\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1279\ngoogle.golang.org/grpc.(*Server).handleStream\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1608\ngoogle.golang.org/grpc.(*Server).serveStreams.func1.2\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:923"}
{"level":"error","ts":1637142750.084912,"caller":"scaler/handlers.go:97","msg":"error getting active status in stream, continuing","error":"host 'myhost.com' not found in counts","stacktrace":"main.(*impl).StreamIsActive\n\t/go/src/github.com/kedacore/http-add-on/scaler/handlers.go:97\ngithub.jparrowsec.cn/kedacore/http-add-on/proto._ExternalScaler_StreamIsActive_Handler\n\t/go/src/github.com/kedacore/http-add-on/proto/scaler_grpc.pb.go:156\ngoogle.golang.org/grpc.(*Server).processStreamingRPC\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1532\ngoogle.golang.org/grpc.(*Server).handleStream\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1612\ngoogle.golang.org/grpc.(*Server).serveStreams.func1.2\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:923"}

[ajanth97]

@tpiperatgod thank you. I think maybe for now we could reduce the minimum resynchronization period. Ideally when the routing table gets updated it should trigger the resync, although this isn't too much of a concern right now. @arschles What do you think ?

[arschles]

@tpiperatgod this looks great! I only have one suggestion below.

Also, regarding the resynchronization period, we can fetch an initial version of the routing table on startup before we start the informer loop? That way, we'll be able to immediately report true for the IsActive call, and can watch for changes with the informer from that point onward.

Thoughts?

[tpiperatgod]

I created a channel to receive the ConfigMap object from eventHandler(addEvtHandler, updateEvtHandler, deleteEvtHandler). When eventhandler get an event, it will pass the ConfigMap object to the StartConfigMapRoutingTableUpdater. This process is now independent with the resynchronization of informer's local cache.

But it still throws the same exception. I will continue to debug it.

[tpiperatgod]

Hi folks, here is the flowchart of keda-http-add-on as I understand it, is it correct?

If this flowchart is correct, I think it is possible that SteamIsActive() was started too early causing a brief exception in the scaler service (extending the SteamIsActive() interval from 5ms to 200ms could circumvent the exception)

I also calculated the time consumption in StartConfigMapRoutingTableUpdater(), from the time the latest configmap object is received to the time updateQueueFromTable() is executed, it takes 10~60 microseconds, while the original time.ticker is 100 milliseconds, it confused me a bit.:thinking:

[arschles]

here is the flowchart of keda-http-add-on as I understand it, is it correct?

@tpiperatgod yes, I think that flowchart is correct, if I'm reading it properly.

I think it is possible that SteamIsActive() was started too early causing a brief exception in the scaler service (extending the SteamIsActive() interval from 5ms to 200ms could circumvent the exception)

Yes, I think you're right. The design of StreamIsActive is going to be updated as part of #97 (and the design document you mentioned) so this change would be temporary.

I also calculated the time consumption in StartConfigMapRoutingTableUpdater(), from the time the latest configmap object is received to the time updateQueueFromTable() is executed, it takes 10~60 microseconds

This time range is excellent.

while the original time.ticker is 100 milliseconds, it confused me a bit.🤔

The original duration was 500ms if I remember correctly. After testing a bit, we lowered it to 100 because it reduced scale-up latencies a bit but didn't put too much more load on the Kubernetes server. In reality, we could reduce it more because there won't be more than 1 scaler replica running at once. In the success case, however, this update duration doesn't matter much because changes should be applied via watch stream events. In other words, this duration is in place so that we can ensure that even if the watch stream is broken, the scaler will have an up-to-date routing table within 100ms (or whatever is configured in the KEDA_HTTP_SCALER_ROUTING_TABLE_UPDATE_DUR environment variable).

To overcome this start-up problem, what if you fetched the routing table before reporting that the scaler is healthy via the health/liveness check?

[tpiperatgod]

I think I probably know what the problem is, it's because the value of KEDA_HTTP_QUEUE_TICK_DURATION is too long. I have added a callback function to StartConfigMapRoutingTableUpdater() which, after updating the routingTable, will make a request to the interceptor service to fetch and save the current queue counts. Thus the exception above has now been resolved.

Also, I think we can deprecate the two environment variables:

• KEDA_HTTP_SCALER_ROUTING_TABLE_UPDATE_DUR in the scaler service
• KEDA_HTTP_ROUTING_TABLE_UPDATE_DURATION_MS in the interceptor service

Since we now use the informer-watch mechanism instead of the original loops.

In the meantime, I introduced the environment variable KEDA_HTTP_SCALER_CONFIG_MAP_INFORMER_RSYNC_PERIOD to set the rsync time of the informer, the default value is 60m, need to discuss if this value is appropriate.

[arschles]

@tpiperatgod that's great to hear that you tracked down the exception. I will do another review of this PR on Monday.

[arschles]

@tpiperatgod this is making really good progress. I only have a few smaller comments here

[arschles]

@tpiperatgod this looks great! I have two nit-pick comments, which we can address in a follow-up PR (in the interest of getting this merged) if you'd like. Let me know

(also, thank you for being patient on reviews. I was taking time off last week and am catching up this week)

[arschles]

@tpiperatgod the diagram you created in #326 (comment) would be very helpful to add to the project documentation. Is it ok with you if I add it? If so, I can do that.

[tpiperatgod]

@tpiperatgod the diagram you created in #326 (comment) would be very helpful to add to the project documentation. Is it ok with you if I add it? If so, I can do that.

Feel free to use it. 😄 And if you find any details that are inaccurately expressed, please let me know and I will fix it.

[arschles]

@tpiperatgod instead of fetching and saving counts, what do you think about simply adding the hostname to the scaler's in-memory map and letting the pinger fetch counts as it currently does in its own loop?

[tpiperatgod]

Can I understand your suggestion in this way:

1. pass in the pinger to StartConfigMapRoutingTableUpdater
2. then add the element with key the hostname to pinger.allCounts in the processing

If so, then I used a generic function cbFunc instead of passing in pinger directly, considering that pinger only runs in the scaler service, while StartConfigMapRoutingTableUpdater is used by both the scaler and the interceptor.

As well fetchAndSaveCounts is an already existing method, and it doesn't need to pass additional arguments, which helps make callbackWhenRoutingTableUpdate a generic function, so maybe I should think it makes sense in this scenario?

Please correct me if I have misunderstood...

[tpiperatgod]

At the same time, the pinger will keep its loop.

[tpiperatgod]

I tried passing in the pinger, but it doesn't seem to be very friendly, which means that the pinger for the scaler package needs to be imported in the routing package.

If using the in-memory map instead of pinger, I think the following error will still occur every time an HSO resource is created (although it does not affect the final result).

# You can see that q.Current() has been updated with the latest hostname
{"level":"info","ts":1638710224.7128677,"logger":"pkg.routing.StartConfigMapRoutingTableUpdater","caller":"routing/config_map_updater.go:79","msg":"updateQueueFromTable","counts":{"myhost.com":0}}

# However, IsActive will be triggered before pinger (frequently).
{"level":"error","ts":1638710224.8019505,"logger":"IsActive","caller":"scaler/handlers.go:72","msg":"Given host was not found in queue count map","host":"myhost.com","allCounts":{},"error":"host 'myhost.com' not found in counts","stacktrace":"main.(*impl).IsActive\n\t/go/src/github.com/kedacore/http-add-on/scaler/handlers.go:72\ngithub.jparrowsec.cn/kedacore/http-add-on/proto._ExternalScaler_IsActive_Handler\n\t/go/src/github.com/kedacore/http-add-on/proto/scaler_grpc.pb.go:139\ngoogle.golang.org/grpc.(*Server).processUnaryRPC\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1279\ngoogle.golang.org/grpc.(*Server).handleStream\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1608\ngoogle.golang.org/grpc.(*Server).serveStreams.func1.2\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:923"}
{"level":"error","ts":1638710224.810591,"caller":"scaler/handlers.go:97","msg":"error getting active status in stream, continuing","error":"host 'myhost.com' not found in counts","stacktrace":"main.(*impl).StreamIsActive\n\t/go/src/github.com/kedacore/http-add-on/scaler/handlers.go:97\ngithub.jparrowsec.cn/kedacore/http-add-on/proto._ExternalScaler_StreamIsActive_Handler\n\t/go/src/github.com/kedacore/http-add-on/proto/scaler_grpc.pb.go:156\ngoogle.golang.org/grpc.(*Server).processStreamingRPC\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1532\ngoogle.golang.org/grpc.(*Server).handleStream\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:1612\ngoogle.golang.org/grpc.(*Server).serveStreams.func1.2\n\t/go/pkg/mod/google.golang.org/[email protected]/server.go:923"}

[arschles]

Can I understand your suggestion in this way:

@tpiperatgod that would be one way to do it. it looks like a larger refactor might be in order, which we can certainly do in a follow-up pull request. if this works now as-is, then I'm fine with it!

for the follow-up, one possible refactor might be to have StartConfigMapRoutingTableUpdater pass more information to the callback about what was updated, so that we can then immediately reflect the updates in the pinger. for example, we could pass the watch.Event to the callback. I think we can look into this in more depth after this is merged.

[arschles]

Thanks for working through this, @tpiperatgod !