Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

anfernee · 2021-02-21T23:01:53Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
In GCE, the current externalTrafficPolicy: Local logic does not work because the nodes that run the pods do not setup load balancer ports. This means that the GCLB does not understand which nodes are serving the pods that can accept traffic. Since all report unhealthy it'll direct traffic to any node. This PR configures the health check ports so that GCLB knows which nodes can handle the traffic.

See #62046 for details.
This work is based on #96998

Which issue(s) this PR fixes:

Fixes #62046

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

externalTrafficPolicy: local works on Windows hosts with DSR support.

Release Note

Add 2 new options for kube-proxy running in winkernel mode. 
`--forward-healthcheck-vip`, if specified as true, health check traffic whose destination is service VIP will be forwarded to kube-proxy's healthcheck service. `--root-hnsendpoint-name` specifies the name of the hns endpoint for the root network namespace.
This option enables the pass-through load balancers like Google's GCLB to correctly health check the backend services. Without this change, the health check packets is dropped, and Windows node will be considered to be unhealthy by those load balancers.

jeremyje · 2021-02-22T01:41:42Z

/sig windows
/priority important-soon

anfernee · 2021-02-22T22:23:25Z

/retest

anfernee · 2021-02-24T17:31:31Z

@madhanrm @elweb9858 PTAL.

jeremyje · 2021-02-24T18:45:23Z

pkg/proxy/winkernel/proxier.go

