Add ProviderConfig Controller #2792

panslava · 2025-01-13T03:07:31Z

/assign @gauravkghildiyal

This will be the very last to review

panslava · 2025-01-13T20:25:29Z

/retest

panslava · 2025-01-14T16:52:31Z

/retest

gauravkghildiyal · 2025-01-17T21:37:36Z

Thanks @panslava!

I'm yet to take a look, but glancing through: now that we've started making use of our libraries/packages, are there some useful unit / integration tests that we can add?

pkg/multiproject/controller/controller.go

cmd/glbc/main.go

pkg/multiproject/start/start.go

panslava · 2025-02-04T19:20:19Z

Added unit tests

Ready for review

This PR has a bit too many commits now, integration tests will be done in next PR

- Introduced a new ProviderConfigController to track ProviderConfig resources and start NEG controllers. - Added initialization and running the ProviderConfigController in multi-project mode in main.go.

…nt have specific purpose

gauravkghildiyal

Looking good. Two comments.

Reviewing the tests separately.

gauravkghildiyal · 2025-02-13T21:12:44Z

cmd/glbc/main.go

+		}, rOption.wg)
+
+		// Wait for the multi-project syncer to finish.
+		waitWithTimeout(rOption.wg, rootLogger)


This is a very strange function. It just waits for 30 seconds and then stops waiting. Do we actually need such a behaviour for some reason?

added waiting for stopCh to close before this

gauravkghildiyal · 2025-02-13T22:10:07Z

pkg/multiproject/neg/neg.go

