Implement NPL agent unification. #3936

XinShuYang · 2022-06-24T02:41:00Z

Unify agent behavior across Linux and Windows. Linux agent should support
allocating different nodeports for different protocols when the podports are the same.
Update port allocation related unit tests.
Enable windows e2e test.
Delete unused functions.

Signed-off-by: Shuyang Xin gavinx@vmware.com

XinShuYang · 2022-06-24T02:41:21Z

/test-all

codecov-commenter · 2022-06-24T03:16:50Z

Codecov Report

Merging #3936 (0a20df2) into main (4b788e7) will increase coverage by 3.45%.
The diff coverage is 51.64%.

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

@@            Coverage Diff             @@
##             main    #3936      +/-   ##
==========================================
+ Coverage   61.42%   64.87%   +3.45%     
==========================================
  Files         296      294       -2     
  Lines       43864    44520     +656     
==========================================
+ Hits        26942    28882    +1940     
+ Misses      14672    13315    -1357     
- Partials     2250     2323      +73

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`40.72% <17.17%> (?)`
kind-e2e-tests	`50.91% <21.29%> (+6.41%)`	⬆️	Carriedforward from 2a37aec
unit-tests	`44.35% <44.62%> (+0.10%)`	⬆️	Carriedforward from 2a37aec

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

Impacted Files	Coverage Δ
...icluster/cmd/multicluster-controller/controller.go	`7.86% <0.00%> (-0.47%)`	⬇️
multicluster/cmd/multicluster-controller/leader.go	`0.00% <0.00%> (ø)`
multicluster/cmd/multicluster-controller/main.go	`0.00% <0.00%> (ø)`
...uster/controllers/multicluster/controller_utils.go	`26.66% <ø> (-0.89%)`	⬇️
pkg/agent/openflow/client.go	`68.30% <0.00%> (+2.05%)`	⬆️
pkg/agent/openflow/multicast.go	`0.00% <0.00%> (ø)`
pkg/agent/openflow/pipeline.go	`73.13% <0.00%> (+2.67%)`	⬆️
pkg/agent/util/iptables/iptables.go	`43.52% <ø> (ø)`
pkg/ovs/openflow/ofctrl_group.go	`47.82% <0.00%> (-2.95%)`	⬇️
pkg/agent/route/route_linux.go	`45.70% <7.14%> (+16.18%)`	⬆️
... and 94 more

XinShuYang · 2022-06-28T07:53:16Z

/test-all

XinShuYang · 2022-06-29T04:59:11Z

/test-all
/test-windows-all

XinShuYang · 2022-06-29T16:06:07Z

/test-all

pkg/agent/nodeportlocal/npl_agent_test.go

pkg/agent/nodeportlocal/portcache/port_table.go

pkg/agent/nodeportlocal/portcache/port_table_linux.go

XinShuYang · 2022-07-15T06:56:38Z

/test-all
/test-windows-all

pkg/agent/nodeportlocal/portcache/port_table.go

pkg/agent/nodeportlocal/portcache/port_table_linux.go

pkg/agent/nodeportlocal/portcache/port_table_windows.go

pkg/agent/nodeportlocal/portcache/port_table.go

pkg/agent/nodeportlocal/portcache/port_table_linux.go

XinShuYang · 2022-07-22T02:27:14Z

/test-all
/test-windows-all

XinShuYang · 2022-08-01T06:02:39Z

/test-windows-proxyall-e2e

XinShuYang · 2022-08-02T01:10:43Z

/test-all
/test-windows-all

XinShuYang · 2022-08-02T01:54:32Z

/test-all
/test-windows-all

XinShuYang · 2022-08-02T16:07:14Z

Hi @antoninbas , do you have more comments for this PR?

pkg/agent/nodeportlocal/npl_agent_test.go

antoninbas · 2022-08-02T23:22:18Z

pkg/agent/nodeportlocal/npl_agent_test.go

-// irrespective of which protocol is in use.
+// In particular we make sure that a given NodePort is never used by more than one Pod.
+// One Pod could use multiple Nodeports for different protocol with the same Pod port
+// because of the new NPL unification implementation.


that may not be a useful precision