+			if gceGatewayEndpoint != nil {
+				hnsHealthCheckLoadBalancer, err := hns.getLoadBalancer(
+					[]endpointsInfo{*gceGatewayEndpoint},
+					loadBalancerFlags{isDSR: svcInfo.preserveDIP || proxier.isDSR, useMUX: svcInfo.preserveDIP, preserveDIP: svcInfo.preserveDIP},


Should the flags here be synced with the flags above?

Actually I am not sure what this annotation means here: preserveDIP := service.Annotations["preserve-destination"] == "true", they are copied over.

The rest is not necessarily synced, because the only targeting client is GCLB health check, the requirement for the service itself doesn't apply to health check.

thockin · 2022-01-29T04:21:06Z

/approve

anfernee · 2022-02-02T06:18:28Z

Thanks everyone for all the help for this PR. Finally almost there. Special thanks for the original author of this PR @jeremyje

anfernee · 2022-02-05T01:53:18Z

When can we merge this PR?

jayunit100 · 2022-02-05T19:06:47Z

/lgtm
/approve

jayunit100 · 2022-02-05T19:07:09Z

Agree it's fine -> Let's merge and keep an eye out

jayunit100 · 2022-02-05T19:08:54Z

Double checking that you got the earlier note: we should make sure our externalTrafficPolicy.... tests are covering stuff like this somehow, eventually, on the sig-win side

anfernee · 2022-02-07T17:29:52Z

Double checking that you got the earlier note: we should make sure our externalTrafficPolicy.... tests are covering stuff like this somehow, eventually, on the sig-win side

I am not exactly familiar with the test setup but I'll check it out. If the test runs on GCE, then we can enable the option there. Existing e2e service test should be able to catch it.

jsturtevant · 2022-03-11T17:54:28Z

/hold cancel

k8s-triage-robot · 2022-03-11T21:01:35Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

anfernee · 2022-03-11T22:17:21Z

/retest

k8s-triage-robot · 2022-03-12T01:22:35Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2022-03-12T04:25:35Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

This change adds 2 options for windows: --forward-healthcheck-vip: If true forward service VIP for health check port --root-hnsendpoint-name: The name of the hns endpoint name for root namespace attached to l2bridge, default is cbr0 When --forward-healthcheck-vip is set as true and winkernel is used, kube-proxy will add an hns load balancer to forward health check request that was sent to lb_vip:healthcheck_port to the node_ip:healthcheck_port. Without this forwarding, the health check from google load balancer will fail, and it will stop forwarding traffic to the windows node. This change fixes the following 2 cases for service: - `externalTrafficPolicy: Cluster` (default option): healthcheck_port is 10256 for all services. Without this fix, all traffic won't be directly forwarded to windows node. It will always go through a linux node and get forwarded to windows from there. - `externalTrafficPolicy: Local`: different healthcheck_port for each service that is configured as local. Without this fix, this feature won't work on windows node at all. This feature preserves client ip that tries to connect to their application running in windows pod. Change-Id: If4513e72900101ef70d86b91155e56a1f8c79719

anfernee · 2022-03-12T06:36:08Z

Fixed pkg/generated/openapi/zz_generated.openapi.go. Pushed again.

k8s-ci-robot · 2022-03-12T06:36:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anfernee, aojea, ibabou, jayunit100, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster/gce/windows/OWNERS~~ [ibabou,thockin]
~~cmd/kube-proxy/OWNERS~~ [aojea,thockin]
~~pkg/generated/openapi/OWNERS~~ [thockin]
~~pkg/proxy/apis/config/OWNERS~~ [aojea,thockin]
~~pkg/proxy/winkernel/OWNERS~~ [aojea,thockin]
~~staging/src/k8s.io/kube-proxy/config/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

anfernee · 2022-03-16T03:25:47Z

This PR requires LGTM again @jayunit100 @jsturtevant

jayunit100 · 2022-03-16T22:46:39Z

/lgtm

anfernee · 2022-03-17T04:55:48Z

/retest

k8s-ci-robot requested review from andrewsykim and ksubrmnn February 21, 2021 23:02

anfernee force-pushed the clientip branch 2 times, most recently from b4e8c9b to 98a3948 Compare February 21, 2021 23:12

k8s-ci-robot added area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 21, 2021

anfernee mentioned this pull request Feb 21, 2021

Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

Closed

jeremyje reviewed Feb 24, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021

anfernee force-pushed the clientip branch 2 times, most recently from 4a102df to 2857436 Compare March 2, 2021 00:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021

marosset added this to Reviewed - Needs Approval From Other SIGs in SIG-Windows Mar 5, 2021

k8s-ci-robot assigned thockin Jan 27, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2022

anfernee force-pushed the clientip branch from 6ac7e3a to 78a507b Compare March 12, 2022 06:35

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2022

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Mar 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2022

k8s-ci-robot merged commit 41b29e6 into kubernetes:master Mar 17, 2022

SIG-Windows automation moved this from In Review (v1.24) to Done (v1.24) Mar 17, 2022

github-actions bot mentioned this pull request Mar 22, 2022

Week Ending March 20, 2022 dev-obs/actus#403

Open

princepereira mentioned this pull request Aug 3, 2022

Mock and unit testcases added for windows kube-proxy. #111031

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

anfernee commented Feb 21, 2021 •

edited

jeremyje commented Feb 22, 2021

anfernee commented Feb 22, 2021

anfernee commented Feb 24, 2021

jeremyje Feb 24, 2021

anfernee Feb 24, 2021

thockin commented Jan 29, 2022

anfernee commented Feb 2, 2022

anfernee commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

anfernee commented Feb 7, 2022

jsturtevant commented Mar 11, 2022

k8s-triage-robot commented Mar 11, 2022

anfernee commented Mar 11, 2022

k8s-triage-robot commented Mar 12, 2022

k8s-triage-robot commented Mar 12, 2022

anfernee commented Mar 12, 2022

k8s-ci-robot commented Mar 12, 2022

anfernee commented Mar 16, 2022

jayunit100 commented Mar 16, 2022

anfernee commented Mar 17, 2022

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

Conversation

anfernee commented Feb 21, 2021 • edited

jeremyje commented Feb 22, 2021

anfernee commented Feb 22, 2021

anfernee commented Feb 24, 2021

jeremyje Feb 24, 2021

Choose a reason for hiding this comment

anfernee Feb 24, 2021

Choose a reason for hiding this comment

thockin commented Jan 29, 2022

anfernee commented Feb 2, 2022

anfernee commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

jayunit100 commented Feb 5, 2022

anfernee commented Feb 7, 2022

jsturtevant commented Mar 11, 2022

k8s-triage-robot commented Mar 11, 2022

anfernee commented Mar 11, 2022

k8s-triage-robot commented Mar 12, 2022

k8s-triage-robot commented Mar 12, 2022

anfernee commented Mar 12, 2022

k8s-ci-robot commented Mar 12, 2022

anfernee commented Mar 16, 2022

jayunit100 commented Mar 16, 2022

anfernee commented Mar 17, 2022

anfernee commented Feb 21, 2021 •

edited