Keep the cached Endpoints the same as those installed in OVS #4691

hongliangl · 2023-03-08T16:31:17Z

The main purpose of this PR is to avoid potential inconsistencies between
the cached Endpoints and those installed in OVS, like #4681, #4692.

This PR also updates:

Method UninstallEndpointFlows of ofClient, support deleting flows of
multiple Endpoints.
Remove possible groups when a Service is deleted.
Log something when a group for a Service is not created.
Optimize and unify log.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

pkg/agent/openflow/client.go

antoninbas · 2023-03-08T17:23:20Z

pkg/agent/proxy/proxier.go

+		// Remove NodePort flows and configurations.
+		if p.proxyAll && svcInfo.NodePort() > 0 {
+			if err := p.uninstallNodePortService(uint16(svcInfo.NodePort()), svcInfo.OFProtocol); err != nil {
+				klog.ErrorS(err, "Failed to remove flows and configurations of Service", "Service", svcPortName)


this error log sounds quite generic, given that this case is specific to NodePort Services?

Updated with

klog.ErrorS(err, "Error when uninstalling NodePort flows and configurations for Service", "ServiceName", svcPortName, "ServiceInfo", svcInfoStr)

antoninbas · 2023-03-08T17:24:36Z

pkg/agent/proxy/proxier.go

-					continue
-				}
-				p.groupCounter.Recycle(svcPortName, true)
+		// Remove Service group which has only local Endpoints.


the svcInfo.ExternalPolicyLocal() condition was removed, yet the commend still mentions "local Endpoints". Is that correct?

Yes, when svcInfo.InternalPolicyLocal() is true, the group has only local Endpoints too.

So there was a bug regarding this? a group may never be deleted?

So there was a bug regarding this? a group may never be deleted?

For a ClusterIP, when svcInfo.InternalPolicyLocal() is true, a group may never be delete.

antoninbas · 2023-03-08T17:28:08Z

pkg/agent/proxy/proxier.go

+			updatedEndpointsInstalled := map[string]k8sproxy.Endpoint{}
+			for _, endpoint := range allReachableEndpoints {
+				updatedEndpointsInstalled[endpoint.String()] = endpoint
+				// If the Endpoint is not newly installed, remove it from stale endpoints.


"remove it from stale endpoints": that doesn't sound correct. You are removing it from endpointsInstalled, which doesn't sound like a list of stale endpoints

I have rephrased the comments here.

antoninbas · 2023-03-08T17:28:55Z

pkg/agent/proxy/proxier.go

+			if mcsLocalService != nil {
+				allReachableEndpoints = append(allReachableEndpoints, mcsLocalService.Endpoint)
+			}
+


IMO, this function (installServices) is getting very long. Is there a way to extract some functionality into helper functions?

Yes, it is quite long for this function, I thought of extracting the code under if needUpdateEndpoints and if needUpdateService into functions, but I am not sure if this is an appropriate idea.

tnqn · 2023-03-09T13:20:33Z

pkg/agent/proxy/proxier.go

-					continue
-				}
-				p.groupCounter.Recycle(svcPortName, true)
+		// Remove Service group which has only local Endpoints.


So there was a bug regarding this? a group may never be deleted?

pkg/agent/proxy/proxier.go

pkg/agent/openflow/client.go

pkg/agent/route/route_linux.go

pkg/agent/proxy/proxier.go

wenyingd · 2023-03-15T07:09:57Z

pkg/agent/proxy/proxier.go

 					continue
 				}

 				// If previous Service is nil or NodePort flows and configurations of previous Service have been removed,
 				// install NodePort flows and configurations for current Service.
 				if svcInfo.NodePort() > 0 && (pSvcInfo == nil || needRemoval) {
-					if err := p.installNodePortService(nGroupID, uint16(svcInfo.NodePort()), svcInfo.OFProtocol, uint16(affinityTimeout), svcInfo.ExternalPolicyLocal()); err != nil {
-						klog.ErrorS(err, "Failed to install NodePort flows and configurations of Service", "Service", svcPortName)
+					groupID, exists = p.groupCounter.Get(svcPortName, externalPolicyLocal)


Question: why do you expect the OF group for the NodePort traffic to share the same identifier as the ClusterIP traffic for a Service? In the previous implementation, here calls p.groupCounter.AllocateIfNotExist to get or create an OF group ID for the new configured NodePort scenario.

In the previous implementation, here calls p.groupCounter.AllocateIfNotExist to get or create an OF group ID for the new configured NodePort scenario.

We should use method Get here to get a OVS group ID, otherwise, there is a chance that we allocate a group ID and pass it to InstallServiceGroup and the corresponding group was not created in OVS.

wenyingd · 2023-03-15T08:59:15Z

pkg/agent/proxy/proxier.go

 		if err := p.ofClient.UninstallServiceFlows(svcInfo.ClusterIP(), uint16(svcInfo.Port()), svcInfo.OFProtocol); err != nil {
-			klog.ErrorS(err, "Failed to remove flows of Service", "Service", svcPortName)
+			klog.ErrorS(err, "Error when uninstalling ClusterIP flows for Service", "ServiceName", svcPortName, "ServiceInfo", svcInfoStr)


The key "ServiceName" in such logs should be "servicePortName"?
BTW, the keys in the logs should not start with a capital according to the logs in other package?

The key "ServiceName" in such logs should be "servicePortName"?

I'll change that.

BTW, the keys in the logs should not start with a capital according to the logs in other package?

I thought that Service in ServiceName means Kubernetes Service, and I capitalized them all.

The main purpose of this PR is to avoid potential inconsistencies between the cached Endpoints and those installed in OVS, like antrea-io#4681, antrea-io#4692. This PR also updates: - Method UninstallEndpointFlows of ofClient, support deleting flows of multiple Endpoints. - Remove possible groups when a Service is deleted. - Log something when a group for a Service is not created. - Optimize and unify log. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

hongliangl · 2023-03-15T12:46:16Z

Thanks for reviewing and approving @tnqn @wenyingd

hongliangl · 2023-03-15T12:46:34Z

/test-all

tnqn · 2023-03-16T04:59:13Z

@wenyingd do you have other comments?

wenyingd · 2023-03-16T06:39:53Z

@wenyingd do you have other comments?

No other comments

…io#4691) The main purpose of this PR is to avoid potential inconsistencies between the cached Endpoints and those installed in OVS, like antrea-io#4681, antrea-io#4692. This PR also updates: - Method UninstallEndpointFlows of ofClient, support deleting flows of multiple Endpoints. - Remove possible groups when a Service is deleted. - Log something when a group for a Service is not created. - Optimize and unify log. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl added the area/proxy Issues or PRs related to proxy functions in Antrea label Mar 8, 2023

hongliangl force-pushed the 20230308-fix-proxy branch 2 times, most recently from 7ed1fb7 to 2a1a6db Compare March 8, 2023 17:13

hongliangl requested review from tnqn, antoninbas and jianjuns March 8, 2023 17:13

antoninbas reviewed Mar 8, 2023

View reviewed changes

hongliangl mentioned this pull request Mar 8, 2023

Graduate feature EndpointSlice from Alpha to Beta #4634

Merged

hongliangl force-pushed the 20230308-fix-proxy branch from 2a1a6db to 96bb35d Compare March 9, 2023 02:16

tnqn reviewed Mar 9, 2023

View reviewed changes

hongliangl force-pushed the 20230308-fix-proxy branch from 96bb35d to 6913627 Compare March 9, 2023 16:12

tnqn added this to the Antrea v1.11 release milestone Mar 10, 2023

hongliangl force-pushed the 20230308-fix-proxy branch 2 times, most recently from 597b534 to 5ed391b Compare March 14, 2023 13:40

tnqn reviewed Mar 14, 2023

View reviewed changes

hongliangl force-pushed the 20230308-fix-proxy branch 2 times, most recently from 54f15f9 to 6ab1d39 Compare March 14, 2023 17:16

tnqn reviewed Mar 15, 2023

View reviewed changes

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230308-fix-proxy branch 3 times, most recently from fcde930 to b053114 Compare March 15, 2023 04:41

wenyingd reviewed Mar 15, 2023

View reviewed changes

hongliangl force-pushed the 20230308-fix-proxy branch from b053114 to 88a4812 Compare March 15, 2023 09:28

tnqn approved these changes Mar 15, 2023

View reviewed changes

tnqn merged commit 4a80363 into antrea-io:main Mar 16, 2023

hongliangl deleted the 20230308-fix-proxy branch April 4, 2023 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep the cached Endpoints the same as those installed in OVS #4691

Keep the cached Endpoints the same as those installed in OVS #4691

hongliangl commented Mar 8, 2023 •

edited

Loading

antoninbas Mar 8, 2023

hongliangl Mar 9, 2023

antoninbas Mar 8, 2023

hongliangl Mar 9, 2023

tnqn Mar 9, 2023

hongliangl Mar 9, 2023

antoninbas Mar 8, 2023

hongliangl Mar 9, 2023 •

edited

Loading

antoninbas Mar 8, 2023

hongliangl Mar 9, 2023

tnqn Mar 9, 2023

wenyingd Mar 15, 2023

hongliangl Mar 15, 2023

wenyingd Mar 15, 2023

hongliangl Mar 15, 2023

tnqn left a comment

hongliangl commented Mar 15, 2023

hongliangl commented Mar 15, 2023

tnqn commented Mar 16, 2023

wenyingd commented Mar 16, 2023

Keep the cached Endpoints the same as those installed in OVS #4691

Keep the cached Endpoints the same as those installed in OVS #4691

Conversation

hongliangl commented Mar 8, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Mar 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

hongliangl commented Mar 15, 2023

hongliangl commented Mar 15, 2023

tnqn commented Mar 16, 2023

wenyingd commented Mar 16, 2023

hongliangl commented Mar 8, 2023 •

edited

Loading

hongliangl Mar 9, 2023 •

edited

Loading