Add configmap for flow controller by pmorie · Pull Request #324 · knative/eventing

pmorie · 2018-08-06T16:07:09Z

Replaces #240, closes #209

pmorie · 2018-08-06T16:16:19Z

I'm looking at the unit failure now

grantr

/lgtm

This seems reasonable, but given the goals of this PR, I'm curious why the watch is needed.

Here's what I understand of the goal:

During Flow reconcile, if a channel should be created, set the created channel's bus name to a default value.
If a channel already exists, never set its bus value again, even if the default changes.

IIUC the original solution for this in knative/serving was to create an informer and use its cache to get the configmap values when needed. But serving has a lot of configmaps and controllers, so it quickly became a big pain to manage all those informer declarations. The configmap package was written to wrap up the configmap informer initialization so the controller constructor can just write an event handler function to persist the change. This was a nice improvement in code size and readability.

The controller-runtime client takes a different approach, handling creation of all informers automatically and serving all Get calls out of informer caches. IMO this makes the configmap package unnecessary, since it serves the same purpose of eliminating informer declarations and allowing the configmap values to be retrieved when they're needed.

So I'm curious whether something like this would be easier to write, read, test, or maintain:

// pseudocode

const configMapKey = client.ObjectKey{
  Namespace: system.Namespace,
  Name: "flow-controller-config",
}

func (r *reconciler) getDefaultClusterBus() (string, error) {
  configMap := &corev1.ConfigMap{}
  if err := r.client.Get(context.TODO(), configMapKey, configMap); err != nil {
    return nil, err
  }

  if value, ok := configMap.Data[defaultClusterBusConfigMapKey]; ok {
    return value, nil
  }
  return "stub", nil // or return an error
}

grantr · 2018-08-06T20:36:33Z

+	controllerConfigMapWatcher configmap.Watcher
+	// defaultBusName is the default bus name to use to create channels; it is
+	// updated if the knative-system/flow-controller-config ConfigMap exists and
+	// contains the 'default-cluster-bus-name' key.


Is the key default-cluster-bus or default-cluster-bus-name?

pmorie · 2018-08-06T22:46:29Z

/retest

pmorie · 2018-08-06T23:04:14Z

/retest

🎲 🎲

pmorie · 2018-08-07T04:27:27Z

@grantr reading from an informer is acceptable here, I think. I know people have wanted this feature - how about I do a refactor in a follow-up?

pmorie · 2018-08-07T04:32:59Z

/retest

pmorie · 2018-08-07T04:37:41Z

Hrm, reading this, what you proposed now looks obviously better and more idiomatic, so I'll do it in this PR.

pmorie · 2018-08-08T19:31:17Z

@grantr feedbacks addressed, should be ready now

pmorie · 2018-08-08T19:31:35Z

/retest

grantr

/lgtm

evankanderson

/approve

