Reconciliation for default broker selection #186

aliok · 2020-04-09T15:02:11Z

Issue to be fixed

Fixes #175

Proposed Changes

New DefaultBrokerClass in KnativeEventing CR
Make the given broker default broker

Notes:

Overwrites the default-br-config field of the config-br-defaults configmap based on the value given on CR.
If the configmap already exists, it will still be overridden. This behavior is like that for everything, not specific to this configmap. For example, if a deployment/configmap/whatever that's defined in the manifest exists, it will be overwritten by Manifestival. I didn't do anything special for this configmap.
I didn't want to introduce dependency to knative/eventing, but in the end it was super bad to duplicate the parsing logic of the configmap's data (YAML in string) and I decided to reuse some functions from knative/eventing. The functions used are specific to eventing only and cannot be really moved to knative/pkg.

Verification instructions:

Create eventing with following CR and you will see MTBroker in the created pods.

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
  name: knative-eventing
  namespace: knative-eventing
spec:
  defaultBrokerClass: MTChannelBasedBroker

With no defaultBrokerClass defined in the spec, ChannelBasedBroker is used in config-br-defaults configmap in knative-eventing namespace.

Release Note

NONE

knative-prow-robot

@aliok: 0 warnings.

In response to this:

Issue to be fixed

Fixes #175

Proposed Changes

New DefaultBrokerClass in KnativeEventing CR

Make the given broker default broker

Notes:

Overwrites the config-br-defaults configmap based on the value given on CR

TODO:

Manifest update (waiting until the changes from Move broker defaults into core eventing#2947 updates the file gs://knative-nightly/eventing/latest/eventing.yaml)

Tests

Verification instructions

Release Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aliok · 2020-04-09T15:56:50Z

Closed #184 as it didn't support custom brokers and pushed this one for early feedback.
The configmap config-br-defaults exists in manifest, eventing.yaml.
The code does a transformation for that configmap.
Sample value for the configmap is this: https://raw.githubusercontent.com/knative/eventing/2c1c5c2e2e7cbb36a716e6fca8802f71704775ff/test/config/mt-channel-broker.yaml

FYI, I simply have done string operations. Didn't want to parse the string content of the configmap's default-br-config field.

cc @jcrossley3

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

aliok · 2020-04-09T20:24:52Z

/hold

Still waiting for the gs://knative-nightly/eventing/latest/eventing.yaml to get updated.

aliok · 2020-04-10T10:32:15Z

/unhold

The nightly manifest is pushed now.

houshengbo · 2020-04-10T14:38:34Z

pkg/reconciler/knativeeventing/common/defaultbroker.go

+			}
+
+			defaultBrokerClass := instance.Spec.DefaultBrokerClass
+			if defaultBrokerClass == "" {


I suggest if channelBasedBrokerClass is empty, we can just do nothing by not changing the configmap, since configmap may have existing values.

@houshengbo I pushed some new commits and added some notes in the PR description.

To answer this concern:

If the configmap already exists, it will still be overridden. This behavior is like that for everything, not specific to this configmap. For example, if a deployment/configmap/whatever that's defined in the manifest exists, it will be overwritten by Manifestival. I didn't do anything special for this configmap.

pkg/reconciler/knativeeventing/common/defaultbroker.go

aliok · 2020-04-10T14:53:15Z

I was trying to get away with no YAML parsing, but there are valid points. I will make some changes.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/reconciler/knativeeventing/common/defaultbroker.go

aliok · 2020-04-11T19:32:28Z

@houshengbo @jcrossley3 Pushed new commits and added some notes in the PR description. Mind having another look?

matzew · 2020-04-14T09:01:19Z

@aliok can you rebase? looks like the recent dependency update conflicts your change, a bit

knative-metrics-robot · 2020-04-14T17:05:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/knativeeventing/common/defaultbroker.go	Do not exist	68.6%

aliok · 2020-04-14T17:07:06Z

Rebased

houshengbo · 2020-04-14T18:42:35Z

pkg/reconciler/knativeeventing/common/extensions_test.go

@@ -62,19 +62,19 @@ func TestTransformers(t *testing.T) {
 	results, err := platform.Transformers(kubeclient.Get(ctx), ke, logger)
 	assertEqual(t, err, nil)
 	// By default, there are 3 functions.
-	assertEqual(t, len(results), 3)
+	assertEqual(t, len(results), 4)


The comment above should change.

houshengbo · 2020-04-14T18:42:39Z

pkg/reconciler/knativeeventing/common/extensions_test.go


 	platform = append(platform, fakePlatform)
 	results, err = platform.Transformers(kubeclient.Get(ctx), ke, logger)
 	assertEqual(t, err, nil)
 	// There is one function in existing platform, so there will be 4 functions in total.
-	assertEqual(t, len(results), 4)
+	assertEqual(t, len(results), 5)


The comment above should change.

houshengbo · 2020-04-14T18:42:43Z

pkg/reconciler/knativeeventing/common/extensions_test.go


 	platformErr = append(platformErr, fakePlatformErr)
 	results, err = platformErr.Transformers(kubeclient.Get(ctx), ke, logger)
 	assertEqual(t, err.Error(), "Test Error")
 	// By default, there are 3 functions.
-	assertEqual(t, len(results), 3)
+	assertEqual(t, len(results), 4)


The comment above should change.

houshengbo

/lgtm
/approve

There are some minor nits, but I still want to accept it for now.

knative-prow-robot · 2020-04-14T18:46:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, houshengbo

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:

~~OWNERS~~ [aliok,houshengbo]

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

knative-prow-robot added the do-not-merge/work-in-progress label Apr 9, 2020

googlebot added the cla: yes label Apr 9, 2020

knative-prow-robot added the size/XXL label Apr 9, 2020

knative-prow-robot requested review from Cynocracy and n3wscott April 9, 2020 15:02

knative-prow-robot reviewed Apr 9, 2020

View reviewed changes

knative-prow-robot added the approved label Apr 9, 2020

mattmoor reviewed Apr 9, 2020

View reviewed changes

aliok marked this pull request as ready for review April 9, 2020 20:08

knative-prow-robot removed the do-not-merge/work-in-progress label Apr 9, 2020

aliok changed the title ~~Reconciliation for default broker selection~~ [WIP] Reconciliation for default broker selection Apr 9, 2020

knative-prow-robot added the do-not-merge/work-in-progress label Apr 9, 2020

knative-prow-robot added the do-not-merge/hold label Apr 9, 2020

aliok mentioned this pull request Apr 10, 2020

Update the manifest to the nightly built version #187

Closed

knative-prow-robot removed the do-not-merge/hold label Apr 10, 2020

aliok changed the title ~~[WIP] Reconciliation for default broker selection~~ Reconciliation for default broker selection Apr 10, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Apr 10, 2020

houshengbo reviewed Apr 10, 2020

View reviewed changes

pkg/reconciler/knativeeventing/common/defaultbroker.go Outdated Show resolved Hide resolved

houshengbo reviewed Apr 10, 2020

View reviewed changes

pkg/reconciler/knativeeventing/common/defaultbroker.go Outdated Show resolved Hide resolved

knative-prow-robot added the needs-rebase label Apr 11, 2020

mattmoor reviewed Apr 11, 2020

View reviewed changes

aliok force-pushed the broker-selection-on-cr branch from 5afac8e to dec9ba9 Compare April 11, 2020 18:33

knative-prow-robot removed the needs-rebase label Apr 11, 2020

mattmoor reviewed Apr 11, 2020

View reviewed changes

pkg/reconciler/knativeeventing/common/defaultbroker.go Show resolved Hide resolved

knative-prow-robot added the needs-rebase label Apr 14, 2020

aliok added 6 commits April 14, 2020 20:01

Reconciliation for default broker selection

5eabe92

Tests for default broker configmap transform

addbd38

Format things

c4e8812

Update manifest with nightly one

a12961e

Actually parse and validate the Broker defaults YAML

29804da

Import formatting

df82790

aliok force-pushed the broker-selection-on-cr branch from 84be676 to df82790 Compare April 14, 2020 17:03

knative-prow-robot removed the needs-rebase label Apr 14, 2020

houshengbo reviewed Apr 14, 2020

View reviewed changes

houshengbo approved these changes Apr 14, 2020

View reviewed changes

knative-prow-robot assigned houshengbo Apr 14, 2020

knative-prow-robot added the lgtm label Apr 14, 2020

knative-prow-robot merged commit d703711 into knative:master Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconciliation for default broker selection #186

Reconciliation for default broker selection #186

aliok commented Apr 9, 2020 •

edited

Loading

knative-prow-robot left a comment

aliok commented Apr 9, 2020

mattmoor left a comment

aliok commented Apr 9, 2020

aliok commented Apr 10, 2020

houshengbo Apr 10, 2020

aliok Apr 11, 2020 •

edited

Loading

aliok commented Apr 10, 2020

mattmoor left a comment

mattmoor left a comment

aliok commented Apr 11, 2020

matzew commented Apr 14, 2020

knative-metrics-robot commented Apr 14, 2020

aliok commented Apr 14, 2020

houshengbo Apr 14, 2020

houshengbo Apr 14, 2020

houshengbo Apr 14, 2020

houshengbo left a comment

knative-prow-robot commented Apr 14, 2020

Reconciliation for default broker selection #186

Reconciliation for default broker selection #186

Conversation

aliok commented Apr 9, 2020 • edited Loading

Issue to be fixed

Proposed Changes

Release Note

knative-prow-robot left a comment

Choose a reason for hiding this comment

Issue to be fixed

Proposed Changes

Release Note

aliok commented Apr 9, 2020

mattmoor left a comment

Choose a reason for hiding this comment

aliok commented Apr 9, 2020

aliok commented Apr 10, 2020

houshengbo Apr 10, 2020

Choose a reason for hiding this comment

aliok Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

aliok commented Apr 10, 2020

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

aliok commented Apr 11, 2020

matzew commented Apr 14, 2020

knative-metrics-robot commented Apr 14, 2020

aliok commented Apr 14, 2020

houshengbo Apr 14, 2020

Choose a reason for hiding this comment

houshengbo Apr 14, 2020

Choose a reason for hiding this comment

houshengbo Apr 14, 2020

Choose a reason for hiding this comment

houshengbo left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 14, 2020

aliok commented Apr 9, 2020 •

edited

Loading

aliok Apr 11, 2020 •

edited

Loading