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
Fix a small regression in Service updates #104601
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,17 +318,37 @@ func patchAllocatedValues(newSvc, oldSvc *api.Service) { | |
} | ||
|
||
if needsNodePort(oldSvc) && needsNodePort(newSvc) { | ||
nodePortsUsed := func(svc *api.Service) sets.Int32 { | ||
used := sets.NewInt32() | ||
for _, p := range svc.Spec.Ports { | ||
if p.NodePort != 0 { | ||
used.Insert(p.NodePort) | ||
} | ||
} | ||
return used | ||
} | ||
|
||
// Build a set of all the ports in oldSvc that are also in newSvc. We know | ||
// we can't patch these values. | ||
used := nodePortsUsed(oldSvc).Intersection(nodePortsUsed(newSvc)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jordan's example considers healthCheckNodePort too https://github.com/kubernetes/kubernetes/pull/103532/files#r694831725 If the user sets a new Nodeport value that matches the current allocated HealthCheckNodePort, should we change the HealthCheckNodePort? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that has always failed, since the node port allocate step happens before the HCNP deallocate. I am fine with that continuing to fail, and honestly, letting nodeports fail isn't SO bad, but it is a breakage in 22 vs 21. |
||
|
||
// Map NodePorts by name. The user may have changed other properties | ||
// of the port, but we won't see that here. | ||
np := map[string]int32{} | ||
for i := range oldSvc.Spec.Ports { | ||
p := &oldSvc.Spec.Ports[i] | ||
np[p.Name] = p.NodePort | ||
} | ||
|
||
// If newSvc is missing values, try to patch them in when we know them and | ||
// they haven't been used for another port. | ||
for i := range newSvc.Spec.Ports { | ||
p := &newSvc.Spec.Ports[i] | ||
if p.NodePort == 0 { | ||
p.NodePort = np[p.Name] | ||
oldVal := np[p.Name] | ||
if !used.Has(oldVal) { | ||
p.NodePort = oldVal | ||
} | ||
} | ||
} | ||
} | ||
|
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.
Low pri
on all proves ( don't think we should fix but we should ack it).
idx
is never tested againstlen(<the thing we want to test>)
. This could yield into undesirable error, specially when i think these should be part ofmake.go
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.
The good news is that they will panic if wrong.
I agree finding a way to re-use these will be nice - as a followup to the mega PR :)