Moved the internal subscriptions delivery configuration to a config map #4832

slinkydeveloper · 2021-02-03T08:56:35Z

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Fixes #4821, for more details on why: #4654

Proposed Changes

🧹 Moved the internal subscriptions delivery configuration to a config map. Note: this is a breaking change

Release Note

:broom: Moved the internal subscriptions delivery configuration to a config map

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

codecov · 2021-02-03T09:02:46Z

Codecov Report

Merging #4832 (a9e7aeb) into master (1fa597e) will decrease coverage by 0.04%.
The diff coverage is 68.85%.

@@            Coverage Diff             @@
##           master    #4832      +/-   ##
==========================================
- Coverage   81.27%   81.23%   -0.05%     
==========================================
  Files         292      293       +1     
  Lines        8309     8344      +35     
==========================================
+ Hits         6753     6778      +25     
- Misses       1146     1156      +10     
  Partials      410      410

Impacted Files	Coverage Δ
pkg/apis/sources/v1beta2/ping_types.go	`100.00% <ø> (ø)`
pkg/reconciler/mtbroker/channel_config.go	`53.33% <ø> (ø)`
pkg/reconciler/mtbroker/delivery_config.go	`30.00% <30.00%> (ø)`
pkg/adapter/apiserver/delegate.go	`88.23% <76.47%> (+10.45%)`	⬆️
pkg/adapter/apiserver/events/events.go	`94.36% <90.90%> (+0.52%)`	⬆️
pkg/adapter/mtping/runner.go	`86.27% <100.00%> (+1.79%)`	⬆️
pkg/apis/eventing/v1beta1/broker_validation.go	`86.66% <100.00%> (+2.05%)`	⬆️
pkg/apis/eventing/v1beta1/trigger_validation.go	`89.85% <100.00%> (+0.46%)`	⬆️
pkg/reconciler/mtbroker/trigger/controller.go	`78.37% <100.00%> (+1.23%)`	⬆️
pkg/reconciler/mtbroker/trigger/trigger.go	`73.07% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fa597e...9d6583a. Read the comment docs.

vaikas

I'm a bit confused about introducing a configmap and all the other stuff, could you clarify a little bit for what reason. And also, I'm confused why this is tagged as a breaking change.

config/brokers/mt-channel-broker/300-config-mtbroker-delivery.yaml

pkg/reconciler/mtbroker/testdata/config-mtbroker-delivery.yaml

vaikas · 2021-02-03T11:45:04Z

pkg/reconciler/mtbroker/trigger/trigger.go

@@ -163,7 +168,7 @@ func (r *Reconciler) subscribeToBrokerChannel(ctx context.Context, b *eventingv1
 		Name:       b.Name,
 		Namespace:  b.Namespace,
 	}
-	expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, b.Spec.Delivery)
+	expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, r.internalDeliveryConfigStore.Load())


I'm little confused why we need to add the ConfigMap here. As discussed in the spec PR, I thought the logic here would simply be (something like this):
del := t.Spec.Delivery
if del == nil {
del = b.Spec.Delivery
}
expected := resources.NewSubscription(t, brokerTrigger, brokerObjRef, uri, del)

if (t.Spec.Delivery != nil

This subscription is an internal subscription between broker components, the actual dispatch to the service is done by the filter handler.

Now, with #4654 we're saying BrokerSpec.Delivery (same for TriggerSpec.Delivery) configures the trigger delivery, hence the hop between the filter_handler and the actual service. On the other hand, this subscription is an internal hop between broker components.

I've added a config map for that to allow the user to continue to modify this internal delivery spec, because in another PR i'll actually implement in mtbroker the delivery spec as stated by #4654

Look at the description of this issue: #4515

devguyio · 2021-02-03T11:56:56Z

/assign

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

devguyio

I think it'd be great if we can have a docs issue for this? also if the release note was more explaining, something like "before, we used this field for, but now it's changing to and instead use the config map"

also, is this a breaking change? it's a different behavior, but eventing will still work and nothing will really break. I'll leave it for @vaikas and you to align on that :)

so other from those NITs, that lgtm.

pkg/reconciler/mtbroker/channel_config.go

knative-prow-robot · 2021-02-04T13:28:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio, 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:

~~OWNERS~~ [slinkydeveloper]

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

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-metrics-robot · 2021-02-11T14:03:54Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/mtbroker/channel_config.go	Do not exist	73.3%
pkg/reconciler/mtbroker/delivery_config.go	Do not exist	61.5%
pkg/reconciler/mtbroker/trigger/controller.go	66.7%	69.2%	2.6

pierDipi · 2021-02-11T15:29:32Z

config/brokers/mt-channel-broker/300-config-mtbroker-delivery.yaml

+    # Configure the internal channels subscription delivery spec
+    delivery: |
+      retry: 10


IIUC, I think we should document or warn users what this means.

If I set this retry here retry: 10 and my trigger has retry: 10 we're actually retrying at most 10 * 10 = 100 times since each time the channel implementation retries due to the delivery in this ConfigMap, the filter deployment (once implemented) will retry 10 times.

Is my understanding correct?

Exactly... Maybe we should use this "internal" configuration just for the ingress channel and then for the other channel we use the BrokerSpec.Delivery/TriggerSpec.Delivery?

The problem with that approach is that retries (in trigger.spec.delivery.retry or broker.spec.delivery.retry) are ephemeral since the filter deployment might crash while it was retrying and in that case, we're not reaching the specified retries in trigger.spec.delivery.retry or broker.spec.delivery.retry before giving up.

I see your point... I guess it makes sense, so maybe we need to just use this internal map for the first channel (the ingress one) and then use BrokerSpec.Delivery/TriggerSpec.Delivery for the filter channel?

slinkydeveloper · 2021-02-12T11:18:06Z

/hold wip

antoineco · 2021-02-12T11:31:29Z

pkg/reconciler/mtbroker/delivery_config.go

+	"knative.dev/pkg/configmap"
+	"sigs.k8s.io/yaml"
+
+	eventingduck "knative.dev/eventing/pkg/apis/duck/v1"


nit: Don't we use duckv1 everywhere else, like corev1 above?

antoineco · 2021-02-12T11:33:04Z

pkg/reconciler/mtbroker/delivery_config_test.go

+
+	eventingduck "knative.dev/eventing/pkg/apis/duck/v1"
+
+	. "knative.dev/pkg/configmap/testing"


Wherever possible it would be nice to avoid . imports. Even something short such as cmt or cmtesting helps with readability I think.

antoineco · 2021-02-12T11:35:27Z

pkg/reconciler/mtbroker/trigger/controller_test.go

 	// Fake injection informers
 	_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/broker/fake"
 	_ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/trigger/fake"
 	_ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/subscription/fake"
+	"knative.dev/eventing/pkg/reconciler/mtbroker"


This import should be relocated, it is not a "Fake injection informer" so future readers will be confused.

antoineco · 2021-02-12T11:37:41Z

pkg/reconciler/mtbroker/delivery_config.go

+	InternalDeliveryConfigMapName        = "mt-broker-internal-delivery"
+	internalDeliveryConfigMapDeliveryKey = "delivery"


Any reason for making the name public but the key private? Can't they both be private?

antoineco · 2021-02-12T11:39:54Z

pkg/reconciler/mtbroker/trigger/controller_test.go

+	cmw := configmap.NewStaticWatcher(&corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      mtbroker.InternalDeliveryConfigMapName,
+			Namespace: system.Namespace(),
+		},


Good candidate for rectesting.NewConfigMap I think.

antoineco · 2021-02-12T11:45:19Z

pkg/reconciler/mtbroker/delivery_config.go

+}
+
+// InternalDeliveryConfigStore is a typed wrapper around configmap.Untyped store to handle our configmaps.
+// +k8s:deepcopy-gen=false


Is that annotation needed? As far as I can see we don't run deepcopy-gen on that package.

antoineco · 2021-02-12T11:49:04Z

pkg/reconciler/mtbroker/delivery_config.go

+			"channeldefaults",
+			logger,
+			configmap.Constructors{
+				InternalDeliveryConfigMapName: NewInternalDeliveryConfigFromConfigMap,


I see that NewInternalDeliveryConfigFromConfigMap can return nil but I don't see a check to fallback to a default when this is the case. Maybe I overlooked, just want to be sure you've ensured this can not cause a panic.

slinkydeveloper · 2021-02-12T13:29:09Z

/close

After reasoning about it and the #4832 (comment) comment, i believe this pr doesn't make sense

knative-prow-robot · 2021-02-12T13:29:14Z

@slinkydeveloper: Closed this PR.

In response to this:

/close

After reasoning about it and the #4832 (comment) comment, i believe this pr doesn't make sense

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.

Moved the internal subscriptions delivery configuration to a config map

5f81887

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 3, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 3, 2021

knative-prow-robot requested review from aliok and tommyreddad February 3, 2021 08:56

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2021

slinkydeveloper mentioned this pull request Feb 3, 2021

Implement TriggerSpec.Delivery for mtbroker #4833

Closed

slinkydeveloper added 2 commits February 3, 2021 10:00

Copyright

d0faeb3

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Newline

100a8e4

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

vaikas reviewed Feb 3, 2021

View reviewed changes

devguyio self-requested a review February 3, 2021 11:56

knative-prow-robot assigned devguyio Feb 3, 2021

Copyright

ff3a2b2

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

devguyio approved these changes Feb 4, 2021

View reviewed changes

pkg/reconciler/mtbroker/channel_config.go Show resolved Hide resolved

devguyio mentioned this pull request Feb 4, 2021

Add devguyio to approvers #4700

Closed

Sort imports

c08416b

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper mentioned this pull request Feb 11, 2021

Implement the DeliverySpec in trigger filter dispatcher #4873

Closed

slinkydeveloper added 3 commits February 11, 2021 14:37

Fix newline

e330be3

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Fix newline

933c048

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Skipping test because it requires another impl

9d6583a

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Feb 11, 2021

pierDipi reviewed Feb 11, 2021

View reviewed changes

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

antoineco reviewed Feb 12, 2021

View reviewed changes

knative-prow-robot closed this Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved the internal subscriptions delivery configuration to a config map #4832

Moved the internal subscriptions delivery configuration to a config map #4832

slinkydeveloper commented Feb 3, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

vaikas left a comment

vaikas Feb 3, 2021

slinkydeveloper Feb 3, 2021 •

edited

Loading

slinkydeveloper Feb 4, 2021

devguyio commented Feb 3, 2021

devguyio left a comment

knative-prow-robot commented Feb 4, 2021

knative-metrics-robot commented Feb 11, 2021

pierDipi Feb 11, 2021

slinkydeveloper Feb 11, 2021

pierDipi Feb 12, 2021 •

edited

Loading

slinkydeveloper Feb 12, 2021

slinkydeveloper commented Feb 12, 2021

antoineco Feb 12, 2021

antoineco Feb 12, 2021

antoineco Feb 12, 2021

antoineco Feb 12, 2021

antoineco Feb 12, 2021

antoineco Feb 12, 2021 •

edited

Loading

antoineco Feb 12, 2021

slinkydeveloper commented Feb 12, 2021

knative-prow-robot commented Feb 12, 2021


		eventingduck "knative.dev/eventing/pkg/apis/duck/v1"

		. "knative.dev/pkg/configmap/testing"

		InternalDeliveryConfigMapName = "mt-broker-internal-delivery"
		internalDeliveryConfigMapDeliveryKey = "delivery"

Moved the internal subscriptions delivery configuration to a config map #4832

Moved the internal subscriptions delivery configuration to a config map #4832

Conversation

slinkydeveloper commented Feb 3, 2021

Proposed Changes

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devguyio commented Feb 3, 2021

devguyio left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 4, 2021

knative-metrics-robot commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoineco Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slinkydeveloper commented Feb 12, 2021

knative-prow-robot commented Feb 12, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

slinkydeveloper Feb 3, 2021 •

edited

Loading

pierDipi Feb 12, 2021 •

edited

Loading

antoineco Feb 12, 2021 •

edited

Loading