kafka-ch-dispatcher has no ownerref

[AceHack]

Describe the bug
kafka-ch-dispatcher gets created as needed but without a ownerref, this causes problems on uninstall of eventing as the kafka-ch-dispatcher deployment never gets cleaned up and is left around as an orphan crash loop forever.

Expected behavior
kafka-ch-dispatcher to get cleaned up on uninstall of knative Kafka channel controller.

To Reproduce
Install knative Kafka channel controller
Create a kafka channel
Delete a Kafka channel
Uninstall knative Kafka channel controller

Knative release version
0.14.1

Related issue
knative/eventing#3035

[aliok]

/assign

[aliok]

This will be only solved for the cluster scoped dispatcher.

The namespaced dispatcher won't still have an "OwnerRef" because the strategy here is to create the dispatcher in advance with an ownerRef. The namespace dispatchers need to be created later and we there's nothing that can be set as the ownerRef. If it was per channel, instead of per namespace, we would be able to set the CR instance as the owner.

Related: #1524

[aliok]

Oh boy... Can't create the dispatcher service in advance... The thing is, the type should be set to ExternalName but that requires the externalName to be set to. And in the code, we set it to fmt.Sprintf("%s.%s.svc.%s", dispatcherName, testNS, utils.GetClusterDomainName()) which cannot be done in the YAML.

Another approach would be creating the service in advance with another type and then change it in the reconciliation. But, once the service is set up like that, we cannot update the service.

@lionelvillard @slinkydeveloper @vaikas any suggestions?

BTW, there are 2 kinds of dispatchers:

1. Cluster scoped dispatchers
2. Namepaced dispatchers

This I was only attempting to fix the 1st one's ownerRef by setting it to the KnativeEventing CR in the operator.
For the 2nd one, there's nothing available as the ownerRef. Instead of dispatcher per namespace, we can actually do dispatcher per channel CR, which would allow us to set the channel CR as the ownerRef.

I need feedback.

[matzew]

sounds messy... 🤔

[slinkydeveloper]

Ok maybe it's me, but i didn't properly get what's the problem here. @aliok the dispatcher is brought up by the controller right? In both cases, could the owner ref be just the controller deployment?

[aliok]

Don't know. Could work I guess. Let me think about that.

[lionelvillard]

we tried setting the owner ref to be the controller using the downward API without success (problem with the uid if I recall well).

[aliok]

For reference:
These 2 comments explain why the controller can't be used as the owner:

• add owner ref to mtping deployment eventing#3040 (comment)
• add owner ref to mtping deployment eventing#3040 (review)
  (thanks @pierDipi for clarification)

Can't come up with any alternatives to set as the owner.

[aliok]

Oh boy... Can't create the dispatcher service in advance... The thing is, the type should be set to ExternalName but that requires the externalName to be set to. And in the code, we set it to fmt.Sprintf("%s.%s.svc.%s", dispatcherName, testNS, utils.GetClusterDomainName()) which cannot be done in the YAML.

My bad... I don't think this is accurate: externalName is not needed.
Reworking a PR now.

[aliok]

Reworked PR: #1536

[vaikas]

For the namespaced one, using the ownerref of the channel seems reasonable to me. The only caveat is that do we really want there to be 1:1 mapping between a channel and a dispatcher?
For the cluster one, if it's the problem getting the UID, can't we just ask the API server who am I and get the UID?

[aliok]

For the namespaced one, using the ownerref of the channel seems reasonable to me. The only caveat is that do we really want there to be 1:1 mapping between a channel and a dispatcher?

Won't work if there's more than 1 channel in a namespace.

For the cluster one, if it's the problem getting the UID, can't we just ask the API server who am I and get the UID?

I think this will be fine now, with the PR: #1536

[AceHack]

We have more than one channel per namespace. Will this fix my issue?

[lionelvillard]

@AceHack Are you using the hidden eventing.knative.dev/scope annotation?

[AceHack]

I didn't think it was hidden, it's called out here in the docs.
https://github.com/knative/eventing-contrib/tree/master/kafka/channel#namespace-dispatchers

And yes we are using that

[lionelvillard]

thanks @AceHack. Can I ask you a couple more questions:

• Are you also using the cluster dispatcher?
• If the answer is no, would you be ok if instead of the annotation we provide a release artifact that only includes the namespace dispatcher?

thanks.

[AceHack]

Currently, we are also using the cluster dispatcher but we could avoid it, it's not absolutely necessary. It's some more works for us to deploy the namespace dispatcher separately but something we could do.

[AceHack]

We have multiple kafka clusters and need the ability to connect to the different ones per namespace

[lionelvillard]

thanks @AceHack, this is very helpful!

[AceHack]

We actually would like to connect to different ones per topic, but we've been getting by with per namespace.

[lionelvillard]

@matzew @slinkydeveloper @lberk

[aliok]

@AceHack

We have more than one channel per namespace. Will this fix my issue?

No, in the fix PR, only the cluster scope dispatcher is created in advance.

For the namespaced ones nothing changed.

[aliok]

We actually would like to connect to different ones per topic, but we've been getting by with per namespace.

IMO we should have all these information in the CR: cluster, topic, etc