+
+// initializeInformers wraps the base SharedIndexInformers in a providerConfig filter
+// and runs them.
+func initializeInformers(


I wonder if it'd be better that initializeInformers also returns a hasSynced function. It minimizes potential for future issues where initializeInformers and createHasSyncedFunc diverge. Basically this helps establish the invariant that if you're Run()ing and informer, it needs a corresponding hasSynced invocation

Added returning hasSynced from this func

panslava · 2025-02-13T23:14:51Z

/hold

found some problems on integration tests

gauravkghildiyal · 2025-02-13T23:40:28Z

pkg/multiproject/controller/controller_test.go

+	if err != nil {
+		t.Fatalf("failed to add ProviderConfig to indexer: %v", err)
+	}
+	time.Sleep(100 * time.Millisecond)


If these are helper functions, shouldn't waiting for any expected delays happen as part of the actual Test?

Moved waiting to test body

gauravkghildiyal · 2025-02-14T00:21:08Z

pkg/multiproject/controller/controller_test.go

+}
+
+// TestProcessUpdateFunc ensures UpdateFunc enqueues the item, triggers re-sync.
+func TestProcessUpdateFunc(t *testing.T) {


As far as being a unit test goes, I think this test is sufficient.

But this does make me wonder about the "immutability" of ProviderConfig. (Thanks to @swetharepakula for reminding me about this aspect a few days back)

We should assume that the ProviderConfig is immutable, in which case our current logic is correct. But if that is true, do we need a "UpdateFunc" handler for it? I'd say yes, it's good to have. But our actual code won't really do anything if the ProviderConfig is updated, and perhaps we should log it somewhere. The only kind of update which our code cares about is a DeletionEvent.

While assuming immutability is sufficient for now, if on the other hand you really did want to support an actual "Update" to the ProviderConfig, than perhaps the code still needs some work with how it will stop the previous ProviderConfig controller and start the new one.

yeah, maybe we just don't need this test for now, removed

Current implementation will not start any new controllers if providerConfig was updated, as we start only once per ProviderConfig key (which is namespace+name)

panslava · 2025-02-18T19:05:29Z

/retest

- Remove Update Unit test - Add waiting for stopCh to close - Return HasSynced from initializeInformers

gauravkghildiyal · 2025-02-18T21:01:49Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2025-02-18T21:02:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gauravkghildiyal, 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~~ [gauravkghildiyal,panslava]

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

panslava · 2025-02-18T22:05:52Z

/unhold

k8s-ci-robot assigned gauravkghildiyal Jan 13, 2025

k8s-ci-robot requested review from aojea and sawsa307 January 13, 2025 03:07

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 13, 2025

panslava force-pushed the multi-project-cluster-slice-controller branch 2 times, most recently from 8ca2383 to 64a3448 Compare January 13, 2025 20:14

panslava force-pushed the multi-project-cluster-slice-controller branch from 64a3448 to 63b6cbe Compare January 13, 2025 20:32

panslava force-pushed the multi-project-cluster-slice-controller branch 2 times, most recently from 0c861ed to fd75ee7 Compare January 15, 2025 20:37

k8s-ci-robot 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 Jan 15, 2025

panslava force-pushed the multi-project-cluster-slice-controller branch 2 times, most recently from 6fde296 to 717f4c9 Compare January 16, 2025 21:20

gauravkghildiyal reviewed Jan 24, 2025

View reviewed changes

panslava force-pushed the multi-project-cluster-slice-controller branch from 12a0122 to 262703c Compare February 4, 2025 18:43

k8s-ci-robot 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 Feb 4, 2025

panslava force-pushed the multi-project-cluster-slice-controller branch from 00d673c to 013fb0a Compare February 4, 2025 19:18

panslava force-pushed the multi-project-cluster-slice-controller branch 3 times, most recently from f413eeb to 1a7e4d5 Compare February 4, 2025 21:30

panslava added 3 commits February 11, 2025 19:26

Add ProviderConfig Controller

b5393a4

- Introduced a new ProviderConfigController to track ProviderConfig resources and start NEG controllers. - Added initialization and running the ProviderConfigController in multi-project mode in main.go.

Add start.go

8f3b823

Remove multiproject sharedcontext. It was a super structure which did…

c3cbc96

…nt have specific purpose

panslava added 5 commits February 11, 2025 19:26

Add leader election to starting multi-project-controller

aaef741

Add an option to stat multiproject controller without leader election

3388fc0

Add unit tests for ProviderConfig controller

8b8d41f

Add comments to public functions

aeb99a9

Properly start (by calling Run) all multiproject informers

c63a7aa

panslava force-pushed the multi-project-cluster-slice-controller branch from 5fe5c23 to 3156e41 Compare February 11, 2025 21:12

Fix MultiProject NEG endpointSliceInformer by adding indexer

bc8909a

panslava force-pushed the multi-project-cluster-slice-controller branch from 3156e41 to bc8909a Compare February 11, 2025 21:29

gauravkghildiyal reviewed Feb 13, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2025

gauravkghildiyal reviewed Feb 14, 2025

View reviewed changes

Fix review comments

663e98b

- Remove Update Unit test - Add waiting for stopCh to close - Return HasSynced from initializeInformers

panslava force-pushed the multi-project-cluster-slice-controller branch from 5819664 to 663e98b Compare February 18, 2025 20:59

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2025

k8s-ci-robot merged commit 499d76e into kubernetes:master Feb 18, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ProviderConfig Controller #2792

Add ProviderConfig Controller #2792

panslava commented Jan 13, 2025

panslava commented Jan 13, 2025

panslava commented Jan 14, 2025

gauravkghildiyal commented Jan 17, 2025

panslava commented Feb 4, 2025

gauravkghildiyal left a comment

gauravkghildiyal Feb 13, 2025

panslava Feb 18, 2025

gauravkghildiyal Feb 13, 2025

panslava Feb 18, 2025

panslava commented Feb 13, 2025

gauravkghildiyal Feb 13, 2025

panslava Feb 18, 2025

gauravkghildiyal Feb 14, 2025

panslava Feb 18, 2025

panslava commented Feb 18, 2025

gauravkghildiyal commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

panslava commented Feb 18, 2025

Add ProviderConfig Controller #2792

Add ProviderConfig Controller #2792

Conversation

panslava commented Jan 13, 2025

panslava commented Jan 13, 2025

panslava commented Jan 14, 2025

gauravkghildiyal commented Jan 17, 2025

panslava commented Feb 4, 2025

gauravkghildiyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panslava commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panslava commented Feb 18, 2025

gauravkghildiyal commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

panslava commented Feb 18, 2025