evankanderson · 2018-08-08T22:11:38Z

 		},
 		Spec: channelsv1alpha1.ChannelSpec{
-			ClusterBus: defaultBusName,
+			ClusterBus: clusterBusName,


Remove defaultBusName on line 40?

evankanderson · 2018-08-08T22:12:49Z

+const (
+	// controllerConfigMapName is the name of the configmap in the eventing
+	// namespace that holds the configuration for this controller.
+	controllerConfigMapName = "flow-controller-config"


controllerAgentName + "-config" ?

evankanderson · 2018-08-08T22:13:57Z

+
+// getDefaultClusterBusName returns the value of the 'default-cluster-bus' key in
+// the knative-system/flow-controller-config configmap or an error. If the
+// 'default-cluster-bus' key is not set, it returns the default value "stub".


Is this something which should be handled through an Informer in the future (it's fine to leave this as a TODO)?

Do you need to update https://github.com/knative/eventing/blob/master/config/200-clusterrole.yaml ?

No, it turns out that we don't care about roles: https://github.com/knative/eventing/blob/master/config/201-clusterrolebinding.yaml (cluster-admin)

The controller-runtime client handles maintaining informers automatically - pretty neat huh?

knative-prow-robot · 2018-08-08T22:17:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr, pmorie

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

evankanderson · 2018-08-08T23:45:14Z

/lgtm

pmorie · 2018-08-09T14:37:48Z

/hold cancel

n3wscott · 2018-08-09T15:32:37Z

 }

 func (r *reconciler) createChannel(flow *v1alpha1.Flow) (*channelsv1alpha1.Channel, error) {
+	clusterBusName, err := r.getDefaultClusterBusName()


We should add an issue that we should make it possible for a flow to use a Bus or a ClusterBus

Yep, I'll create that issue.

Created #336

pmorie · 2018-08-09T19:40:57Z

/hold cancel

grantr · 2018-08-09T19:43:04Z

I wonder why the first /hold cancel didn't work. Maybe a failed delivery?

/poke tide

knative-metrics-robot · 2018-08-13T15:39:16Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/flow/reconcile.go	44.3%	44.9%	0.6

evankanderson

/lgtm

Someone should remove the TODO on 39 and the definition on line 40 of defaultBusName, but let's get this PR in.

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

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 Aug 6, 2018

knative-prow-robot requested review from inlined and ultrasaurus August 6, 2018 16:07

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 6, 2018

pmorie force-pushed the flow-controller-config branch 5 times, most recently from 4476026 to f49faec Compare August 6, 2018 19:50

grantr approved these changes Aug 6, 2018

View reviewed changes

knative-prow-robot assigned grantr Aug 6, 2018

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

pmorie force-pushed the flow-controller-config branch from f49faec to 441962e Compare August 6, 2018 22:08

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2018

pmorie force-pushed the flow-controller-config branch from 441962e to 434a716 Compare August 6, 2018 22:18

pmorie force-pushed the flow-controller-config branch from 434a716 to fd4fb04 Compare August 7, 2018 04:16

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2018

pmorie force-pushed the flow-controller-config branch from fd4fb04 to 2b48572 Compare August 7, 2018 15:11

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2018

pmorie force-pushed the flow-controller-config branch 2 times, most recently from acd6b98 to c89e25e Compare August 7, 2018 15:30

pmorie changed the title ~~WIP: add configmap for flow controller~~ Add configmap for flow controller Aug 7, 2018

pmorie closed this Aug 8, 2018

pmorie reopened this Aug 8, 2018

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

grantr approved these changes Aug 8, 2018

View reviewed changes

pmorie force-pushed the flow-controller-config branch from 6a58eaf to 1e9a1ac Compare August 8, 2018 22:01

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

evankanderson reviewed Aug 8, 2018

View reviewed changes

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

knative-prow-robot assigned evankanderson Aug 8, 2018

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

n3wscott reviewed Aug 9, 2018

View reviewed changes

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

Add configmap for flow controller

7ec0cbd

pmorie force-pushed the flow-controller-config branch from 1e9a1ac to 7ec0cbd Compare August 13, 2018 15:36

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2018

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

evankanderson reviewed Aug 13, 2018

View reviewed changes

knative-prow-robot merged commit 3d956ce into knative:master Aug 13, 2018

n3wscott pushed a commit to n3wscott/eventing that referenced this pull request Aug 14, 2018

Add configmap for flow controller (knative#324)

42e0ca1

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged

n3wscott mentioned this pull request Sep 25, 2018

adding n3wscott to approvers. #465

Merged

pierDipi added a commit to pierDipi/eventing that referenced this pull request Aug 31, 2023

Run make generate-release (knative#324)

d243a05

Co-authored-by: pierDipi <pierDipi@users.noreply.github.com>

Conversation

pmorie commented Aug 6, 2018

Uh oh!

pmorie commented Aug 6, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmorie commented Aug 6, 2018

Uh oh!

pmorie commented Aug 6, 2018

Uh oh!

pmorie commented Aug 7, 2018

Uh oh!

pmorie commented Aug 7, 2018

Uh oh!

pmorie commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmorie commented Aug 8, 2018

Uh oh!

pmorie commented Aug 8, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

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!

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 8, 2018

Uh oh!

evankanderson commented Aug 8, 2018

Uh oh!

pmorie commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pmorie commented Aug 9, 2018

Uh oh!

grantr commented Aug 9, 2018

Uh oh!

knative-metrics-robot commented Aug 13, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pmorie commented Aug 7, 2018 •

edited

Loading

pmorie commented Aug 9, 2018 •

edited

Loading