Add IPv4/v6 dual stack support in Flow aggregator #1962

dreamtalen · 2021-03-16T23:29:15Z

After investigation, we found that the e2e test failure in dual stack cluster of flow aggregator is because we couldn't retrieve the service info from antrea-agent-proxier for IPv6 service address in jenkins dual stack cluster.

In this commit, we fixed a bug in GetServiceByIP() function of dual-stack proxy.
Also, we improved the e2e test of flow aggregator: for dual stack clusters, we will test services for both IPv4 and IPv6 clusterIP.

dreamtalen · 2021-03-17T00:22:16Z

/test-ipv6-only-e2e
/test-ipv6-e2e

codecov-io · 2021-03-17T01:09:39Z

Codecov Report

Merging #1962 (6b2fe93) into main (2c1666d) will increase coverage by 1.70%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1962      +/-   ##
==========================================
+ Coverage   65.37%   67.08%   +1.70%     
==========================================
  Files         197      197              
  Lines       17217    18014     +797     
==========================================
+ Hits        11256    12085     +829     
+ Misses       4786     4708      -78     
- Partials     1175     1221      +46

Flag	Coverage Δ
e2e-tests	`46.30% <0.00%> (?)`
kind-e2e-tests	`56.51% <0.00%> (+0.06%)`	⬆️
unit-tests	`41.82% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/flowexporter/connections/connections.go	`79.38% <ø> (ø)`
pkg/agent/proxy/proxier.go	`66.77% <0.00%> (-1.14%)`	⬇️
pkg/agent/openflow/pipeline_other.go	`50.00% <0.00%> (-16.67%)`	⬇️
pkg/util/ip/ip.go	`80.30% <0.00%> (-6.49%)`	⬇️
pkg/agent/openflow/client.go	`64.33% <0.00%> (-2.86%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`68.94% <0.00%> (-1.84%)`	⬇️
pkg/agent/openflow/network_policy.go	`75.97% <0.00%> (+0.14%)`	⬆️
pkg/agent/openflow/pipeline.go	`73.13% <0.00%> (+0.38%)`	⬆️
pkg/ovs/openflow/ofctrl_bridge.go	`49.01% <0.00%> (+1.18%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.75% <0.00%> (+1.62%)`	⬆️
... and 22 more

srikartati · 2021-03-19T20:58:30Z

/test-all

Is the "Draft status" still intended?

dreamtalen · 2021-03-19T21:01:14Z

/test-all

Is the "Draft status" still intended?

Hi Srikar, yes it's still under development. I found the dual cluster environment on jenkins is different from my local setup, and I'm still debugging on it.

dreamtalen · 2021-03-22T22:26:57Z

/test-ipv6-only-e2e
/test-ipv6-e2e

dreamtalen · 2021-03-22T23:20:34Z

/test-all
/test-ipv6-only-e2e
/test-ipv6-e2e

dreamtalen · 2021-03-23T18:41:46Z

/test-all
/test-ipv6-only-e2e
/test-ipv6-e2e

srikartati · 2021-03-23T22:11:13Z

test/e2e/fixtures.go

-func skipIfDualStackCluster(tb testing.TB) {
-	if clusterInfo.podV6NetworkCIDR != "" && clusterInfo.podV4NetworkCIDR != "" {
-		tb.Skipf("Skipping test as it is not supported in dual stack cluster")
-	}
-}


Keep it for future use?

Thanks, addressed.

srikartati · 2021-03-23T22:15:33Z

test/e2e/flowaggregator_test.go

+	}
+}
+
+func createSvcRunTests(t *testing.T, data *TestData, podAIP *PodIPs, podBIP *PodIPs, podCIP *PodIPs, isIPv6 bool) {


Separate out createServices function and runTests function for better readability

Thanks, addressed.

srikartati · 2021-03-23T22:19:28Z

third_party/proxy/meta_proxier.go

@@ -160,12 +161,12 @@ func (proxier *metaProxier) OnEndpointsSynced() {
 }

 func (proxier *metaProxier) GetServiceByIP(serviceStr string) (ServicePortName, bool) {
-	if utilnet.IsIPv6String(serviceStr) {
+	// Format of serviceStr is fmt.Sprintf("%s:%d/%s", clusterIP, svcPort, protocol), so IPv6 serviceStr has more than 3 colons.
+	if strings.Count(serviceStr, ":") >= 3 {


Check looks somewhat arbitrary. Can we parse the string with last colon as the separator and use the IP?

Thanks, addressed.

srikartati · 2021-03-23T22:33:24Z

third_party/proxy/meta_proxier.go

@@ -160,12 +161,12 @@ func (proxier *metaProxier) OnEndpointsSynced() {
 }

 func (proxier *metaProxier) GetServiceByIP(serviceStr string) (ServicePortName, bool) {


Hi @jianjuns ,
GetServiceByIP is a method in Proxier interface defined in pkg/third_party/proxy
This was originally defined in pkg/agent/proxy but moved to third-party pkg. This is currently used by Flow Exporter running in Agent and no one else uses it.
Therefore, I think it should follow the placement of GetServiceFlowKeys method and placed in pkg/agent/proxy. Do you agree?

That sounds good to me.

Thanks Srikar and Jianjun, moved the GetServiceByIP under pkg/agent/proxy and removed it from Provider interface in third_party/proxy.

srikartati

Will there be any changes on Kibana dashboard for dual-stack clusters?

pkg/agent/proxy/proxier.go

dreamtalen · 2021-03-24T03:47:27Z

Will there be any changes on Kibana dashboard for dual-stack clusters?

Hi Srikar, Kibana dashboard has supported dual stack environment in the previous IPv6 support PR.

srikartati · 2021-03-24T17:32:45Z

/test-all
/test-ipv6-e2e
/test-ipv6-only-e2e

srikartati

LGTM

antoninbas · 2021-03-24T20:09:32Z

pkg/agent/proxy/proxier.go

@@ -53,6 +54,9 @@ type Proxier interface {
 	// flows and the OVS group IDs for a Service. False is returned if the
 	// Service is not found.
 	GetServiceFlowKeys(serviceName, namespace string) ([]string, []binding.GroupIDType, bool)
+	// GetServiceByIP returns the ServicePortName struct of giving serviceString(ClusterIP:Port/Proto).


"of giving"? do you mean "for the given"?

Thanks, addressed.

antoninbas · 2021-03-24T20:09:47Z

pkg/agent/proxy/proxier.go

@@ -53,6 +54,9 @@ type Proxier interface {
 	// flows and the OVS group IDs for a Service. False is returned if the
 	// Service is not found.
 	GetServiceFlowKeys(serviceName, namespace string) ([]string, []binding.GroupIDType, bool)
+	// GetServiceByIP returns the ServicePortName struct of giving serviceString(ClusterIP:Port/Proto).
+	// False is returned if the serviceString is not existed in serviceStringMap.


s/is not existed/does not exist
or
s/is not existed/is not found

Thanks, addressed.

antoninbas · 2021-03-24T20:11:14Z

pkg/agent/proxy/proxier.go

@@ -639,6 +643,16 @@ func (p *metaProxierWrapper) GetServiceFlowKeys(serviceName, namespace string) (
 	return append(v4Flows, v6Flows...), append(v4Groups, v6Groups...), v4Found || v6Found
 }

+func (p *metaProxierWrapper) GetServiceByIP(serviceStr string) (k8sproxy.ServicePortName, bool) {
+	// Format of serviceStr is fmt.Sprintf("%s:%d/%s", clusterIP, svcPort, protocol).


that's a weird way of presenting it, why not:

// Format of serviceStr is <clusterIP>:<svcPort>/<protocol>

Thanks, addressed.

antoninbas · 2021-03-24T20:14:10Z

test/e2e/flowaggregator_test.go

+		time.Sleep(3 * time.Second)
+
+		runTests(t, data, podAIP, podBIP, podCIP, svcB, svcC, false)
+		deletePerftestServices(data)


maybe use a defer statement after the call to createPerftestServices

same below

Thanks, addressed.

antoninbas · 2021-03-24T20:19:24Z

test/e2e/flowaggregator_test.go

+	if v6Enabled {
+		svcB, svcC, err := createPerftestServices(data, true)
+		if err != nil {
+			t.Fatalf("Error when creating IPv6 perftest services: %v", err)
+		}
+		// Wait for the Service to be realized.
+		time.Sleep(3 * time.Second)
+
+		runTests(t, data, podAIP, podBIP, podCIP, svcB, svcC, true)
+		deletePerftestServices(data)
+	}


This looks like the exact same code as above. Maybe define a local function.

testHelper := func(isIPv6 bool) { ... } if v4Enabled { t.Logf("Running tests with IPv4 perftest service") testHelper() } if v6Enabled t.Logf("Running tests with IPv6 perftest service") testHelper() }

or even better, use subtests:

testHelper := func(t *testing.T, isIPv6 bool) { ... } if v4Enabled { t.Run("IPv4", func(t *testing.T) { testHelper(t, false) }) } if v4Enabled { t.Run("IPv6", func(t *testing.T) { testHelper(t, true) }) }

Thanks, addressed it using subtests.

antoninbas · 2021-03-24T20:20:27Z

test/e2e/flowaggregator_test.go

+	}
+}
+
+func runTests(t *testing.T, data *TestData, podAIP *PodIPs, podBIP *PodIPs, podCIP *PodIPs, svcB *corev1.Service, svcC *corev1.Service, isIPv6 bool) {


why is this helper method not in charge of creating the test resources (pods, services)? Why the need to pass them as parameters?

Hi Antonin, after changing to use subtests, only pod IPs are passed as parameters to the testHelper method. The reason is that we would like to reuse perf pods in both IPv4 and IPv6 tests, while perf services need to be created for IPv4 and IPv6 differently.

antoninbas · 2021-03-24T20:21:21Z

third_party/proxy/types.go

 - Add Run(), GetServiceByIP() to Provider interface
+- Remove GetServiceByIP() from Provider interface


These list the changes compared to upstream...
you should edit the existing last bullet point, not add a new one

Thanks, addressed.

antoninbas

some nits, otherwise LGTM

antoninbas · 2021-03-26T00:22:32Z

test/e2e/flowaggregator_test.go

 	if err != nil {
 		t.Fatalf("Error when setting up test: %v", err)
 	}
 	defer teardownTest(t, data)
 	defer teardownFlowAggregator(t, data)

-	podAIP, podBIP, podCIP, svcB, svcC, err := createPerftestPods(data, isIPv6)
+	podAIP, podBIP, podCIP, err := createPerftestPods(data)


nit: looks like these should be named podAIPs, podBIPs, podCIPs to match the rest of the code

Thanks, addressed.

antoninbas · 2021-03-26T00:22:39Z

test/e2e/flowaggregator_test.go

 	if err != nil {
-		t.Fatalf("Error when creating perftest pods and services: %v", err)
+		t.Fatalf("Error when creating perftest pods: %v", err)


s/pods/Pods

Thanks, addressed.

antoninbas · 2021-03-26T00:22:48Z

test/e2e/flowaggregator_test.go

+func testHelper(t *testing.T, data *TestData, podAIP *PodIPs, podBIP *PodIPs, podCIP *PodIPs, isIPv6 bool) {
+	svcB, svcC, err := createPerftestServices(data, isIPv6)
+	if err != nil {
+		t.Fatalf("Error when creating perftest services: %v", err)


s/services/Services

Thanks, addressed.

antoninbas · 2021-03-26T00:24:41Z

test/e2e/flowaggregator_test.go

+	if err != nil {
+		t.Fatalf("Error when creating perftest services: %v", err)
+	}
+	defer deletePerftestServices(data)


don't you want to log the error if there is one?

Thanks, addressed.

antoninbas · 2021-03-26T00:25:00Z

test/e2e/flowaggregator_test.go

+	}
+	podBIP, err = data.podWaitForIPs(defaultTimeout, "perftest-b", testNamespace)
+	if err != nil {
+		return nil, nil, nil, fmt.Errorf("Error when getting the perftest server Pod's IP: %v", err)


Thanks, addressed.

antoninbas · 2021-03-26T00:25:15Z

test/e2e/flowaggregator_test.go

 	if err != nil {
-		return nil, nil, nil, nil, nil, fmt.Errorf("Error when getting the perftest server Pod's IP: %v", err)
+		return nil, nil, fmt.Errorf("Error when creating perftest service: %v", err)


s/service/Service

same in every other place

Thanks, addressed.

antoninbas · 2021-03-26T00:25:20Z

test/e2e/flowaggregator_test.go

 	if err != nil {
-		return nil, nil, nil, nil, nil, fmt.Errorf("Error when creating perftest service: %v", err)
+		return nil, nil, nil, fmt.Errorf("Error when getting the perftest server Pod's IP: %v", err)


Thanks, addressed.

Fixed a bug in GetServiceByIP() function of dual-stack proxy and moved it under pkg/agent/proxy. Improved the e2e test of flow aggregator: for dual stack clusters, we will test services for both IPv4 and IPv6 clusterIP.

antoninbas

LGTM

srikartati · 2021-03-26T17:15:45Z

/test-all

srikartati · 2021-03-26T19:17:58Z

/test-e2e
/test-ipv6-e2e
/test-ipv6-only-e2e

dreamtalen · 2021-03-26T20:31:04Z

/test-ipv6-e2e
/test-ipv6-only-e2e

srikartati · 2021-03-26T21:38:26Z

All the reviews are done. Merging this.

vmwclabot added the cla-not-required label Mar 16, 2021

dreamtalen force-pushed the local-fa-ds branch from 9fcff92 to 69e613e Compare March 16, 2021 23:30

dreamtalen force-pushed the local-fa-ds branch 5 times, most recently from b43f752 to 958314b Compare March 19, 2021 18:10

dreamtalen force-pushed the local-fa-ds branch 2 times, most recently from 0cde7a5 to 543e90a Compare March 22, 2021 22:26

dreamtalen force-pushed the local-fa-ds branch from 543e90a to 308632b Compare March 22, 2021 23:18

dreamtalen marked this pull request as ready for review March 22, 2021 23:34

dreamtalen marked this pull request as draft March 23, 2021 17:00

dreamtalen force-pushed the local-fa-ds branch 2 times, most recently from 9df5a8c to d92cc41 Compare March 23, 2021 18:21

dreamtalen marked this pull request as ready for review March 23, 2021 21:03

srikartati reviewed Mar 23, 2021

View reviewed changes

dreamtalen force-pushed the local-fa-ds branch 2 times, most recently from 9a6fa10 to 6901b39 Compare March 24, 2021 00:52

srikartati reviewed Mar 24, 2021

View reviewed changes

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

dreamtalen force-pushed the local-fa-ds branch from 6901b39 to dbdab06 Compare March 24, 2021 03:45

dreamtalen force-pushed the local-fa-ds branch from dbdab06 to e2e02c3 Compare March 24, 2021 05:22

srikartati previously approved these changes Mar 24, 2021

View reviewed changes

antoninbas reviewed Mar 24, 2021

View reviewed changes

dreamtalen dismissed srikartati’s stale review via 7ce58b3 March 24, 2021 21:42

dreamtalen force-pushed the local-fa-ds branch 2 times, most recently from 7ce58b3 to 8efcf56 Compare March 24, 2021 21:52

antoninbas reviewed Mar 26, 2021

View reviewed changes

Add IPv4/v6 dual stack support in Flow aggregator

6b2fe93

Fixed a bug in GetServiceByIP() function of dual-stack proxy and moved it under pkg/agent/proxy. Improved the e2e test of flow aggregator: for dual stack clusters, we will test services for both IPv4 and IPv6 clusterIP.

dreamtalen force-pushed the local-fa-ds branch from 8efcf56 to 6b2fe93 Compare March 26, 2021 00:52

antoninbas approved these changes Mar 26, 2021

View reviewed changes

srikartati merged commit ae0705a into antrea-io:main Mar 26, 2021

		@@ -160,12 +161,12 @@ func (proxier *metaProxier) OnEndpointsSynced() {
		}

		func (proxier *metaProxier) GetServiceByIP(serviceStr string) (ServicePortName, bool) {

		- Add Run(), GetServiceByIP() to Provider interface
		- Remove GetServiceByIP() from Provider interface

Add IPv4/v6 dual stack support in Flow aggregator #1962

Add IPv4/v6 dual stack support in Flow aggregator #1962

Conversation

dreamtalen commented Mar 16, 2021 • edited Loading

dreamtalen commented Mar 17, 2021

codecov-io commented Mar 17, 2021 • edited Loading

Codecov Report

srikartati commented Mar 19, 2021

dreamtalen commented Mar 19, 2021

dreamtalen commented Mar 22, 2021

dreamtalen commented Mar 22, 2021

dreamtalen commented Mar 23, 2021

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

srikartati left a comment

Choose a reason for hiding this comment

dreamtalen commented Mar 24, 2021

srikartati commented Mar 24, 2021

srikartati 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

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

antoninbas 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

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

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Mar 26, 2021

srikartati commented Mar 26, 2021

dreamtalen commented Mar 26, 2021

srikartati commented Mar 26, 2021

dreamtalen commented Mar 16, 2021 •

edited

Loading

codecov-io commented Mar 17, 2021 •

edited

Loading