Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a toggle for the port allocator feature, allowing it to be enabled or disabled via a command-line flag and Helm configuration. The changes include conditional initialization in the main entry point and safety checks in the port allocator package to handle cases where it is not initialized. A review comment suggests improving error handling by using a sentinel error instead of ad-hoc error strings, which would allow callers to programmatically detect the disabled state.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in switch for the port allocator feature so clusters can run RBGS without port allocation unless explicitly enabled (via CLI flag / Helm value), and wires that toggle through controller startup and Helm deployment.
Changes:
- Introduces
--enable-port-allocatorand plumbs it intoSetupPortAllocator(...), plus a globalIsEnabled()check. - Gates RoleInstanceSet role-scoped port annotation handling behind the allocator-enabled switch.
- Updates Helm values/template and e2e workflow to optionally enable the feature.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/reconciler/roleinstanceset_reconciler.go | Gates role-scoped port annotation allocation/copying behind portallocator.IsEnabled() and refactors logic into a helper. |
| pkg/port-allocator/port_allocator.go | Adds enabled/disabled semantics (IsEnabled, ErrPortAllocatorDisabled) and updates initialization to support being “off” by default. |
| cmd/rbgs/main.go | Adds --enable-port-allocator flag and passes it into port allocator setup. |
| deploy/helm/rbgs/values.yaml | Adds portAllocator.enabled value and documents port allocator settings. |
| deploy/helm/rbgs/templates/manager.yaml | Conditionally injects port allocator flags only when enabled. |
| .github/workflows/e2e-test.yml | Enables port allocator in e2e Helm install. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if portallocator.IsEnabled() { | ||
| if err := allocateRoleScopedPortAnnotations(roleInstanceTemplateConfig.Components, old, roleInstanceSetAnnotation); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Gating RoleScoped port annotation handling on portallocator.IsEnabled() means that when the allocator is disabled the reconciler will stop applying the previously-managed port allocation annotations. With server-side apply, this can result in existing role-scoped port annotations being removed from RoleInstanceSet on update. Consider always preserving/copying existing role-scoped port annotation keys from the current RoleInstanceSet, and only performing new allocations when the allocator is enabled.
| needCreate := old.Name == "" | ||
| if needCreate { | ||
| // Allocate RoleScoped ports for this component. | ||
| portAnnotations, err := portallocator.AllocateRoleScopedPorts(config, *component.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
RoleScoped ports are only allocated when old.Name == "" (create). If the controller starts with port allocator disabled (so no annotations are written) and later runs with it enabled, existing RoleInstanceSets will never get role-scoped ports because they are treated as updates and only the (missing) annotations are copied. To support “enable on demand”, consider allocating any missing role-scoped ports on update when enabled (while still preserving existing values to avoid reallocation).
| } | ||
| alloc.pa = pa | ||
| alloc.enabled = true | ||
| return nil |
There was a problem hiding this comment.
configure() sets alloc.enabled=true before SetupPortAllocator calls start(). If start() fails, SetupPortAllocator returns an error but leaves the global allocator in an “enabled” state with a partially initialized allocator, which can cause confusing behavior if any code continues running (tests, embedding, or future refactors). Consider setting enabled=true only after start succeeds, or resetting enabled=false on start error.
| func AllocateBatch(num int32) ([]int32, error) { | ||
| if portAllocator == nil || !IsEnabled() { | ||
| return nil, ErrPortAllocatorDisabled | ||
| } | ||
| return portAllocator.pa.AllocateBatch(num) |
There was a problem hiding this comment.
AllocateBatch now returns ErrPortAllocatorDisabled unless portAllocator.enabled is true. In-package tests (e.g. pkg/port-allocator/port_allocator_test.go) and any internal callers that set the global portAllocator directly (without setting enabled) will start failing with ErrPortAllocatorDisabled. Either update those tests/helpers to set enabled=true, or provide a helper initializer to avoid brittle direct global mutation.
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.