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

pkg/multiproject/informerset/informerset.go

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

I think we should instead throw an error in this situation. Is there a reason we don't think the version in the cache is not up to date?

Yeah, I agree actually, removed this, inlined getting providerconfig

pkg/multiproject/manager/manager_test.go

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

pkg/multiproject/informerset/informerset.go

pkg/multiproject/informerset/informerset_test.go

swetharepakula · 2025-12-03T22:39:06Z

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)


I think we should instead throw an error in this situation. Is there a reason we don't think the version in the cache is not up to date?

swetharepakula · 2025-12-04T01:11:50Z

pkg/multiproject/manager/manager.go

 	// happens in that window, cleanup could be skipped. We roll this back on
-	// any subsequent startup error.
+	// any subsequent startup error only if we added it.
 	err := finalizer.EnsureProviderConfigNEGCleanupFinalizer(pc, pccm.providerConfigClient, logger)


move this to right before we start the controller, so we ensure all clients are up. This way we don't have to remove because GCE client didn't initialize.

- Create GCE client before adding finalizer (no rollback needed on fail) - Return errors from StopControllersForProviderConfig for requeue - Remove silent fallback in latestPCWithCleanupFinalizer

panslava · 2025-12-04T09:09:18Z

Ready for re-review @swetharepakula

swetharepakula

One more nit and this PR lgtm. Thanks!

pkg/multiproject/manager/manager.go

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2025-12-05T02:31:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: panslava, swetharepakula

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,swetharepakula]

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

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

swetharepakula reviewed Dec 4, 2025

View reviewed changes

Address PR review: reorder start flow; return errors from Stop

4953fed

- Create GCE client before adding finalizer (no rollback needed on fail) - Return errors from StopControllersForProviderConfig for requeue - Remove silent fallback in latestPCWithCleanupFinalizer

swetharepakula reviewed Dec 4, 2025

View reviewed changes

pkg/multiproject/manager/manager.go Show resolved Hide resolved

swetharepakula reviewed Dec 5, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2025

k8s-ci-robot merged commit acc1f4e into kubernetes:master Dec 5, 2025
5 checks passed

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

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

Uh oh!

Conversation

panslava commented Sep 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swetharepakula Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

panslava Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

swetharepakula Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

panslava Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panslava commented Dec 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swetharepakula Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

swetharepakula Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

panslava Dec 4, 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!

panslava commented Dec 4, 2025

Uh oh!

swetharepakula left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swetharepakula left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

panslava Dec 4, 2025 •

edited

Loading