Allow registering custom conditions for the Broker

[pierDipi]

Fixes #3094

Proposed Changes

• Allow registering custom conditions for the Broker

[pierDipi]

/cc @vaikas @grantr
I would like to know your opinions on the approach.

[knative-test-reporter-robot]

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-upgrade-tests	pull-knative-eventing-upgrade-tests pull-knative-eventing-upgrade-tests pull-knative-eventing-upgrade-tests pull-knative-eventing-upgrade-tests	3/3

Job pull-knative-eventing-upgrade-tests expended all 3 retries without success.

[vaikas]

Thanks for taking this on!! I understood we were going to just expose the simpler solution as described here:
#3094

I'd like to first focus on moving mt broker to use this so there's no special casing for that Broker and that also validates the model as well as makes it a good reference implementation.
What do y'all think?

[knative-metrics-robot]

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/apis/eventing/v1/broker_lifecycle.go	95.5%	100.0%	4.5
pkg/apis/eventing/v1/broker_lifecycle_mt.go	Do not exist	100.0%
pkg/apis/eventing/v1beta1/broker_lifecycle.go	95.5%	100.0%	4.5
pkg/apis/eventing/v1beta1/broker_lifecycle_mt.go	Do not exist	100.0%
pkg/reconciler/mtbroker/controller.go	65.7%	66.7%	1.0

[vaikas]

I'm sorry I must have misunderstood what the single broker proposal was. In my mind I had been thinking that we'd be converting the class variable to instance variable, something like this (obvs not necessarily the best place)? And hence each Broker instance would have the conditionset that it uses. But now I'm wondering if your use of the annotation to store which condition set data is really the only way to achieve this without changing the actual API shape visible to the user. Hm... @grantr @dprotaso thoughts on which way is better.

Does it actually need to be persisted anywhere? (sorry, been a long week already :) )

diff --git a/pkg/apis/eventing/v1/broker_types.go b/pkg/apis/eventing/v1/broker_types.go
index 7771fc866..b238151e1 100644
--- a/pkg/apis/eventing/v1/broker_types.go
+++ b/pkg/apis/eventing/v1/broker_types.go
@@ -48,6 +48,10 @@ type Broker struct {
        // date.
        // +optional
        Status BrokerStatus `json:"status,omitempty"`
+        // not visible to the user, only injected to reconciler
+       // +optional
+       BrokerCondSet *apis.ConditionSet
 }

[vaikas]

I had actually been thinking about moving them away from our API directory alltogether and into the pkg/reconciler. Does not have to be part of this PR obviously :) But again, just sharing my thoughts.

[vaikas]

Instead of that field living in the Broker.Spec (what the diff shows), it really should be living in the Broker.Status for this case. and it needs the proper json status tag (not just the comment).
Anyhoo, had a quick "would it be bad chat with @mattmoor " and they might be also interested in this for net-* status management as well, perhaps with these being KRshaped, maybe we should think about adding this to duck.Status as a field (non-serialized as described above), and then others would benefit. @whaught since you've done so much around that, we could chat about this thought experiment?

That would also pave the way to the injection path (since we already inject the class annotations), we could just inject the whole configset for direct manipulation by the actual controller?
@n3wscott

[vaikas]

And just to be clear, I'm not advocating that we do all these obviously as part of this, just trying to figure out the eventual shape we want things to look that would make the migration then easier. I think having it in the Broker.Status today might make it easier migration path 🤔
Ok, I'm done yammering for now :)

[grantr]

Thanks for tackling this @pierDipi! I haven't read the PR, so this is more a response to the comment by @vaikas about persistence.

I don't understand why the flavor of ConditionSet needs to be persisted in the object if we already have the broker class annotation there. Maybe if I tried to write the code it would become more obvious. But my initial thought is that the only indicator necessary is the broker class. The controller binary knows in advance what broker class it's going to be reconciling and what conditions are necessary for that broker class, so having a field to say what condition set is being used seems redundant.

In my head, the simplest solution is an exported package variable for BrokerCondSet that a controller main can override before starting controllers:

func main() {
  // ...
  
  v1beta1.BrokerCondSet = myBrokerClassCondSet()
  
  // Start controllers
}

Again I haven't read the PR or tried to implement this so I don't know if this really works or if there are benefits to something more complex. I know it doesn't work if we want to support multiple broker classes in a single process (as mentioned above), and maybe there's a reason to expect we'll need that sooner rather than later.

Just my thoughts from a high level.

[pierDipi]

Just to clarify what I'm doing in this PR:

• Both v1 and v1beta1
  • Add a function RegisterAlternateBrokerConditionSet(conditionSet apis.ConditionSet) that changes the global variable brokerCondSet in broker_lifecycle.go
    (see https://github.com/knative/eventing/pull/3545/files#diff-527d676d440c57bd3fbf06add81e5b33R41-R47)
  • Move methods non-implementation agnostic from broker_lifecycle.go to another file called broker_lifecyle_mt.go
    (see https://github.com/knative/eventing/pull/3545/files#diff-527d676d440c57bd3fbf06add81e5b33L75-R93 and https://github.com/knative/eventing/pull/3545/files#diff-aabccca390bb0fe4df1180597bcf3b95R2-R62).
    The reasons for this are described here: Allow registering custom conditions for the Broker #3545 (comment) (If this change doesn't belong to this PR, let me know)
  • Test RegisterAlternateBrokerConditionSet(conditionSet apis.ConditionSet)
    (see https://github.com/knative/eventing/pull/3545/files#diff-9860d42fce97062f53d90f0b39125437R62-R109)
• Use RegisterAlternateBrokerConditionSet() in pkg/reconciler/mtbroker/controller.go#NewController
  (see https://github.com/knative/eventing/pull/3545/files#diff-1f897f90f1413f2d479927e07fc5bc6dR97-R102) so that our reference implementation is the first user of this API.

I'm not changing Broker, Broker.Status or Broker.Spec.

Am I missing something in the big picture that you have instead?

Does it actually need to be persisted anywhere?

No, it doesn't.

That would also pave the way to the injection path (since we already inject the class annotations), we could just inject the whole configset for direct manipulation by the actual controller?

If I understand correctly, this is the longterm goal of the plan: #3094 (comment)

In my head, the simplest solution is an exported package variable for BrokerCondSet that a controller main can override before starting controllers:

func main() {
    // ...

   v1beta1.BrokerCondSet = myBrokerClassCondSet()

   // Start controllers
}

This is what this PR does through the function RegisterAlternateBrokerConditionSet instead of manipulating the variable directly.

[vaikas]

Thanks! And sorry for all the thought dumping on your PR :)
/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, vaikas

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 [vaikas]

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

[vaikas]

/check-cla

[vaikas]

@googlebot check-cla

[pierDipi]

@vaikas No problem 😄. As I wrote before, I agree with your thoughts, in particular, this one: #3545 (comment)
FYI, I've enabled this feature here: knative-extensions/eventing-kafka-broker#39, and it looks like it's working.

[dprotaso]

To clarify are you adding a dependent condition? ie. when your condition is false it changes 'Ready/Succeeded' to false?

[vaikas]

@dprotaso it's being able to add / replace the entire condition set. For example, something like kafka / rabbitmq broker might not even have a Channel, so exposing conditions that talk about them doesn't make sense. Does that make sense?
As I was talking to @mattmoor about this last week, he also mentioned that some of the net* classes might have a similar need (iirc, I think there are conditions that talk about virtual services that does not really make sense in some of the other net-* components).