Fights service shouldn't use client side load balancing when deployed in knative

[edeandrea]

The fight service shouldn't use Stork's client-side load balancing when deployed in KNative. This breaks because if the heroes/villains services scale to 0, then the client side load balancing in the fights service won't have any downstream clients.

[edeandrea]

This one actually turns out to be quite difficult to solve. I had a conversation with @cescoffier today and will add the conversation here so it can be tracked and/or referenced.

The fight service does not work because the configuration of the heroes/villains services isn't correct.

Currently the fight service uses Stork and the kubernetes discovery to discover the URLs, but that won't work if the services are scaled down to 0.

I tried to change it to use a static list, but in order to do that you can't just use the service name (like quarkus.stork.hero-service.service-discovery.address-list: rest-heroes), even though the service is in the same namespace.

In KNative you need to use the fully-qualified URL, like quarkus.stork.hero-service.service-discovery.address-list: rest-heroes.<namespace-name>.svc.cluster.local

The problem is that with the k8s descriptors we provide, we don't/can't make any assumptions about the name of the namespace. This means that if a user wanted to kubectl apply -f https://raw.githubusercontent.com/quarkusio/quarkus-super-heroes/main/deploy/k8s/java17-knative.yml they'd first have to go into the yaml, find the rest-fights knative service, and then update it.

Another alternative I tried was to use the Kubernetes downward API and inject the URLs directly into the service as an environment variable, like

env:
  - name: POD_NAMESPACE
    valueFrom:
      fieldRef:
        fieldPath: metadata.namespace
  - name: QUARKUS_STORK_HERO_SERVICE_SERVICE_DISCOVERY_ADDRESS_LIST
    value: "rest-heroes.$(POD_NAMESPACE).svc.cluster.local"
  - name: QUARKUS_STORK_VILLAIN_SERVICE_SERVICE_DISCOVERY_ADDRESS_LIST
    value: "rest-villains.$(POD_NAMESPACE).svc.cluster.local"

but that doesn't work either, because by default knative does not allow use of the downward API. You have to specifically enable it by setting a flag in the config-features ConfigMap in the knative-serving namespace, which again I can't assume a user has permission to do (i.e. Red Hat dev sandbox). You need cluster-admin permissions to do that.

So I'm not quite sure how to solve this problem, at least not without requiring the user to make a manual edit to the yaml before applying it.

Alternative 1

I think what we need is something in Stork for knative service discovery. That way we could configure it like

quarkus.stork.hero-service.service-discovery.type: knative
quarkus.stork.hero-service.service-discovery.application: rest-heroes

and under the covers Stork would construct the URL rest-heroes.<namespace>.svc.cluster.local. Stork would be able to determine the name of the namespace on its own. There wouldn't be any client-side load balancing of course, but does that even make sense in knative if pods can scale to 0 at any time?

Alternative 2

In this alternative we wouldn't deploy the fights service as a knative service, but instead just as a regular Deployment. In this case we would have to use the downward API, since the heroes & villains services would still be knative services and would still need to construct a fully-scoped URL. We wouldn't have the permissions issue on the downward API though.

@cescoffier / @agoncal / @tqvarnst / @aureamunoz I'm open to other ideas/suggestions.

[cescoffier]

@edeandrea Thanks for this issue.

@aureamunoz, we would need a new service discovery implementation in Stork for KNative. This SD locates the KNative service (and not the pods as we do for Kubernetes). It would not be possible to use load balancing. However, if you use KNative, load balancing can be handled there.

This SD would take a (Knative) service name as parameter and search for it in the current (or configured) namespace. It will not check if there are pods, just locate the service, resolve the name and return that name. It fails if no service can be found. You will never have more than one service (this needs to be verified when using revisions).

[aureamunoz]

Hi!
Ok, will open an issue in the Stork side to implement the new SD.

[edeandrea]

For now, knative is broken in the fights service. We need to wait for smallrye/smallrye-stork#402 is implemented, and then trickles into a Quarkus version.

[edeandrea]

Thanks @geoand ! I'm going to re-open this though because it isn't actually fixed yet in this repo until its updated to Quarkus 2.16...

[geoand]

💪🏼

[edeandrea]

Just updating here - this still seems to be broken after quarkusio/quarkus#30272

See smallrye/smallrye-stork#488

[edeandrea]

This is waiting on quarkusio/quarkus#31617, which should be available in Quarkus 2.16.5.Final.