Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Broker class spec based default #6621

Conversation

liuchangyan
Copy link
Contributor

@liuchangyan liuchangyan commented Nov 21, 2022

Fixes #5992
Signed-off-by: Teresaliu [email protected]

Proposed Changes

  • Add multiple broker.spec fields based on the configured broker class in clusterDefault of default-br-config
  • Achieve to set different brokerConfig for different brokerClass in a cluster

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

In Administrator configuration options of Broker, we've added a new Spec feature: brokerClasses

Docs

Choosing the Broker Config for the configured Brokerclass in clusterDefault of default-br-config.

You can configure BrokerClasses in clusterDefault to set multiple broker.spec. When there are multiple classes in the same cluster, the BrokerClass in clusterDefault or namespaceDefault could find the corresponding Broker Config for the configured BrokerClass.
For example,if you wanted to set the KafkaBroker Class and its config-kafka-channel ConfigMap for all other Brokers created, but wanted to set MTChannelBasedBroker Class and its config-br-default-channel ConfigMap for namespace-1 and namespace-2, you would use the following settings:

  apiVersion: v1
  kind: ConfigMap
  metadata:
    name: config-br-defaults
    namespace: knative-eventing
    labels:
      eventing.knative.dev/release: devel
  data:
    default-br-config: |
      clusterDefault:
        brokerClass: KafkaBroker
        brokerClasses:
          MTChannelBasedBroker:
            spec:
              delivery:
                retry: 3
                deadLetterSink:
                  ref:
                    apiVersion: serving.knative.dev/v1
                    kind: Service
                    name: mt-handle-error
                    namespace: knative-eventing
                backoffPolicy: exponential
                backoffDelay: 3s
              config:
                apiVersion: v1
                kind: ConfigMap
                name: config-br-default-channel
                namespace: knative-eventing
          KafkaBroker:
            spec:
              delivery:
                retry: 3
                deadLetterSink:
                  ref:
                    apiVersion: serving.knative.
                    kind: Service
                    name: kafka-handle-error
                    namespace: knative-eventing
                backoffPolicy: exponential
                backoffDelay: 3s
              config:
                apiVersion: v1
                kind: ConfigMap
                name: config-kafka-channel
                namespace: knative-eventing
      namespaceDefaults:
        namespace-1:
          brokerClass: MTChannelBasedBroker
        namespace-2:
          brokerClass: MTChannelBasedBroker

@knative-prow
Copy link

knative-prow bot commented Nov 21, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liuchangyan
Once this PR has been reviewed and has the lgtm label, please assign zroubalik for approval by writing /assign @zroubalik in a comment. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 81.94% // Head: 80.60% // Decreases project coverage by -1.33% ⚠️

Coverage data is based on head (3ba0387) compared to base (03eafac).
Patch coverage: 90.62% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6621      +/-   ##
==========================================
- Coverage   81.94%   80.60%   -1.34%     
==========================================
  Files         235      236       +1     
  Lines       11774    12050     +276     
==========================================
+ Hits         9648     9713      +65     
- Misses       1650     1856     +206     
- Partials      476      481       +5     
Impacted Files Coverage Δ
pkg/apis/config/defaults.go 88.15% <90.62%> (-5.03%) ⬇️
pkg/apis/flows/v1/sequence_lifecycle.go 86.79% <0.00%> (-4.61%) ⬇️
pkg/channel/message_dispatcher.go 76.85% <0.00%> (-3.88%) ⬇️
pkg/apis/messaging/v1/subscription_validation.go 87.71% <0.00%> (-1.99%) ⬇️
pkg/adapter/v2/main.go 67.30% <0.00%> (-0.83%) ⬇️
pkg/reconciler/subscription/subscription.go 83.33% <0.00%> (-0.80%) ⬇️
pkg/broker/filter/filter_handler.go 75.51% <0.00%> (-0.35%) ⬇️
pkg/adapter/v2/configurator_configmap.go 82.19% <0.00%> (-0.25%) ⬇️
pkg/apis/eventing/v1/broker_lifecycle.go 100.00% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 88 to 91
// BrokerClassSpec set the multiple configurations of different brokerClass.
// Different brokerClass use corresponding brokerConfig for all the namespaces that
// are not in NamespaceDefaultBrokerConfigs.
BrokerClassSpec map[string]*BrokerConfigSpec `json:"brokerClassSpec,omitempty"`
Copy link
Member

@pierDipi pierDipi Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it brokerClasses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call it brokerClasses?

Sure

@pierDipi
Copy link
Member

Thanks @liuchangyan!

Can you please add release note doc in the PR description?
We should also document this new feature in knative/docs, so we would need an issue in that repo linked to this PR

@liuchangyan
Copy link
Contributor Author

Thanks @liuchangyan!

Can you please add release note doc in the PR description? We should also document this new feature in knative/docs, so we would need an issue in that repo linked to this PR

ok

Comment on lines 88 to 90
// BrokerClassSpec set the multiple configurations of different brokerClass.
// Different brokerClass use corresponding brokerConfig for all the namespaces that
// are not in NamespaceDefaultBrokerConfigs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we leaving broker-class based defaults for namespace defaults out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no, we are not leaving, the annotation description is wrong

@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from ca24516 to bdce2f4 Compare November 22, 2022 11:53
@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from 28e09ae to ca0f0e7 Compare November 22, 2022 14:12
@liuchangyan
Copy link
Contributor Author

