is there a reason we can't just simplify this to

nodePort2 := ServiceNodePortRange.Base + (nodePort1 + i) % ServiceNodePortRange.Size

? The multiple offsets with subtraction and addition is a little confusing to follow.

That doesn't give me the loop I want. I could just linear search from base
to base+limit if you think that would be easier to understand, but I
thought this was not too bad - get the offset of nodePort1, increment it
with wraparound, add it to base.

On Thu, Dec 10, 2015 at 4:20 PM, Jeff Grafton notifications@github.com
wrote:

In test/e2e/service.go
#18532 (comment)
:

@@ -504,15 +504,22 @@ var _ = Describe("Services", func() {
By("hitting the pod through the service's LoadBalancer")
testLoadBalancerReachable(ingress1, 80)

•   By("changing service " + serviceName + " update NodePort")
  

•   nodePort2 := nodePort1 - 1
  

•   if !ServiceNodePortRange.Contains(nodePort2) {
  

•       //Check for (unlikely) assignment at bottom of range
  

•       nodePort2 = nodePort1 + 1
  

•   By("changing service " + serviceName + ": update NodePort")
  

•   nodePort2 := 0
  

•   for i := 1; i < ServiceNodePortRange.Size; i++ {
  

•       offs1 := nodePort1 - ServiceNodePortRange.Base
  

is there a reason we can't just simplify this to

nodePort2 := ServiceNodePortRange.Base + (nodePort1 + i) % ServiceNodePortRange.Size

? The multiple offsets with subtraction and addition is a little confusing
to follow.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/18532/files#r47307011.

I think it's good enough (I also spent some time thinking why it is correct, but it seems to be).
Anyway - I'm going to manually merge this PR to see if that helps.

If we decide that it's too complicated, we can fix that in a follow-up PR, I guess.