Fix Kafka Channel dispatcher ownerRef #1536

aliok · 2020-09-09T11:46:54Z

Proposed Changes

Create the dispatcher deployment and service in advance
Deployment has 0 replicas initially and will be scaled up when the first KafkaChannel is created
Existing reconcilation logic is kept as it is harmless
Also tested with namespaced channels

Release Note

🐛 Fix bug
Cluster scoped KafkaChannel dispatcher is now created with 0 replicas in advance and will be scaled up with the creation of the first KafkaChannel. No manual operation is needed when upgrading.

Docs

aliok · 2020-09-09T12:48:05Z

cc @lionelvillard

slinkydeveloper · 2020-09-09T13:08:53Z

This approach makes sense to me 👍

/lgtm
/approve
/hold waiting for @lionelvillard approval

kafka/channel/pkg/reconciler/controller/kafkachannel.go

kafka/channel/pkg/dispatcher/dispatcher.go

aliok · 2020-09-09T14:19:49Z

kafka/channel/pkg/utils/util.go

@@ -89,3 +91,13 @@ func TopicName(separator, namespace, name string) string {
 	topic := []string{knativeKafkaTopicPrefix, namespace, name}
 	return strings.Join(topic, separator)
 }
+
+func FindContainer(d *appsv1.Deployment, containerName string) *corev1.Container {


I couldn't find a function like this in the codebase. Any pointers?

same. I ended-up implemented it. https://github.com/knative/eventing/blob/master/pkg/reconciler/pingsource/pingsource.go#L198

/hold
Ok, looks like I need to write some unit tests for this function I created...

/unhold

Done with unit tests.
Next time I need this, I will refactor it into a function in knative/pkg

kafka/channel/config/500-kafka-ch-dispatcher.yaml

matzew · 2020-09-09T14:48:43Z

kafka/channel/pkg/reconciler/controller/kafkachannel.go

+			d, err := r.KubeClientSet.AppsV1().Deployments(dispatcherNamespace).Update(expected)
+			if err == nil {
+				controller.GetEventRecorder(ctx).Event(kc, corev1.EventTypeNormal, dispatcherDeploymentUpdated, "Dispatcher deployment updated")
+				kc.Status.PropagateDispatcherStatus(&d.Status)
+				return d, nil
+			} else {
+				kc.Status.MarkServiceFailed("DispatcherDeploymentUpdateFailed", "Failed to update the dispatcher deployment: %v", err)
+			}
+			return d, newDeploymentWarn(err)


Isn't that like: https://github.com/knative/eventing-contrib/pull/1536/files#diff-c0164358c6e61a5abb38a4914a1dc150R332-R341

Pushed a new commit and made it easier to understand now.
Looks more similar to old code: https://github.com/knative/eventing-contrib/blob/master/kafka/channel/pkg/reconciler/controller/kafkachannel.go#L307

n3wscott · 2020-09-09T15:05:36Z

/lgtm

This sets us up for different kinds of deployments as the dispatcher as well in the future with a bit more work.

aliok · 2020-09-09T16:01:03Z

~~Code changes to address comments are not unit tested yet. Let me do them first.~~
UPDATE: done

knative-metrics-robot · 2020-09-09T16:54:17Z

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
kafka/channel/pkg/reconciler/controller/kafkachannel.go	53.9%	54.2%	0.3
kafka/channel/pkg/utils/util.go	88.2%	90.5%	2.2

aliok · 2020-09-09T18:45:42Z

@matzew @lionelvillard
This is all green now, ready for another look

lionelvillard · 2020-09-09T20:51:27Z

kafka/channel/pkg/reconciler/controller/kafkachannel.go

+		}
+
+		if *d.Spec.Replicas == 0 {
+			logging.FromContext(ctx).Infof("Dispatcher deployment has 0 replicas. Scaling up deployment to 1 replicas")


nit: replicas -> replica, both instances.

matzew

/lgtm
/approve

knative-prow-robot · 2020-09-10T05:25:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, matzew, slinkydeveloper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~kafka/OWNERS~~ [matzew,slinkydeveloper]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Fix Kafka Channel dispatcher ownerRef

09e6273

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 9, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2020

knative-prow-robot requested review from lberk and matzew September 9, 2020 11:47

aliok changed the title ~~[WIP] Fix Kafka Channel dispatcher ownerRef~~ Fix Kafka Channel dispatcher ownerRef Sep 9, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2020

aliok mentioned this pull request Sep 9, 2020

kafka-ch-dispatcher has no ownerref #1188

Closed

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2020

knative-prow-robot assigned slinkydeveloper Sep 9, 2020

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 9, 2020

lionelvillard reviewed Sep 9, 2020

View reviewed changes

kafka/channel/pkg/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

lionelvillard reviewed Sep 9, 2020

View reviewed changes

kafka/channel/pkg/reconciler/controller/kafkachannel.go Outdated Show resolved Hide resolved

Fix Kafka Channel dispatcher ownerRef

811ad7a

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020

aliok commented Sep 9, 2020

View reviewed changes

kafka/channel/pkg/dispatcher/dispatcher.go Outdated Show resolved Hide resolved

aliok commented Sep 9, 2020

View reviewed changes

Fix Kafka Channel dispatcher ownerRef

9bb36be

matzew reviewed Sep 9, 2020

View reviewed changes

kafka/channel/config/500-kafka-ch-dispatcher.yaml Outdated Show resolved Hide resolved

matzew reviewed Sep 9, 2020

View reviewed changes

kafka/channel/config/500-kafka-ch-dispatcher.yaml Outdated Show resolved Hide resolved

Addressing comments

59e3a1f

matzew reviewed Sep 9, 2020

View reviewed changes

knative-prow-robot assigned n3wscott Sep 9, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020

Make more readable

08a4156

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020

aliok added 2 commits September 9, 2020 19:33

Unit tests for scale up

38aba44

Unit tests for findContainer

e655f90

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2020

lionelvillard reviewed Sep 9, 2020

View reviewed changes

matzew approved these changes Sep 10, 2020

View reviewed changes

knative-prow-robot assigned matzew Sep 10, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2020

knative-prow-robot merged commit 1fcaa7b into knative:master Sep 10, 2020

matzew mentioned this pull request Sep 10, 2020

Fixing typo #1541

Merged

aliok deleted the fix-kafka-ch-dispatcher-ownerref2 branch September 14, 2020 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Kafka Channel dispatcher ownerRef #1536

Fix Kafka Channel dispatcher ownerRef #1536

aliok commented Sep 9, 2020 •

edited

Loading

aliok commented Sep 9, 2020

slinkydeveloper commented Sep 9, 2020

aliok Sep 9, 2020

matzew Sep 9, 2020

lionelvillard Sep 9, 2020

aliok Sep 9, 2020 •

edited

Loading

aliok Sep 9, 2020

matzew Sep 9, 2020

aliok Sep 9, 2020

n3wscott commented Sep 9, 2020

aliok commented Sep 9, 2020 •

edited

Loading

knative-metrics-robot commented Sep 9, 2020

aliok commented Sep 9, 2020

lionelvillard Sep 9, 2020

matzew left a comment

knative-prow-robot commented Sep 10, 2020

Fix Kafka Channel dispatcher ownerRef #1536

Fix Kafka Channel dispatcher ownerRef #1536

Conversation

aliok commented Sep 9, 2020 • edited Loading

Proposed Changes

aliok commented Sep 9, 2020

slinkydeveloper commented Sep 9, 2020

aliok Sep 9, 2020

Choose a reason for hiding this comment

matzew Sep 9, 2020

Choose a reason for hiding this comment

lionelvillard Sep 9, 2020

Choose a reason for hiding this comment

aliok Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

aliok Sep 9, 2020

Choose a reason for hiding this comment

matzew Sep 9, 2020

Choose a reason for hiding this comment

aliok Sep 9, 2020

Choose a reason for hiding this comment

n3wscott commented Sep 9, 2020

aliok commented Sep 9, 2020 • edited Loading

knative-metrics-robot commented Sep 9, 2020

aliok commented Sep 9, 2020

lionelvillard Sep 9, 2020

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 10, 2020

aliok commented Sep 9, 2020 •

edited

Loading

aliok Sep 9, 2020 •

edited

Loading

aliok commented Sep 9, 2020 •

edited

Loading