Add PushScaler interface and impl external-push scaler

[ahmelsayed]

Closes #820

Signed-off-by: Ahmed ElSayed ahmels@microsoft.com

This PR introduces

type PushScaler interface {
	Scaler

	// Run is the only writer to the active channel and must close it once done.
	Run(ctx context.Context, active chan<- bool)
}

with 1 implementation in an external-push scaler with the following grpc call

service ExternalScaler {
  rpc StreamIsActive(ScaledObjectRef) returns (stream IsActiveResponse) {}
}

ExternalScaler also keeps a pool of GRPC connections per-metadata hash. So if there are multiple scaledObjects all pointing to the same grpc endpoint with the same metadata, they will all use the same grpc.ClientConn with separate grpc.Client per-scaledObject.

This PR also removes the following methods from the external scaler interface:

service ExternalScaler {
-    rpc New(NewRequest) returns (google.protobuf.Empty) {}
-    rpc Close(ScaledObjectRef) returns (google.protobuf.Empty) {}
}

This is a breaking change for external scaler.

Checklist

• [ pending ] A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
• Commits are signed with Developer Certificate of Origin (DCO)
• Tests have been added

Fixes #820

[tomkerkhove]

Might be a stupid question but is there a reason why this impacts the external scaler spec? How I understood it it was going to be a new “push” scaler built in the core.

[tomkerkhove]

Is this downgrade on purpose?

[zroubalik]

LGTM

[ahmelsayed]

Might be a stupid question but is there a reason why this impacts the external scaler spec? How I understood it it was going to be a new “push” scaler built in the core.

@tomkerkhove It's a really good question. It didn't have to alter the existing scaler, I just meant to add a completely new scaler, external-push, but since a bunch of grpc plumbing code will be shared between then I was taking a look at the existing scaler, and thought of few improvements to it.

This PR can be thought of as 2 parts:

1. all files under pkg/scaling/* (and the interface in pkg/scalers/scaler.go) are the "core" part of it. This change allows a scaler to implement a method where they'll get a write channel to an active state anytime.

2. all the external scaler changes under pkg/scalers/* are adding support for external-push and making changes to the existing external scaler.

The reason to change external scaler was just to clean up the contract a bit. The current contract is this:

service ExternalScaler {
    rpc New(NewRequest) returns (google.protobuf.Empty) {}
    rpc IsActive(ScaledObjectRef) returns (IsActiveResponse) {}
    rpc GetMetricSpec(ScaledObjectRef) returns (GetMetricSpecResponse) {}
    rpc GetMetrics(GetMetricsRequest) returns (GetMetricsResponse) {}
    rpc Close(ScaledObjectRef) returns (google.protobuf.Empty) {}
}

When I was implementing external scalers for testing the contract for "new" and "close" as GRPC calls is very awkward/unclear. In fact both the example external scalers we have don't do anything on Close (durable and artemis) and durable also just logs the request on New. Artemis uses new to store some global metadata, but this locks it to only 1 scaledObject. Handling this on per invocation for IsActive is a more flexible and doesn't require every implementation to handle state. This PR adds IsActiveStreaming that allows the implementation to return isActive: true any time.

[tomkerkhove]

@ahmelsayed Thanks for the info! So if you build an external scaler, we don't mandate it to support pull & push but merely just give the contract or am I understanding it wrong (sorry, have to dig deeper on this topic).

Just want to avoid that it's becoming more complex to write an external scaler while we don't need the complexity.

[zroubalik]

@ahmelsayed Thanks for the info! So if you build an external scaler, we don't mandate it to support pull & push but merely just give the contract or am I understanding it wrong (sorry, have to dig deeper on this topic).

@tomkerkhove Yeah, that's correct. See there are 2 types of External scaler:

keda/pkg/scaling/scale_handler.go

Lines 353 to 356 in 5996879

	case "external":
	return scalers.NewExternalScaler(name, namespace, triggerMetadata)
	case "external-push":
	return scalers.NewExternalPushScaler(name, namespace, triggerMetadata)

[tomkerkhove]

[ahmelsayed]

btw, is there a place we're tracking breaking changes in v2? I can add a couple of lines to the CHANGELOG for this.

[zroubalik]

btw, is there a place we're tracking breaking changes in v2? I can add a couple of lines to the CHANGELOG for this.

@ahmelsayed that's right. We don't have anything atm, the same goes for docs. Start the v2 section in Changelog with this PR and I'll add the other changes then.

[tomkerkhove]

Since it has not been shipped yet, would it make sense to use "Upcoming" or move the WIP to a Google Doc?

[zroubalik]

@tomkerkhove the changelog will be in the v2 branch, so it will not interfere with master branch.

[tomkerkhove]

Good point, sounds good to me 👍

[zroubalik]

@ahmelsayed are going to add the Changelog as part of this PR or do you want to create another? If it is the latter option we can merge this PR.

[ahmelsayed]

Sorry for the delay, I was out the last couple of weeks. I added a line to the CHANGELOG for this breaking change. We can add another PR for other breaking changes in v2.

I also opened a docs PR here kedacore/keda-docs#193