Reduce mt-broker-controller memory usage with namespaced endpoint informer#8418

pierDipi · 2025-01-22T13:49:00Z

Currently, the mt-broker-controller is using a cluster-wide endpoints
informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE".

Using the namespaced informer factory ensures that the watcher
is only watching endpoints in the knative-eventing (also known as
SYSTEM_NAMESPACE) namespace.

The secret informer is already using a namesapced watcher.
The only one which is cluster-wide is the configmap watcher but
we can't remove that one since broker configurations could
exist in any namespace.

Fixes #

Proposed Changes

Reduce mt-broker-controller memory usage with namespaced endpoint informer

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

Reduce `mt-broker-controller` memory usage with namespaced endpoint informer.

Docs

…ormer Currently, the mt-broker-controller is using a cluster-wide endpoints informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE". Using the namespaced informer factory ensures that the watcher is only watching endpoints in the `knative-eventing` (also known as `SYSTEM_NAMESPACE`) namespace. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2025-01-22T13:53:13Z

/cherry-pick release-1.17

knative-prow-robot · 2025-01-22T13:53:16Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.17 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.17

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-sigs/prow repository.

pierDipi · 2025-01-22T13:53:17Z

/cherry-pick release-1.16

pierDipi · 2025-01-22T13:53:20Z

/cherry-pick release-1.15

knative-prow-robot · 2025-01-22T13:53:20Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.16 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.16

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-sigs/prow repository.

knative-prow-robot · 2025-01-22T13:53:23Z

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

codecov · 2025-01-22T13:55:07Z

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 64.10%. Comparing base (9cec50f) to head (060934f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/reconciler/broker/controller.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8418      +/-   ##
==========================================
- Coverage   64.10%   64.10%   -0.01%     
==========================================
  Files         388      388              
  Lines       23359    23363       +4     
==========================================
+ Hits        14975    14976       +1     
- Misses       7589     7591       +2     
- Partials      795      796       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matzew

/lgtm
/approve

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

matzew · 2025-01-22T16:16:09Z

pkg/reconciler/broker/controller.go

-	endpointsInformer := endpointsinformer.Get(ctx)
+
+	endpointsInformer := namespacedinformerfactory.Get(ctx).Core().V1().Endpoints()
+	if err := controller.StartInformers(ctx.Done(), endpointsInformer.Informer()); err != nil {


matzew

/lgtm
/approve

knative-prow · 2025-01-22T16:16:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [matzew,pierDipi]

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

creydr

/lgtm

knative-prow-robot · 2025-01-22T16:18:15Z

@pierDipi: new pull request created: #8420

Details

In response to this:

/cherry-pick release-1.15

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-sigs/prow repository.

knative-prow-robot · 2025-01-22T16:18:53Z

@pierDipi: new pull request created: #8421

Details

In response to this:

/cherry-pick release-1.17

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-sigs/prow repository.

knative-prow-robot · 2025-01-22T16:19:31Z

@pierDipi: new pull request created: #8422

Details

In response to this:

/cherry-pick release-1.16

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-sigs/prow repository.

…ormer (knative#8418) * Reduce mt-broker-controller memory usage with namespaced endpoint informer Currently, the mt-broker-controller is using a cluster-wide endpoints informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE". Using the namespaced informer factory ensures that the watcher is only watching endpoints in the `knative-eventing` (also known as `SYSTEM_NAMESPACE`) namespace. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Start informer Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

…ormer (knative#8418) (#1002) * Reduce mt-broker-controller memory usage with namespaced endpoint informer Currently, the mt-broker-controller is using a cluster-wide endpoints informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE". Using the namespaced informer factory ensures that the watcher is only watching endpoints in the `knative-eventing` (also known as `SYSTEM_NAMESPACE`) namespace. * Start informer --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

…ormer (knative#8418) * Reduce mt-broker-controller memory usage with namespaced endpoint informer Currently, the mt-broker-controller is using a cluster-wide endpoints informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE". Using the namespaced informer factory ensures that the watcher is only watching endpoints in the `knative-eventing` (also known as `SYSTEM_NAMESPACE`) namespace. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Start informer Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

…ormer (knative#8418) (#1012) * Reduce mt-broker-controller memory usage with namespaced endpoint informer Currently, the mt-broker-controller is using a cluster-wide endpoints informer but it actually only uses endpoints in the "SYSTEM_NAMESPACE". Using the namespaced informer factory ensures that the watcher is only watching endpoints in the `knative-eventing` (also known as `SYSTEM_NAMESPACE`) namespace. * Start informer --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025

knative-prow bot requested review from Cali0707 and matzew January 22, 2025 13:49

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 22, 2025

matzew approved these changes Jan 22, 2025

View reviewed changes

knative-prow bot assigned matzew Jan 22, 2025

knative-prow bot added lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 22, 2025

pierDipi force-pushed the namespaced-endpoints-informer-mt-broker branch 2 times, most recently from 509d984 to b76b7ba Compare January 22, 2025 14:34

Start informer

060934f

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi force-pushed the namespaced-endpoints-informer-mt-broker branch from b76b7ba to 060934f Compare January 22, 2025 14:34

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2025

pierDipi requested a review from matzew January 22, 2025 15:15

pierDipi changed the title ~~[WIP] Reduce mt-broker-controller memory usage with namespaced endpoint informer~~ Reduce mt-broker-controller memory usage with namespaced endpoint informer Jan 22, 2025

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025

matzew reviewed Jan 22, 2025

View reviewed changes

matzew approved these changes Jan 22, 2025

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2025

creydr reviewed Jan 22, 2025

View reviewed changes

knative-prow bot assigned creydr Jan 22, 2025

knative-prow bot merged commit 28c489e into knative:main Jan 22, 2025
35 of 36 checks passed

knative-prow-robot mentioned this pull request Jan 22, 2025

[release-1.15] Reduce mt-broker-controller memory usage with namespaced endpoint informer #8420

Merged

knative-prow-robot mentioned this pull request Jan 22, 2025

[release-1.17] Reduce mt-broker-controller memory usage with namespaced endpoint informer #8421

Merged

knative-prow-robot mentioned this pull request Jan 22, 2025

[release-1.16] Reduce mt-broker-controller memory usage with namespaced endpoint informer #8422

Merged

Conversation

pierDipi commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

pierDipi commented Jan 22, 2025

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

pierDipi commented Jan 22, 2025

Uh oh!

pierDipi commented Jan 22, 2025

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

codecov bot commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

matzew Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Jan 22, 2025

Uh oh!

creydr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

knative-prow-robot commented Jan 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierDipi commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading