Adds Kafka Channel Provisioner Controllers by neosab · Pull Request #468 · knative/eventing

neosab · 2018-09-27T07:06:25Z

Related to #442

PR adds basic code for Kafka Channel Provisioner.

Note: This is based on my interpretation of the spec. Please correct me if I am wrong.

Proposed Changes

Controller for reconciling ClusterProvisioner
Controller for reconciling Channel
Provision/Deprovision topic for Channel

Limitations

Only reconciles and updates status for a single configured ClusterProvisioner
- I am not sure how to deal with multiple ClusterProvisioner that target different kafka clusters. For now the controller updates status for a configured ClusterProvisioner that targets a single kafka cluster. I am open to suggestions.
Mostly assumes ClusterProvisioner but can be improved to be generic and handle Provisioner in the future.

Release Note

grantr

I think this is the first PR for a Channel implementation in the new model! Thanks @neosab, awesome work.

I haven't reviewed the whole thing because it's WIP, but I have a few comments that might be interesting to you. Feel free to ignore.

grantr · 2018-09-27T21:23:24Z

+	}
+
+	// Skip channel provisioners that we don't manage
+	if provisioner.Name != config.Name || provisioner.Namespace != config.Namespace {


I think this filtering should be done in the Watch call in the provider. That way the informer cache doesn't even store the other provisioners locally.

Unfortunately this feature doesn't exist in controller-runtime, though it doesn't seem too hard to add. We need a variant of source.Kind that allows filtering by field values. In the meantime we can prototype using source.Informer given a filtered informer.

This will be a common pattern in these controllers so IMO it makes sense to invest in making this easy.

Thanks for the pointer. I took a quick glance, I do see the generated NewFilteredClusterProvisionerInformer, I will give this a try soon.

neosab · 2018-09-28T06:21:09Z

@grantr @adamharwayne Thanks for your quick reviews! 👍 Sorry about the late response though.

…oner

neosab · 2018-10-08T18:19:34Z

Sorry about the delay with this PR. I was on vacation 🛫 last week and just getting back to this!

…oner

neosab · 2018-10-08T22:49:46Z

I removed the logic to fetch the provisioner configmap in the controller and instead read it from a mounted volume. This way the controller does not need the cluster-wide privilege to read all config maps.

neosab · 2018-10-09T18:30:09Z

/assign grantr

n3wscott

Nice start. I would still call it WIP because you don't do any work on the channels yet.

@grantr and I have been talking about the ups and downs of using controller runtime vs meta controller and this PR is exactly what I was talking about with the boilerplate required to get controller runtime up and running. It is not super fun as the dev to implement all this and we think we have an idea to refactor some of this closer inline with how meta-controller does things while still using controller-runtime. You can see, #513, I did a lot of the same stuff (but I did not add tests yet :D)

n3wscott · 2018-10-11T03:34:55Z

+
+	// Skip channel not managed by this provisioner
+	provisionerRef := channel.Spec.Provisioner.Ref
+	clusterProvisioner, err := r.getClusterProvisioner()


this is a cool idea! I wonder if the flexibility of setting the name of the provisioner should be a argument though, what does it mean for the controller to change which provisioner it is servicing while it is running?

The idea was to support multiple provisioners not just the one in the kafka.yaml. The configmap can be updated when new provisioners are added. You are right, it is not that useful with just one provisioner. I can remove this and make the name a const if there is a strong opinion.

I removed the flexibility of the setting the provisioner name with the latest commit. I don't find a use case as of now. And this will be in line with the in-memory channel provisioner.

n3wscott · 2018-10-11T03:36:02Z

+	}
+
+	// Skip channel not managed by this provisioner
+	provisionerRef := channel.Spec.Provisioner.Ref


perhaps all of this validating the provisioner logic can be moved to a helper function called before the deepcopy has to happen.

I see that #484 also has similar logic (so would most channel provisioners) and would like to move into a common util method once we have both PR's in.

neosab · 2018-10-11T19:23:21Z

Nice start. I would still call it WIP because you don't do any work on the channels yet.

When I posted this couple of weeks back there was no reference implementation for cluster provisioners so I just wanted to test the waters by addressing the Task 1 in #442. If you wish I can implement kafka channel provisioning in this PR itself.

@grantr and I have been talking about the ups and downs of using controller runtime vs meta controller and this PR is exactly what I was talking about with the boilerplate required to get controller runtime up and running. It is not super fun as the dev to implement all this and we think we have an idea to refactor some of this closer inline with how meta-controller does things while still using controller-runtime. You can see, #513, I did a lot of the same stuff (but I did not add tests yet :D)

I agree it's a lot of boilerplate code (w/ tests) when implementing provisioners. Not too familiar with meta-controller impl, let me take a look at it to see what you intend to do.

…oner

adamharwayne · 2018-10-10T20:08:39Z

