Manual cherrypick of part of #44053 to release-1.6

[thockin]

This is a manual application of git commit 7664b97 to release-1.6 branch, because the context leading up to that commit was a lot of churn not necessary for the surgical fix. We have at least one user report of hitting this.

Fix for kube-proxy healthcheck handling an update that simultaneously removes one port and adds another.

@ethernetdan are you patch czar for 1.6 ?

[k8s-reviewable]

This change is 

[thockin]

@MrHohn @freehan there is significantly different context for 1.5 - do either of you have time to help me backport this? Or should we not try? Feedback welcome..

[MrHohn]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, thockin

Needs approval from an approver in each of these OWNERS Files:

• pkg/proxy/OWNERS [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[MrHohn]

there is significantly different context for 1.5 - do either of you have time to help me backport this? Or should we not try?

Let me give it a try.

[MrHohn]

Took a look at the codes, it seems a bit risky to cherrypick the whole commit (7664b97) into 1.5, given recent overhauls on kube-proxy. Specifically, for backporting the new added healthCheck update tests, we have at least these dependencies:

• Refactor OnEndpointsUpdate for testing (9507af3)
• Add tests for updateEndpoints (6069d49)
• Make healthcheck an interface (cddda17)

However, if we only cherrypick the proxier.go part (which basically just changes map[proxy.ServicePortName]bool to map[types.NamespacedName][]*endpointsInfo{}), it seems simple enough and sufficient for the bug fix.

What do you think? @thockin @freehan

[thockin]

That's even more surgical, but it means net-new code. If we port an equivalent testcase it could fly.

…

On Sun, Apr 9, 2017 at 11:22 PM, Zihong Zheng ***@***.***> wrote: Took a look at the codes, it seems a bit risky to cherrypick the whole commit (7664b97 <7664b97>) into 1.5, given recent overhauls on kube-proxy. Specifically, for backporting the new added healthCheck update tests, we have at least these dependencies: - Refactor OnEndpointsUpdate for testing (9507af3 <9507af3> ) - Add tests for updateEndpoints (6069d49 <6069d49> ) - Make healthcheck an interface (cddda17 <cddda17> ) However, if we only cherrypick the proxier.go part (which basically just changes map[proxy.ServicePortName]bool to map[types.NamespacedName][]* endpointsInfo{}), it seems simple enough and sufficient for the bug fix. What do you think? @thockin <https://github.com/thockin> @freehan <https://github.com/freehan> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44246 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVN_OGZ_Kd072SsU2JnkKElsBzR8aks5rucq0gaJpZM4M31_p> .

[enisoc]

@thockin I tried to reword the release note in terms of what a user might observe. Please edit it if I've mangled the meaning.

[k8s-github-robot]

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue

[k8s-cherrypick-bot]

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

[MrHohn]

Quick update, just reproduced the same issue on with latest 1.6 codes:

• Create a LoadBalancer Service with OnlyLocal annotation on it (and the backends):

apiVersion: v1
kind: Service
metadata:
  name: my-nginx
  labels:
    run: my-nginx
  annotations:
    service.beta.kubernetes.io/external-traffic: OnlyLocal
spec:
  selector:
    run: my-nginx
  ports:
  - name: "port1"
    port: 80
    protocol: TCP
  type: LoadBalancer

• Rename the port name from "port1" to "port2"
• kube-proxy stops servicing health check for this service on nodes.

Dug a bit into it, found the bug is triggered by OnServiceUpdate, but this cherrypick only fixes OnEndpointUpdate :(

[thockin]

We may want to roll this back, depending on what the real fix to the fix is. This alone is not detrimental, but it seems it is incomplete.

[cblecker]

@MrHohn @thockin Do we need an additional PR to fix OnServiceUpdate on the 1.6 branch too?

[thockin]

#44315 covered it.