antoninbas · 2022-08-02T23:26:28Z

pkg/agent/nodeportlocal/npl_agent_test.go

+// TestNodePortAlreadyBoundTo validates that when a port with TCP protocol is already bound to,
+// the same port should be selected for NPL if any other protocol is available.


this comment is incorrect.

the same port should be selected for NPL

we don't use the same port

pkg/agent/nodeportlocal/npl_agent_test.go

pkg/agent/nodeportlocal/portcache/port_table.go

antoninbas · 2022-08-02T23:33:22Z

pkg/agent/nodeportlocal/portcache/port_table.go

@@ -20,9 +20,15 @@ import (
 	"net"
 	"sync"

-	"k8s.io/klog/v2"


removing klog seems to be causing build issues?

Yes, it should not be removed.

antoninbas · 2022-08-02T23:34:24Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

-func (pt *PortTable) getEntryByPodIPPortProto(ip string, port int, protocol string) *NodePortData {
-	return pt.PodEndpointTable[podIPPortProtoFormat(ip, port, protocol)]
-}
-
 func (pt *PortTable) GetEntry(ip string, port int, protocol string) *NodePortData {


This function is duplicated for the Linux and Windows implementations I think. I don't a difference between the 2 versions.

This function has been moved to port_table.go. So both linux and windows implementation can call it.

Have you made this change, I don't see it?

antoninbas · 2022-08-02T23:37:38Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

 	var protocolSocketData *ProtocolSocketData
-	protocolSocketData = &data.Protocols[0]
+	protocolSocketData = &data.Protocol
 	if protocolSocketData != nil {


I don't understand what we need protocolSocketData for here. I don't see similar logic in the Linux version?

This data structure is shared by both linux and windows. Do you think we need to build a new data structure without socket variable for windows?

Sorry, I don't understand your reply.

Let me rephrase my question. This is the corresponding code for Linux:

if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort, protocol); err != nil { return err }

why do we need to check protocolSocketData / data.Protocol for Windows:

var protocolSocketData *ProtocolSocketData protocolSocketData = &data.Protocols[0] protocolSocketData = &data.Protocol if protocolSocketData != nil { if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort, protocol); err != nil { return err } }

we don't seem to actually use protocolSocketData

is it even possible for protocolSocketData to be nil? I am not sure, I don't see any other place where we check for this.

@antoninbas I see, yes, protocolSocketData will never be nil if we can successfully get entry from cache table. I have removed it, thanks.

antoninbas · 2022-08-02T23:39:45Z

pkg/agent/nodeportlocal/portcache/port_table_linux.go

+		pt.addPortTableCache(npData)
+	} else {
+		// Only add rules for if the entry does not exist.
+		return 0, fmt.Errorf("existed windows nodeport entry for %s:%d:%s", podIP, podPort, protocol)


antoninbas · 2022-08-02T23:40:11Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

 		}

