Implement service health check in Antrea-agent #4120

shettyg · 2022-08-16T16:54:06Z

antrea-agent: Implement service health check.

When services are created with "externalTrafficPolicy: Local",
a "healthCheckNodePort" is created in the k8s service object.
kube-proxy in turn will listen on this port and answer queries
on http://0.0.0.0:healthCheckNodePort/healthz". In kube-proxy
replacement mode, antrea does not support this feature. This
becomes more important for Windows support as userspace
kube-proxy is being deprecated.

This commit implements this feature in Antrea and is inspired
by the same feature in upstream kube-proxy with some differences.
The predominant difference is that upstream kube-proxy goes
through all endpoints of cluster in each iteration of
endpoints:update() to find the local endpoints. This has been
changed here to only look for changed endpoints.

shettyg · 2022-08-16T16:55:03Z

CC: @jianjuns @hongliangl For any early comment on the general direction/approach taken here.

codecov · 2022-08-16T17:09:15Z

Codecov Report

Merging #4120 (0b95eee) into main (d1c6a43) will decrease coverage by 9.37%.
The diff coverage is 71.03%.

❗ Current head 0b95eee differs from pull request most recent head faa2f6b. Consider uploading reports for the commit faa2f6b to get more accurate results

@@            Coverage Diff             @@
##             main    #4120      +/-   ##
==========================================
- Coverage   65.70%   56.33%   -9.38%     
==========================================
  Files         304      371      +67     
  Lines       46604    53558    +6954     
==========================================
- Hits        30621    30170     -451     
- Misses      13557    21037    +7480     
+ Partials     2426     2351      -75

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`39.40% <28.46%> (?)`
integration-tests	`34.97% <49.35%> (+0.02%)`	⬆️	Carriedforward from 3714619
kind-e2e-tests	`43.05% <48.52%> (-6.30%)`	⬇️	Carriedforward from 3714619
unit-tests	`40.27% <51.96%> (-4.11%)`	⬇️	Carriedforward from 3714619

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
cmd/antrea-agent/options.go	`20.83% <0.00%> (ø)`
cmd/antrea-controller/controller.go	`0.00% <0.00%> (ø)`
cmd/antrea-controller/options.go	`0.00% <0.00%> (ø)`
cmd/flow-aggregator/flow-aggregator.go	`0.00% <0.00%> (ø)`
...icluster/controllers/multicluster/common/helper.go	`85.29% <ø> (+27.29%)`	⬆️
...uster/commonarea/acnp_resourceimport_controller.go	`0.00% <0.00%> (-64.71%)`	⬇️
...llers/multicluster/leader_clusterset_controller.go	`0.00% <ø> (-56.04%)`	⬇️
pkg/agent/agent.go	`54.43% <ø> (+4.09%)`	⬆️
pkg/agent/apiserver/apiserver.go	`67.02% <0.00%> (ø)`
... and 267 more

jianjuns · 2022-08-17T00:37:53Z

pkg/agent/proxy/proxier.go


 	p.removeStaleServices()
 	p.installServices()
 	p.removeStaleEndpoints()

