Preserve user-supplied service port#3003
Conversation
Signed-off-by: Manabu McCloskey <manabu.mccloskey@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR fixes mutateServerService so that, when creating a new SparkConnect server Service, it no longer overwrites spec.ports entirely. Instead, it preserves any user-supplied ports and appends any missing required SparkConnect ports (matched by port name), while continuing to leave existing Services’ ports untouched.
Changes:
- Update
mutateServerServiceto merge required Service ports into any user-suppliedspec.portsduring Service creation. - Add unit tests covering preservation of custom ports and non-overwrite behavior for user-supplied required ports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/controller/sparkconnect/reconciler.go | Changes Service port mutation logic to merge required ports instead of replacing user-supplied ports on creation. |
| internal/controller/sparkconnect/reconciler_test.go | Adds tests validating port preservation/merge behavior for user-supplied Service port overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nabuskey
changed the title
tariq-hasan
left a comment
tariq-hasan
left a comment
There was a problem hiding this comment.
Hi @nabuskey! I have added a few comments.
| ports := map[string]struct{}{} | ||
| for _, port := range svc.Spec.Ports { | ||
| ports[port.Name] = struct{}{} | ||
| } | ||
| for _, port := range requiredPorts { | ||
| _, ok := ports[port.Name] | ||
| if !ok { | ||
| svc.Spec.Ports = append(svc.Spec.Ports, port) | ||
| } | ||
| } |
There was a problem hiding this comment.
I am wondering if we want to start with the required ports as the base and only add in user-provided ports which do not conflict on the port name and which do not conflict on the port number and protocol combination.
I mention this because otherwise we might have a user-provided name targeting 15002, for example, and spark-connect-server targeting 15002 as well which would then cause an API server error.
Also the ports are hardcoded either in the operator or natively in Spark. So if a user overrides the port number for web-ui to 4000, for example, then it could be pointing at a port on the driver pod that may not be serving any requests.
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I am wondering if webhook validation on the user-provided service would be useful as well.
This would be for the four protected ports as well as other user-provided ports in case the spec for those are invalid as well.
mutateServerServiceoverwrote the entireSpec.Portsslice on creation,discarding ports supplied via the
Server.Serviceoverride.It now merges instead: user-supplied ports are kept, and any missing required
ports are appended by name. Existing Services are left untouched.
Change Category
Rationale
Checklist
Additional Notes