-
Notifications
You must be signed in to change notification settings - Fork 532
Reuse controller manager #11602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Reuse controller manager #11602
Conversation
Signed-off-by: Dmitri Dolguikh <[email protected]>
…e option is set in tests Signed-off-by: Dmitri Dolguikh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Extends the kgateway startup API to reuse a single controller manager instance and allow external registration of additional controllers and manager options.
- Introduced functional options for REST config, controller-manager options, and extra manager configuration.
- Refactored setup flow: replaced multiple StartKgateway variants with a unified Start→BuildKgatewayWithConfig→mgr.Start sequence.
- Updated tests and examples to use the new manager-based setup API.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
test/envtestutil/util.go | Switched test harness to instantiate a controller-runtime Manager and call the new BuildKgateway API. |
pkg/setup/setup.go | Added RestConfig , CtrlMgrOptions , and ExtraManagerConfig to Options , removed core.AddToScheme invocation. |
internal/kgateway/setup/setup.go | Added functional option builders (WithRestConfig , WithControllerManagerOptions , WithExtraManagerConfig ), removed old XDS-specific entrypoints, unified Start . |
internal/kgateway/controller/start.go | Updated StartConfig to accept an injected manager.Manager , removed scheme-extension callback via AddToScheme , wired through cfg.Manager . |
examples/plugin/main.go | Replaced direct StartKgateway call with setup.New(...).Start(...) to demonstrate the new API. |
Comments suppressed due to low confidence (1)
pkg/setup/setup.go:28
- Add a GoDoc comment for
RestConfig
(and the other new fields) inOptions
to clarify their purpose.
RestConfig *rest.Config
}, | ||
}) | ||
if err != nil { | ||
panic("failed to create a manager") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] In a test goroutine, use t.Fatalf
or require.NoError
instead of panic
so the failure is reported by the test harness.
panic("failed to create a manager") | |
t.Fatalf("failed to create a manager: %v", err) |
Copilot uses AI. Check for mistakes.
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
@dmitri-d I think we can skip release note here? |
Description
It should be possible to update kgateway setup/initialization to support creating additional controllers without creating additional instances of controller manager.
Change Type
/kind cleanup
Changelog
Additional Notes