+	if p.serviceHealthServer != nil {
+		if err := p.serviceHealthServer.SyncServices(serviceUpdateResult.HCServiceNodePorts); err != nil {
+			klog.ErrorS(err, "Error syncing healthcheck services")


FYI - we use CamelCase for resource/CRD kinds, so it should be "Services", "Endpoints".

jianjuns · 2022-08-17T00:39:21Z

Thanks for working on this. I have not done detailed review yet, but the approach looks good to me after a quick look.

tnqn

@shettyg thanks for the PR. I have two comments, otherwise LGTM.

tnqn · 2022-08-18T16:00:11Z

third_party/proxy/healthcheck/service_health.go

+	"sync"
+
+	"github.com/lithammer/dedent"
+	"k8s.io/klog"


Antrea uses klog/v2, could you change it to "k8s.io/klog/v2" so the patch won't add "k8s.io/klog v1.0.0" back in go.mod. The upstream code have also switched to v2: https://github.com/kubernetes/kubernetes/blob/58c10aa6eb5adfb1f3aa4d6cb898b8c347ba9e72/pkg/proxy/healthcheck/service_health.go#L28

I decided to import healthcheck from 1.24.4 tree as it uses klog/v2 and supports dualstack

tnqn · 2022-08-18T16:54:17Z

third_party/proxy/healthcheck/service_health.go

+		addr := fmt.Sprintf(":%d", port)
+		svc.server = hcs.httpFactory.New(addr, hcHandler{name: nsn, hcs: hcs})


I think this doesn't work in dual-stack clusters as the same address and port will be opened twice, the second attemp would fail and cause error logs.

Should we pass an ipv6 boolean or an bindAddress string when calling newServiceHealthServer so each stack of Proxier only listens to its own address?

Thanks for catching this. I decided to import healthcheck from 1.24.4 tree as it supports dualstack. Please take a look again.

hongliangl

Overall LGTM, maybe you could add an e2e test to verify the feature.

third_party/proxy/healthcheck/service_health.go

shettyg · 2022-08-24T20:43:29Z

@tnqn @hongliangl PTAL again (I am trying to see what is the best way to add a unit test here, making myself familiar with the type of tests that currently exist)

shettyg · 2022-08-25T02:58:35Z

pkg/agent/proxy/proxier.go

+		nodePortAddressesString := make([]string, len(nodePortAddresses))
+		for i, address := range nodePortAddresses {
+			if isIPv6 {
+				nodePortAddressesString[i] = fmt.Sprintf("%s/128", address.String())


Not sure whether passing /128 is fine. I will test this.

tnqn · 2022-08-25T09:37:57Z

pkg/agent/proxy/proxier.go

+		nodePortAddressesString := make([]string, len(nodePortAddresses))
+		for i, address := range nodePortAddresses {
+			if isIPv6 {
+				nodePortAddressesString[i] = fmt.Sprintf("%s/128", address.String())
+			} else {
+				nodePortAddressesString[i] = fmt.Sprintf("%s/32", address.String())
+			}


There is some difference between the nodePortAddresses argument of Antrea's proxy and kube-proxy: the former is already the concrete Node IPs that this Node is configured with:

antrea/cmd/antrea-agent/agent.go

Lines 215 to 221 in 52db699

var nodePortAddressesIPv4, nodePortAddressesIPv6 []net.IP

if o.config.AntreaProxy.ProxyAll {

nodePortAddressesIPv4, nodePortAddressesIPv6, err = getAvailableNodePortAddresses(o.config.AntreaProxy.NodePortAddresses, append(excludeNodePortDevices, o.config.HostGateway))

if err != nil {

return fmt.Errorf("getting available NodePort IP addresses failed: %v", err)

}

}

while the latter is the user provided CIDRs that will be used to filter concrete Node IPs.

Since service health server actually needs the former, I think we could pass the slice of node IP strings to it directly, instead of passing the slice of CIDRs and calling utilproxy.GetNodeAddresses(nodePortAddresses, utilproxy.RealNetwork{}) to get concrete Node IPs again, which should just get the same results.

In this way, the changes to util/network.go and util/utils.go are not needed.

There will be a small defect that unlike kube-proxy listening to "0.0.0.0"/"::0" when user provided nodeAddresses is empty, antrea proxy will always listen to concrete node IPs because the nodePortAddresses argument are used for other purposes which need the IPs to be concrete ones (for checking whether a packet is accessing a Node IP). But it shouldn't affect the functionality of service health check so should be ok at the moment.

Thanks for the insight!

tnqn · 2022-08-25T10:32:20Z

@tnqn @hongliangl PTAL again (I am trying to see what is the best way to add a unit test here, making myself familiar with the type of tests that currently exist)

Since most of the implementation code of service health server is from K8s which should already be verified, maybe you could add a verification about service health port to existing Antrea e2e test for LB service with Local ExternalTrafficPolicy that we can get expected output by accessing the service health port of each Node:

antrea/test/e2e/proxy_test.go

Lines 228 to 230 in 7afa4fb

    
           t.Run("ExternalTrafficPolicy:Local/Client:Node", func(t *testing.T) { 
        
           	testLoadBalancerLocalFromNode(t, data, nodes, localUrl, hostnames) 
        
           })

These APIs will be used in an upcoming commit to provide service health check (to support externalTrafficPolicy: Local for a k8s service. Signed-off-by: Guru Shetty <[email protected]>

When services are created with "externalTrafficPolicy: Local", a "healthCheckNodePort" is created in the k8s service object. kube-proxy in turn will listen on this port and answer queries on http://0.0.0.0:healthCheckNodePort/healthz". In kube-proxy replacement mode, antrea does not support this feature. This becomes more important for Windows support as userspace kube-proxy is being deprecated. This commit implements this feature in Antrea and is inspired by the same feature in upstream kube-proxy with some differences. The predominant difference is that upstream kube-proxy goes through all endpoints of cluster in each iteration of endpoints:update() to find the local endpoints. This has been changed here to only look for changed endpoints. Signed-off-by: Guru Shetty <[email protected]>

shettyg · 2022-08-30T17:33:18Z

@tnqn I added a e2e test and updated the service health library to reflect your suggestions. PTAL again

tnqn

LGTM, thanks @shettyg

tnqn · 2022-08-31T14:20:08Z

~~/skip-all~~ (typed by mistake)

tnqn · 2022-08-31T14:20:36Z

@jianjuns @hongliangl do you have further comments?

tnqn · 2022-08-31T14:20:53Z

/test-all

jianjuns · 2022-08-31T21:21:17Z

@jianjuns @hongliangl do you have further comments?

No further comment from me.

hongliangl

LGTM

When services are created with "externalTrafficPolicy: Local", a "healthCheckNodePort" is created in the k8s service object. kube-proxy in turn will listen on this port and answer queries on http://0.0.0.0:healthCheckNodePort/healthz". In kube-proxy replacement mode, antrea does not support this feature. This becomes more important for Windows support as userspace kube-proxy is being deprecated. This commit implements this feature in Antrea and is inspired by the same feature in upstream kube-proxy with some differences. The predominant difference is that upstream kube-proxy goes through all endpoints of cluster in each iteration of endpoints:update() to find the local endpoints. This has been changed here to only look for changed endpoints. Signed-off-by: Guru Shetty <[email protected]>

jianjuns reviewed Aug 17, 2022

View reviewed changes

jianjuns requested a review from hongliangl August 17, 2022 00:38

jianjuns added the area/proxy Issues or PRs related to proxy functions in Antrea label Aug 17, 2022

shettyg force-pushed the main branch 2 times, most recently from 8752281 to 587ca05 Compare August 17, 2022 22:09

tnqn reviewed Aug 18, 2022

View reviewed changes

tnqn added this to the Antrea v1.9 release milestone Aug 18, 2022

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Aug 18, 2022

hongliangl reviewed Aug 19, 2022

View reviewed changes

third_party/proxy/healthcheck/service_health.go Outdated Show resolved Hide resolved

shettyg force-pushed the main branch from 587ca05 to 8382290 Compare August 24, 2022 20:40

shettyg force-pushed the main branch from 8382290 to 3396912 Compare August 25, 2022 02:51

shettyg commented Aug 25, 2022

View reviewed changes

tnqn reviewed Aug 25, 2022

View reviewed changes

Guru Shetty added 2 commits August 30, 2022 16:44

Import service health code from upstream 1.24.4 tree.

e325ad2

These APIs will be used in an upcoming commit to provide service health check (to support externalTrafficPolicy: Local for a k8s service. Signed-off-by: Guru Shetty <[email protected]>

shettyg force-pushed the main branch from 3396912 to faa2f6b Compare August 30, 2022 17:31

tnqn approved these changes Aug 31, 2022

View reviewed changes

hongliangl approved these changes Sep 1, 2022

View reviewed changes

tnqn merged commit 7cf5277 into antrea-io:main Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement service health check in Antrea-agent #4120

Implement service health check in Antrea-agent #4120

shettyg commented Aug 16, 2022

shettyg commented Aug 16, 2022

codecov bot commented Aug 16, 2022 •

edited

Loading

jianjuns Aug 17, 2022

shettyg Aug 24, 2022

jianjuns commented Aug 17, 2022

tnqn left a comment

tnqn Aug 18, 2022

shettyg Aug 24, 2022

tnqn Aug 18, 2022

shettyg Aug 24, 2022

hongliangl left a comment

shettyg commented Aug 24, 2022

shettyg Aug 25, 2022

tnqn Aug 25, 2022

tnqn Aug 25, 2022 •

edited

Loading

shettyg Aug 25, 2022

tnqn commented Aug 25, 2022

shettyg commented Aug 30, 2022

tnqn left a comment

tnqn commented Aug 31, 2022 •

edited

Loading

tnqn commented Aug 31, 2022

tnqn commented Aug 31, 2022

jianjuns commented Aug 31, 2022

hongliangl left a comment

		addr := fmt.Sprintf(":%d", port)
		svc.server = hcs.httpFactory.New(addr, hcHandler{name: nsn, hcs: hcs})

	var nodePortAddressesIPv4, nodePortAddressesIPv6 []net.IP
	if o.config.AntreaProxy.ProxyAll {
	nodePortAddressesIPv4, nodePortAddressesIPv6, err = getAvailableNodePortAddresses(o.config.AntreaProxy.NodePortAddresses, append(excludeNodePortDevices, o.config.HostGateway))
	if err != nil {
	return fmt.Errorf("getting available NodePort IP addresses failed: %v", err)
	}
	}

Implement service health check in Antrea-agent #4120

Implement service health check in Antrea-agent #4120

Conversation

shettyg commented Aug 16, 2022

shettyg commented Aug 16, 2022

codecov bot commented Aug 16, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Aug 17, 2022

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl left a comment

Choose a reason for hiding this comment

shettyg commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Aug 25, 2022

shettyg commented Aug 30, 2022

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 31, 2022 • edited Loading

tnqn commented Aug 31, 2022

tnqn commented Aug 31, 2022

jianjuns commented Aug 31, 2022

hongliangl left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2022 •

edited

Loading

tnqn Aug 25, 2022 •

edited

Loading

tnqn commented Aug 31, 2022 •

edited

Loading