Thanks @liuchangyan!

Can you please add release note doc in the PR description? We should also document this new feature in knative/docs, so we would need an issue in that repo linked to this PR

I will take few days off this week, I will add release note doc next week, Thanks for your review :) !

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add additional tests to broker_defaults.go https://github.com/knative/eventing/blob/main/pkg/apis/eventing/v1/broker_defaults_test.go to test the e2e defaulting mechanism ?

For example with scenario like:

  • no broker class specified and the defaulted broker class set to point to a broker class based default
  • broker class specified that points to a broker class based default (one for each "scope" namespaced and cluster)
  • etc

Comment on lines 184 to 192
for bClass, bCSpec := range brokerClasses {
if bClass == brokerClass {
var bConfig BrokerConfig
bConfig.KReference = bCSpec.Spec.Config
bConfig.Delivery = bCSpec.Spec.Delivery
return &bConfig
}
}
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the map access here:

bCSpec, ok := brokerClasses[brokerClass]

@liuchangyan
Copy link
Contributor Author

Can we add additional tests to broker_defaults.go https://github.com/knative/eventing/blob/main/pkg/apis/eventing/v1/broker_defaults_test.go to test the e2e defaulting mechanism ?

For example with scenario like:

  • no broker class specified and the defaulted broker class set to point to a broker class based default
  • broker class specified that points to a broker class based default (one for each "scope" namespaced and cluster)
  • etc

Sure

@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2022
@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from d22c904 to 2ad62d2 Compare December 9, 2022 08:27
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2022
@matzew
Copy link
Member

matzew commented Dec 9, 2022

/retest

@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from de2f59a to 2ad62d2 Compare December 12, 2022 05:45
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 12, 2022
@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from c481170 to 2ad62d2 Compare December 12, 2022 06:23
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 12, 2022
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2022
@liuchangyan liuchangyan force-pushed the KNATIVE_Add-BrokerClass-based-default branch from 4829d81 to 1fa7225 Compare December 12, 2022 10:18
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2022
@liuchangyan
Copy link
Contributor Author

/test upgrade-tests

@@ -461,6 +531,96 @@ func TestBrokerSetDefaults(t *testing.T) {
},
},
},
"no config, uses namespace4's broker class and config": {
Copy link
Member

@pierDipi pierDipi Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another interesting use case to test is:

  • initial: broker in namespace namespace4 (without broker class set)
  • namespace4's class-based default is set for the cluster-level broker class (aka BrokerClass: eventing.MTChannelBrokerClassValue, line 144)

I think, in this case we expect that we use the cluster-level default to default the class and the namespace4 default for that particular class to default the rest

Copy link
Member

@pierDipi pierDipi Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, another interesting tests is when:

  • initial: broker in namespace namespace11 (without broker class set)
  • cluster-level class-based default is set for the cluster-level broker class (aka BrokerClass: eventing.MTChannelBrokerClassValue, line 144)

I think, in this case we expect that we use the cluster-level default to default the class and cluster-level class-based default to default the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got your point


// Add cluster-level multi-class-based configs to defaultConfig
brokerClasses := make(map[string]*config.BrokerConfigSpec)
brokerSpec1 := &config.BrokerSpec{
Copy link
Member

@pierDipi pierDipi Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call all brokerSpec<X> variables with the associated class for clarity like brokerSpecMTChannelBasedBroker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it

@@ -117,6 +142,34 @@ func TestGetBrokerClass(t *testing.T) {
}

func TestDefaultsConfiguration(t *testing.T) {
brokerClasses := make(map[string]*BrokerConfigSpec)
brokerSpec1 := &BrokerSpec{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here on the naming

Comment on lines +122 to 125
// get namespace default config
if present && value.BrokerConfig != nil {
return value.BrokerConfig, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I specify broker-class based default at the namespace level value.BrokerClasses I don't think this would work because we returning always the single value.BrokerConfig

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and that makes me think we need a test for this case as well.

Copy link
Contributor Author

@liuchangyan liuchangyan Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I specify broker-class based default at the namespace level value.BrokerClasses I don't think this would work because we returning always the single value.BrokerConfig

I see your point, I think the broker-class based default at the namespace level we may not use it , but once we set the the single value.BrokerConfig we surely want to use it. And is it necessary to set broker-class based default at the namespace level ? Because we already set it at the cluster level, and in namespace we use the config of cluster is ok, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to set broker-class based default at the namespace level ?

Yes, I think it's a valid use case and also for consistency with the cluster-level defaults

return nil
}
if bCSpec := getBrokerClasses(d); bCSpec != nil {
bConfig := matchBrokerClass(cb.BrokerClass, bCSpec)
Copy link
Member

@pierDipi pierDipi Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we are always using cb.BrokerClass which is equal to ClusterDefault.BrokerClass but if I create a Broker with an already set Broker class, we won't chose the correct configuration

Copy link
Contributor Author

@liuchangyan liuchangyan Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the brokerclass we set is in cluster level right? We always need to set a BrokerClass in ClusterDefault, and then we choose the corresponding BrokerConfig

@matzew
Copy link
Member

matzew commented Feb 3, 2023

What is the status here?

@github-actions
Copy link

github-actions bot commented May 5, 2023

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 5, 2023
@github-actions github-actions bot closed this Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broker class based defaults
4 participants