-		pt.NodePortTable[NodePortProtoFormat(nodePort, protocol)] = npData
-		pt.PodEndpointTable[podIPPortProtoFormat(podIP, podPort, protocol)] = npData
+		pt.addPortTableCache(npData)
 	} else {
 		// Only add rules for if the entry does not exist.
 		return 0, fmt.Errorf("existed windows nodeport entry for %s:%d:%s", podIP, podPort, protocol)


Suggested change

return 0, fmt.Errorf("existed windows nodeport entry for %s:%d:%s", podIP, podPort, protocol)

return 0, fmt.Errorf("existing Windows NodePort entry for %s:%d:%s", podIP, podPort, protocol)

Updated, thanks.

XinShuYang · 2022-08-04T23:16:25Z

/test-all
/test-windows-all

antoninbas · 2022-08-04T23:57:21Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

 	var protocolSocketData *ProtocolSocketData
-	protocolSocketData = &data.Protocols[0]
+	protocolSocketData = &data.Protocol
 	if protocolSocketData != nil {


Sorry, I don't understand your reply.

Let me rephrase my question. This is the corresponding code for Linux:

if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort, protocol); err != nil { return err }

why do we need to check protocolSocketData / data.Protocol for Windows:

var protocolSocketData *ProtocolSocketData protocolSocketData = &data.Protocols[0] protocolSocketData = &data.Protocol if protocolSocketData != nil { if err := pt.PodPortRules.DeleteRule(data.NodePort, podIP, podPort, protocol); err != nil { return err } }

we don't seem to actually use protocolSocketData

is it even possible for protocolSocketData to be nil? I am not sure, I don't see any other place where we check for this.

antoninbas · 2022-08-05T00:00:40Z

pkg/agent/nodeportlocal/npl_agent_test.go

-// TestNodePortAlreadyBoundTo validates that when a port is already bound to, a different port will
-// be selected for NPL.
+// TestNodePortAlreadyBoundTo validates that when a port with TCP protocol is already bound to,
+// the next port should be selected for NPL if the same protocol is available.


Suggested change

// the next port should be selected for NPL if the same protocol is available.

// the next sequential TCP port should be selected for NPL when it is available.

antoninbas · 2022-08-05T00:02:33Z

pkg/agent/nodeportlocal/portcache/port_table_linux.go

-			socket:   socket,
-		})
+func openSocketsForPort(localPortOpener LocalPortOpener, port int, protocol string) (ProtocolSocketData, error) {
+	// Port needs to be only available for the protocol used by NPL rule.


Suggested change

// Port needs to be only available for the protocol used by NPL rule.

// Port only needs to be available for the protocol used by the NPL rule.

antoninbas · 2022-08-05T00:03:35Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

-func (pt *PortTable) getEntryByPodIPPortProto(ip string, port int, protocol string) *NodePortData {
-	return pt.PodEndpointTable[podIPPortProtoFormat(ip, port, protocol)]
-}
-
 func (pt *PortTable) GetEntry(ip string, port int, protocol string) *NodePortData {


Have you made this change, I don't see it?

antoninbas · 2022-08-05T00:04:21Z

pkg/agent/nodeportlocal/portcache/port_table.go

+	return nil
+}
+
+func (pt *PortTable) delPortTableCache(npData *NodePortData) error {


s/del/delete

XinShuYang · 2022-08-05T00:50:13Z

pkg/agent/nodeportlocal/portcache/port_table.go

+	return fmt.Sprintf("%s:%d:%s", ip, port, protocol)
+}
+
+func (pt *PortTable) getEntryByPodIPPortProto(ip string, port int, protocol string) *NodePortData {


Have you made this change, I don't see it?
@antoninbas I moved getEntryByPodIPPortProto here.

I was talking about GetEntry

My bad. Move it to port_table.go, thanks.

* Unify agent behavior across Linux and Windows. Linux agent should support allocating different nodeports for different protocols when the podports are the same. * Replace map with cache.indexer for cachetable to reduce repeated insertion. * Update port allocation related unit tests. * Enable windows e2e test. * Delete unused functions. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

XinShuYang · 2022-08-05T10:07:28Z

/test-all
/test-windows-all

antoninbas

LGTM

XinShuYang · 2022-08-06T01:29:53Z

/test-windows-e2e
/test-windows-proxyall-e2e

XinShuYang · 2022-08-06T10:12:14Z

/test-windows-proxyall-e2e

wenyingd

LGTM

tnqn · 2022-08-08T09:55:08Z

/skip-integration tested manually

XinShuYang force-pushed the npluni branch from ebe6bd9 to 6e0072e Compare June 28, 2022 07:47

XinShuYang marked this pull request as ready for review June 28, 2022 07:53

XinShuYang force-pushed the npluni branch 5 times, most recently from 85c426b to 3156ae3 Compare June 28, 2022 16:31

XinShuYang mentioned this pull request Jun 28, 2022

Fix running NPL unit test on MacOS #3945

Merged

XinShuYang force-pushed the npluni branch 4 times, most recently from bf0acfb to d97231f Compare June 29, 2022 04:59

XinShuYang force-pushed the npluni branch from d97231f to 1a09e74 Compare June 29, 2022 10:34

XinShuYang requested a review from wenyingd June 29, 2022 16:05

wenyingd reviewed Jul 4, 2022

View reviewed changes

XinShuYang force-pushed the npluni branch from 1a09e74 to 7fce18b Compare July 15, 2022 06:52

XinShuYang force-pushed the npluni branch from 7fce18b to 5312e67 Compare July 15, 2022 07:11

wenyingd reviewed Jul 15, 2022

View reviewed changes

XinShuYang force-pushed the npluni branch from 5312e67 to 551e928 Compare July 22, 2022 01:39

wenyingd reviewed Jul 22, 2022

View reviewed changes

pkg/agent/nodeportlocal/portcache/port_table.go Outdated Show resolved Hide resolved

pkg/agent/nodeportlocal/portcache/port_table_linux.go Outdated Show resolved Hide resolved

XinShuYang force-pushed the npluni branch from 551e928 to 2a23dd5 Compare July 22, 2022 02:26

XinShuYang force-pushed the npluni branch 2 times, most recently from 9ac1c52 to e50a9e8 Compare July 22, 2022 02:30

XinShuYang force-pushed the npluni branch from 31396cd to 0c29ac7 Compare August 2, 2022 01:07

XinShuYang force-pushed the npluni branch from 0c29ac7 to 05ddb09 Compare August 2, 2022 01:54

tnqn added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 2, 2022

XinShuYang force-pushed the npluni branch 3 times, most recently from 7fedcb5 to bf9ac29 Compare August 2, 2022 14:09

XinShuYang force-pushed the npluni branch from bf9ac29 to f6179cc Compare August 2, 2022 23:09

antoninbas reviewed Aug 2, 2022

View reviewed changes

XinShuYang force-pushed the npluni branch 3 times, most recently from 16d5be1 to f00f3ae Compare August 4, 2022 23:16

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

antoninbas reviewed Aug 5, 2022

View reviewed changes

XinShuYang commented Aug 5, 2022

View reviewed changes

XinShuYang force-pushed the npluni branch from f00f3ae to 69a5cd7 Compare August 5, 2022 09:19

XinShuYang force-pushed the npluni branch from 69a5cd7 to 8461ba4 Compare August 5, 2022 10:05

antoninbas approved these changes Aug 6, 2022

View reviewed changes

XinShuYang requested a review from tnqn August 7, 2022 11:34

wenyingd approved these changes Aug 8, 2022

View reviewed changes

tnqn merged commit cdd98b4 into antrea-io:main Aug 8, 2022

		// TestNodePortAlreadyBoundTo validates that when a port with TCP protocol is already bound to,
		// the same port should be selected for NPL if any other protocol is available.

	return 0, fmt.Errorf("existed windows nodeport entry for %s:%d:%s", podIP, podPort, protocol)
	return 0, fmt.Errorf("existing Windows NodePort entry for %s:%d:%s", podIP, podPort, protocol)

	// the next port should be selected for NPL if the same protocol is available.
	// the next sequential TCP port should be selected for NPL when it is available.

	// Port needs to be only available for the protocol used by NPL rule.
	// Port only needs to be available for the protocol used by the NPL rule.

Implement NPL agent unification. #3936

Implement NPL agent unification. #3936

Conversation

XinShuYang commented Jun 24, 2022 • edited Loading

XinShuYang commented Jun 24, 2022

codecov-commenter commented Jun 24, 2022 • edited by codecov bot Loading

Codecov Report

XinShuYang commented Jun 28, 2022

XinShuYang commented Jun 29, 2022

XinShuYang commented Jun 29, 2022

XinShuYang commented Jul 15, 2022

XinShuYang commented Jul 22, 2022

XinShuYang commented Aug 1, 2022

XinShuYang commented Aug 2, 2022

XinShuYang commented Aug 2, 2022

XinShuYang commented Aug 2, 2022

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

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

Choose a reason for hiding this comment

XinShuYang commented Aug 4, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinShuYang commented Aug 5, 2022

antoninbas left a comment

Choose a reason for hiding this comment

XinShuYang commented Aug 6, 2022

XinShuYang commented Aug 6, 2022

wenyingd left a comment

Choose a reason for hiding this comment

tnqn commented Aug 8, 2022

XinShuYang commented Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jun 24, 2022 •

edited by codecov bot

Loading