+
+type reconciler struct {
+	client     client.Client
+	restConfig *rest.Config


This looks unused.

adamharwayne · 2018-10-10T20:13:20Z

+		return nil
+	}
+
+	// Skip Channel as it is not targeting any provisioner


I think the decision of whether or not to reconcile should happen earlier. For example, when delete does something, we might not remember that it is not checking whether this controller should be controlling this channel.

My preference is to check before calling reconcile(). At least checking everything but the status of the ClusterProvisioner itself.

Makes sense, I refactored this.

…oner

evankanderson

A few comments, but this looks like it's close to ready.

evankanderson · 2018-11-07T23:56:27Z

+    ```
+    ko apply -f config/provisioners/kafka/kafka-provisioner.yaml
+    ```
+    > Note: If you are using Strimzi, you need to update the `KAFKA_BOOTSTRAP_SERVERS` value in the `kafka-channel-controller-config` ConfigMap to `my-cluster-kafka-bootstrap.kafka.9092`.


Can this be done after the ko apply using kubectl edit?

correct - will update

evankanderson · 2018-11-08T00:01:01Z

+        kind: ClusterChannelProvisioner
+        name: kafka-channel
+    ```
+1. (Optional) Install [Kail](https://github.com/boz/kail) - Kubernetes tail


@Harwayne is working on allowing a default provisioner if not specified. It might be worth a commented item here to suggest being able to set this as default.

evankanderson · 2018-11-08T00:01:36Z

+    ```
+    ko apply -f config/provisioners/kafka/kafka-provisioner.yaml
+    ```
+    > Note: If you are using Strimzi, you need to update the `KAFKA_BOOTSTRAP_SERVERS` value in the `kafka-channel-controller-config` ConfigMap to `my-cluster-kafka-bootstrap.kafka.9092`.


For non-Strimzi Kafka, are there other parameters (servers and/or credentials) that need to be configured?

just the bootstrap server - in both cases: Strimzi and no strimzi

evankanderson · 2018-11-08T00:02:14Z

+* ClusterChannelProvisioner Controller
+* Channel Controller Config Map
+
+The ClusterChannelProvisioner Controller and the Channel Controller are colocated in one Pod.


+1, I think this is a good colocation.

evankanderson · 2018-11-08T01:20:23Z

+	}
+
+	err := kafkaClusterAdmin.CreateTopic(topicName, &sarama.TopicDetail{
+		ReplicationFactor: 1,


Should this be another argument?

It could be but it wasn't in the previous ClusterBus implementation hence I didn't make it an argument. Maybe something that can be done later?

evankanderson · 2018-11-08T01:25:55Z

+		// If we didn't change anything then don't call updateStatus.
+		// This is important because the copy we loaded from the informer's
+		// cache may be stale and we don't want to overwrite a prior update
+		// to status with this stale state.


It looks like the when-to-update logic here is different than for Channel. Am I mis-reading?

You are right. Actually, I added a util method to update provisioner status in #560. I missed to use it here. I fixed it.

Actually, my bad, I do use the util method but in #573. But I also went ahead and changed it here.

…oner

googlebot · 2018-11-08T08:43:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

knative-metrics-robot · 2018-11-08T08:44:46Z

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

File	Old Coverage	New Coverage
pkg/provisioners/kafka/controller/channel/provider.go	Do not exist	0.0%
pkg/provisioners/kafka/controller/channel/reconcile.go	Do not exist	84.2%
pkg/provisioners/kafka/controller/provider.go	Do not exist	0.0%
pkg/provisioners/kafka/controller/reconcile.go	Do not exist	86.7%
pkg/provisioners/kafka/main.go	Do not exist	0.0%
pkg/provisioners/logging.go	Do not exist	0.0%

Updating docs, based on feedback

knative-metrics-robot · 2018-11-08T17:54:22Z

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

File	Old Coverage	New Coverage
pkg/provisioners/kafka/controller/channel/provider.go	Do not exist	0.0%
pkg/provisioners/kafka/controller/channel/reconcile.go	Do not exist	84.2%
pkg/provisioners/kafka/controller/provider.go	Do not exist	0.0%
pkg/provisioners/kafka/controller/reconcile.go	Do not exist	86.7%
pkg/provisioners/kafka/main.go	Do not exist	0.0%
pkg/provisioners/logging.go	Do not exist	0.0%

matzew · 2018-11-08T19:56:32Z

@evankanderson the CLA bot needs, again, some help.
not sure why he does not like two folks on one PR :-(

vaikas · 2018-11-08T20:47:52Z

/lgtm

vaikas · 2018-11-08T20:48:32Z

Please jump through the hoops and both add a comment stating that you are ok with these changes to appease the CLA manual approval process.

matzew · 2018-11-08T20:59:10Z

I am 💯 OK w/ the changes!

/lgtm

neosab · 2018-11-08T21:03:08Z

I am ok with the change too

googlebot · 2018-11-08T21:16:31Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

vaikas · 2018-11-08T21:16:40Z

ok, I'm leaving for Evan to approve, flipped the CLA bit.

matzew · 2018-11-08T22:32:39Z

we have two PRs, depending on this. so we can - and will - address issues one way or the other 😹

evankanderson

/lgtm
/approve

evankanderson · 2018-11-08T23:21:30Z

 )

+type channelArgs struct {
+	NumPartitions int32 `json:"NumPartitions,omitempty"`


(FYI) You don't need to supply a name parameter here the name is the same as the public field name.

Thanks, fixed it in #573

evankanderson · 2018-11-08T23:22:36Z

+
+	if errors.IsNotFound(err) {
+		r.logger.Info("could not find channel", zap.Any("request", request))
+		return reconcile.Result{}, nil


I'm not sure why the dependent resources (e.g. topic in Kafka) would have been deleted when the apiserver resource was deleted. Wouldn't that be the job of this controller? (In particular, the code on line 122.)

(You can fix this in #573 if it is a bug)

evankanderson · 2018-11-08T23:23:49Z

+		err = r.reconcile(newChannel)
+	} else {
+		newChannel.Status.MarkNotProvisioned("NotProvisioned", "ClusterChannelProvisioner %s is not ready", clusterChannelProvisioner.Name)
+		err = fmt.Errorf("ClusterChannelProvisioner %s is not ready", clusterChannelProvisioner.Name)


Ah, I'd missed that err below was scoped to the if block. Thanks for changing the name.

knative-prow-robot · 2018-11-08T23:32:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, neosab

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

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

Kafka Channel Provisioner Controllers

8ea7c13

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 27, 2018

knative-prow-robot requested review from grantr and vaikas September 27, 2018 07:06

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 27, 2018

grantr reviewed Sep 27, 2018

View reviewed changes

adamharwayne reviewed Sep 28, 2018

View reviewed changes

Comment thread pkg/provisioners/kafka/controller/channel/provider.go Outdated

Comment thread pkg/provisioners/kafka/controller/channel/reconcile.go

Remove controllerRuntimeStart and address PR comments

6637554

Merge remote-tracking branch 'upstream/master' into try_kafka_provisi…

0dd7028

…oner

neosab added 2 commits October 8, 2018 14:23

Merge remote-tracking branch 'upstream/master' into try_kafka_provisi…

dd502b0

…oner

Remove fetching configmap in controller

c25ce1b

neosab changed the title ~~WIP: Adds Kafka Channel Provisioner Controllers~~ Adds Kafka Channel Provisioner Controllers Oct 8, 2018

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 Oct 8, 2018

knative-prow-robot assigned grantr Oct 9, 2018

Add more tests, improv coverage

2ec153b

neosab mentioned this pull request Oct 10, 2018

Adding standard isReady method to Channel. #515

Closed

Add ChannelStatus.IsReady

4c8232d

n3wscott reviewed Oct 11, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into try_kafka_provisi…

72a219a

…oner

adamharwayne reviewed Oct 15, 2018

View reviewed changes

neosab added 3 commits October 15, 2018 13:44

Address PR comments

00ab78e

Merge remote-tracking branch 'upstream/master' into try_kafka_provisi…

5dd3164

…oner

Remove unfinished README

514773c

neosab changed the title ~~Adds Kafka Channel Provisioner Controllers~~ WIP: Adds Kafka Channel Provisioner Controllers Oct 15, 2018

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 Oct 15, 2018

neosab mentioned this pull request Nov 7, 2018

Kafka Channel Dispatcher #589

Merged

evankanderson reviewed Nov 8, 2018

View reviewed changes

neosab added 2 commits November 7, 2018 23:04

Merge remote-tracking branch 'upstream/master' into try_kafka_provisi…

32aa096

…oner

Address PR feedback

e9dad0d

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Nov 8, 2018

matzew and others added 2 commits November 8, 2018 15:49

Updating docs, based on feedback

0190820

Merge pull request #4 from matzew/doc_feedback

c301c95

Updating docs, based on feedback

knative-prow-robot assigned vaikas Nov 8, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2018

knative-prow-robot assigned matzew Nov 8, 2018

vaikas added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Nov 8, 2018

evankanderson reviewed Nov 8, 2018

View reviewed changes

knative-prow-robot assigned evankanderson Nov 8, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2018

knative-prow-robot merged commit 160c27c into knative:master Nov 9, 2018

nak3 mentioned this pull request Mar 8, 2019

Add replica config #869

Merged

Conversation

neosab commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

neosab commented Sep 28, 2018

Uh oh!

neosab commented Oct 8, 2018

Uh oh!

neosab commented Oct 8, 2018

Uh oh!

neosab commented Oct 9, 2018

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neosab commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

neosab commented Sep 27, 2018 •

edited

Loading

neosab Nov 8, 2018 •

edited

Loading