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

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.

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

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?

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

swetharepakula · 2025-11-26T19:32:49Z

pkg/multiproject/informerset/informerset_test.go

+		t.Fatalf("FilterByProviderConfig returned nil (after start)")
+	}
+	if !filteredAfter.started {
+		t.Fatalf("expected filtered InformerSet to have started=true after base Start")


should we check all the informers?

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 ?

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)

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?

swetharepakula · 2025-11-26T22:24:25Z

pkg/multiproject/neg/neg_test.go

+
+	testCases := []struct {
+		name          string
+		closeProvider bool


add a comment to what this bool dictates in the test

k8s-ci-robot · 2025-11-27T06:26:42Z

PR needs rebase.

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.

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 added 3 commits October 9, 2025 19:11

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

19268b8

…here

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

Add MT NEG Controller unit tests

291e886

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

panslava commented Sep 3, 2025

Uh oh!

k8s-ci-robot commented Sep 3, 2025

Uh oh!

swetharepakula Nov 24, 2025

Uh oh!

swetharepakula Nov 25, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

swetharepakula Nov 26, 2025

Uh oh!

k8s-ci-robot commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants