multiproject: centralize informers; harden manager; add docs #2972

panslava · 2025-09-03T12:57:53Z

Centralize informer lifecycle with InformerSet for simpler, shared wiring.
Harden ProviderConfigControllersManager:
- Add NEG cleanup finalizer on start; roll back on failures.
- Idempotent, concurrency-safe start; stop closes channel and removes finalizer using latest object.
- Introduce startNEGController test seam; unexport controllerSet; wrap errors; add docs.
Add README explaining multiproject structure and flow.
Tests: pkg/multiproject/manager/manager_test.go covers finalizer add/rollback, idempotent start, stop behavior, and concurrent single-shot.
No happy-path behavior change; improves robustness and testability.

/assign @swetharepakula

k8s-ci-robot · 2025-09-03T12:57:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: panslava

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~~ [panslava]

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

swetharepakula · 2025-11-24T22:14:59Z

pkg/multiproject/informerset/informerset.go

+	// Start optional informers
+	startInformer(i.SvcNeg, stopCh)
+	startInformer(i.Network, stopCh)
+	startInformer(i.GkeNetworkParams, stopCh)
+	startInformer(i.NodeTopology, stopCh)


since we are replicating the check on whether the client is nil or not multiple times. Can we consolidate in initialization the list of informers that needs to be started

tbh, tried to do it, but felt like overengineering. Adding a new abstraction to store with not so much gain, and makes it easier to miss and just seems even less readable to store same informers in additional list... I think we should leave it like this...

swetharepakula · 2025-11-25T19:33:39Z

pkg/multiproject/informerset/informerset.go

+}
+
+// hasSyncedFuncs returns a list of HasSynced functions for all non-nil informers.
+func (i *InformerSet) hasSyncedFuncs() []func() bool {


can we consolidate these checks perhaps at initialization?

So we save the set of functions and don't have to re-calculate every time?

Same as the previous comment... If you insist I can do it but seems a bit like overengineering, and I don't think re-calculation is too bad.... and nil checks are cheap

swetharepakula · 2025-11-26T04:49:59Z

pkg/multiproject/informerset/informerset_test.go

+		if err != nil {
+			t.Fatalf("Start() returned error: %v", err)
+		}
+		if ok := cache.WaitForCacheSync(stop, inf.CombinedHasSynced()); !ok {


can you expand this to show that it is waiting for multiple informers

Done, if i correctly understood what is needed

pkg/multiproject/informerset/informerset_test.go

swetharepakula · 2025-11-26T20:35:03Z

pkg/multiproject/manager/manager.go

+	pcLatest, err := pccm.providerConfigClient.CloudV1().ProviderConfigs().Get(context.Background(), pc.Name, metav1.GetOptions{})
+	if err != nil {
+		pcCopy := pc.DeepCopy()
+		pcCopy.Finalizers = append(pcCopy.Finalizers, finalizer.ProviderConfigNEGCleanupFinalizer)


We should optionally add this finalizer if it doesn't exist.
Is this to cover the case our cache hasn't updated to after us putting it on ?

Maybe this is a bit overengineering, but this function is only used to get PC on deletion

Idea is -- if by some case "get" failed (idk, some transient error), we don't want to leave PC hanging and still want to try to remove finalizer, so that's the logic besides this function

swetharepakula · 2025-11-26T21:28:44Z

pkg/multiproject/manager/manager_test.go

+// TestStart_AddsFinalizer_AndIsIdempotent verifies that starting controllers
+// for a ProviderConfig adds the NEG cleanup finalizer and that repeated starts
+// are idempotent (the underlying controller is launched only once).
+func TestStart_AddsFinalizer_AndIsIdempotent(t *testing.T) {


can you add a testcase for starting a controller for a PC that already has a finalizer (in the case of controller restart)

Added TestStart_WithExistingFinalizer_IsIdempotent

swetharepakula · 2025-11-26T21:29:25Z

pkg/multiproject/manager/manager_test.go

+
+// TestStart_FailureCleansFinalizer verifies that when starting controllers
+// fails, the added finalizer is rolled back and removed from the object.
+func TestStart_FailureCleansFinalizer(t *testing.T) {


what about the case that the PC already had a finalizer to begin with?

Good catch - was a bug. We were removing pre-existing finalizers on failure. Fixed + added test.

pkg/multiproject/neg/neg_test.go

Replace factory-based informer creation with centralized InformerSet that: - Creates base SharedIndexInformers directly without factories - Manages lifecycle and synchronization in one place - Provides filtered views for ProviderConfig-specific controllers - Reduces boilerplate and improves maintainability Using informers directly without factories simplifies the logic and eliminates potential mistakes from unnecessary factory usage, such as cidentally creating duplicate informers or incorrect factory scoping.

…here

- Add manager_test.go covering: - Start adds NEG cleanup finalizer and is idempotent - Start failure and GCE client creation failure roll back finalizer - Stop closes controller channel and removes finalizer - Concurrent starts for same ProviderConfig are single-shot - Improve manager.go: - Introduce test seam via package var startNEGController - Ensure finalizer is added before start; roll back on any startup error - Delete finalizer using latest ProviderConfig from API to avoid stale updates - Wrap errors with %w and add GoDoc comments - Rename exported ControllerSet to unexported controllerSet - No expected behavior change in the happy path; robustness and testability improved.

- Fix bug: preserve pre-existing finalizer on startup failure - Expand informerset tests to verify multiple informers sync - Add tests for controller restart with existing finalizer - Add comment explaining closeProvider bool in neg_test

panslava · 2025-12-02T06:24:21Z

Ready for re-review @swetharepakula

k8s-ci-robot assigned swetharepakula Sep 3, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 3, 2025

k8s-ci-robot requested review from MrHohn and sawsa307 September 3, 2025 12:58

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 3, 2025

panslava force-pushed the improve-multiproject branch 2 times, most recently from 69aec65 to 438d992 Compare September 3, 2025 13:21

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2025

panslava force-pushed the improve-multiproject branch from 72a34fc to 2eb739c Compare September 7, 2025 10:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2025

panslava force-pushed the improve-multiproject branch from 2eb739c to 3f6e8d2 Compare October 10, 2025 02:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2025

panslava force-pushed the improve-multiproject branch from 3f6e8d2 to 291e886 Compare October 20, 2025 11:47

swetharepakula reviewed Nov 26, 2025

View reviewed changes

swetharepakula mentioned this pull request Nov 26, 2025

Mt framework #3007

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2025

panslava added 4 commits December 1, 2025 17:03

Add README for multiproject folder, to briefly explain the logic in t…

d993403

…here

Add MT NEG Controller unit tests

470f2fe

panslava force-pushed the improve-multiproject branch from 291e886 to ea200f4 Compare December 2, 2025 04:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2025

Address PR review comments

ce5c037

- Fix bug: preserve pre-existing finalizer on startup failure - Expand informerset tests to verify multiple informers sync - Add tests for controller restart with existing finalizer - Add comment explaining closeProvider bool in neg_test

panslava force-pushed the improve-multiproject branch from ea200f4 to ce5c037 Compare December 2, 2025 04:12

multiproject: centralize informers; harden manager; add docs #2972

Are you sure you want to change the base?

multiproject: centralize informers; harden manager; add docs #2972

Uh oh!

Conversation

panslava commented Sep 3, 2025

Uh oh!

k8s-ci-robot commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panslava Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

panslava commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

panslava Dec 2, 2025 •

edited

Loading