Support EgressIP assigning and failover in antrea-agent #2186

wenqiq · 2021-05-18T12:36:00Z

Support EgressIP assigning and failover in antrea-agent

A cluster will be created in the background when the Egress feature is turned on.
And the local Node will join all the other K8s Nodes in a memberlist cluster.

Each Node in the cluster holds the same consistent hash ring for each ExternalIPPool,
in order to distribute egress IPs equally among the selected Nodes (which are part of the
memberlist cluster). When a Node leaves the cluster, its IPs are redistributed.
When a Node joins the cluster, it's added to the hash ring and a small fraction of IPs are re-assigned to that Node.

For #2128

codecov-commenter · 2021-05-18T13:26:04Z

Codecov Report

Merging #2186 (c1b970c) into main (a8c7970) will increase coverage by 2.46%.
The diff coverage is 54.44%.

@@            Coverage Diff             @@
##             main    #2186      +/-   ##
==========================================
+ Coverage   61.91%   64.38%   +2.46%     
==========================================
  Files         281      283       +2     
  Lines       21771    22121     +350     
==========================================
+ Hits        13480    14243     +763     
+ Misses       6871     6409     -462     
- Partials     1420     1469      +49

Flag	Coverage Δ
e2e-tests	`54.02% <32.95%> (?)`
kind-e2e-tests	`51.55% <0.00%> (-0.70%)`	⬇️
unit-tests	`41.89% <58.23%> (+0.23%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/metrics/prometheus.go	`4.34% <ø> (ø)`
.../controller/egress/ipassigner/ip_assigner_linux.go	`25.00% <25.00%> (ø)`
pkg/agent/controller/egress/egress_controller.go	`63.85% <25.45%> (+23.05%)`	⬆️
pkg/agent/memberlist/cluster.go	`74.75% <74.75%> (ø)`
pkg/ovs/openflow/ofctrl_bridge.go	`50.00% <0.00%> (+0.34%)`	⬆️
pkg/agent/openflow/network_policy.go	`76.41% <0.00%> (+0.59%)`	⬆️
pkg/controller/networkpolicy/status_controller.go	`87.09% <0.00%> (+0.64%)`	⬆️
pkg/agent/cniserver/pod_configuration.go	`54.68% <0.00%> (+0.78%)`	⬆️
... and 34 more

cmd/antrea-agent/config.go

pkg/agent/memberlist/server.go

pkg/agent/memberlist/cluster.go

pkg/agent/memberlist/cluster_test.go

pkg/agent/controller/egress/egress_controller.go

pkg/agent/memberlist/cluster_test.go

pkg/agent/memberlist/cluster.go

pkg/agent/controller/egress/egress_controller.go

pkg/agent/memberlist/cluster.go

tnqn · 2021-07-06T12:14:42Z

pkg/agent/memberlist/cluster_test.go

+					}
+				}
+				assert.Equal(t, 1, len(actualNodes), "Selected Node num for Egress not match")
+				assert.Equal(t, []string{tCase.expectedNode}, actualNodes, "Select Node for Egress not match")


The test itself is O(n^2), I understand it needs to construct a fakeCluster for each Node to check whether a Node can select the Egress in current implementation. Why don't just change ShouldSelectNode return the desired Node to make the test much easier. The client just needs to check whether the desired Node is itself.

tnqn · 2021-07-06T12:19:22Z

cmd/antrea-agent/config.go

@@ -98,6 +98,11 @@ type AgentConfig struct {
 	// APIPort is the port for the antrea-agent APIServer to serve on.
 	// Defaults to 10350.
 	APIPort int `yaml:"apiPort,omitempty"`
+
+	// ClusterMembershipPort is the server port used by the antrea-agent to run a gossip-based cluster membership protocol.


You missed the comment: "Currently it's used only when the Egress feature is enabled."

tnqn · 2021-07-06T12:20:27Z

pkg/apis/crd/v1alpha2/types.go

+// EgressStatus represents the current status of an Egress.
+type EgressStatus struct {
+	// The name of the Node that holds the Egress IP.
+	NodeName string `json:"nodeName"`


I think yes

tnqn

Please update the commit message which has out-of-date description about when the node will join and leave the cluster.

tnqn · 2021-07-07T04:08:12Z

cmd/antrea-agent/config.go

@@ -98,6 +98,11 @@ type AgentConfig struct {
 	// APIPort is the port for the antrea-agent APIServer to serve on.
 	// Defaults to 10350.
 	APIPort int `yaml:"apiPort,omitempty"`
+
+	// ClusterMembershipPort is the server port used by the antrea-agent to run a gossip-based cluster membership protocol.


nit: don't mix the usage and the default value in same line.

ClusterMembershipPort is the server port used by the antrea-agent to run a gossip-based cluster membership protocol. Currently it's used only when the Egress feature is enabled.
Defaults to 10351.

tnqn · 2021-07-07T04:30:38Z

pkg/agent/memberlist/cluster_test.go

+		{
+			name:           fmt.Sprintf("Recover to %d nodes", 10),
+			nodes:          nodes[:10],
+			consistentHash: consistenthash.New(defaultVirtualNodeReplicas, nil),


I'm a bit confused by the test:

what does expectEgressSeqSum mean?

Does the 1st subtest have any difference from the 4th one given they have exactly same input?

What is it verifying given there is no expected value and it doesn't call any function/method in source file?

I feel it's likely just testing consistentHash directly which should be already covered by TestCluster_ShouldSelectEgress. If so, maybe just remove this one.

expectEgressSeqSum is the sum of all selected Egress sequence numbers. It ensures each Egress has been selected once. That is, Egresses distributed in different Nodes, no duplicate or missing items. Maybe I should rephrase nodeSelectedForEgress and use ShouldSelectEgress in this test case.

If verifying duplicate or missing items is the purpose, maybe you can just make a little change toTestCluster_ShouldSelectEgress;

genNodes := func(n int) []string { nodes := make([]string, n) for i := 0; i < n; i++ { nodes[i] = fmt.Sprintf("node-%d", i) } return nodes } fakeEIPName := "fakeExternalIPPool" fakeEgress := &crdv1a2.Egress{ ObjectMeta: metav1.ObjectMeta{Name: "fakeEgress"}, Spec: crdv1a2.EgressSpec{ExternalIPPool: fakeEIPName, EgressIP: tCase.egressIP}, } consistentHashMap := newNodeConsistentHashMap() consistentHashMap.Add(genNodes(tCase.nodeNum)...) fakeCluster := &Cluster{ consistentHashMap: map[string]*consistenthash.Map{fakeEIPName: consistentHashMap}, } for i := 0; i < n; i++ { node := fmt.Sprintf("node-%d", i) fakeCluster.nodeName = node selected, err := fakeCluster.ShouldSelectEgress(fakeEgress) assert.NoError(t, err) assert.Equal(t, node == tCase.expectedNode, selected, "Selected Node for Egress not match") }

tnqn · 2021-07-07T04:39:50Z

pkg/agent/memberlist/cluster_test.go

+// BenchmarkCluster_ShouldSelect/select_node_from_100_alive_nodes-16          	13036746	       103 ns/op
+// BenchmarkCluster_ShouldSelect/select_node_from_10_alive_nodes
+// BenchmarkCluster_ShouldSelect/select_node_from_10_alive_nodes-16           	14923483	        77.5 ns/op
+func BenchmarkCluster_ShouldSelect(b *testing.B) {


The benchmark doesn't really measure a method in source code, it calls one step in ShouldSelectEgress but also counts the time spent on creating a consistenthashMap. Can it focus one method i.e. ShouldSelectEgress? I don't think we need many input matrix either (as long as it's efficient in 1000 nodes, no one will worry about 10 nodes).

tnqn · 2021-07-07T04:44:16Z

Maybe name the patch and PR "Support EgressIP assigning and failover in antrea-agent"

tnqn · 2021-07-07T04:47:13Z

@jianjuns @antoninbas do you have more comments after the existing ones are resolved? If no, I will merge it after all comments are resolved so I can rebase #2345 and #2342 on it.

tnqn

LGTM overall

tnqn · 2021-07-07T16:45:01Z

Integration test failed.
/test-all

antoninbas · 2021-07-07T17:09:04Z

@tnqn no further comments from me

wenqiq · 2021-07-07T17:53:18Z

Integration test failed.
/test-all

Fix integration test case.

-       if err := ipAssigner.UnassignEgressIP(eg.Spec.EgressIP); err != nil {
+       if err := ipAssigner.UnassignEgressIP(eg.Name); err != nil {
                t.Fatalf("Unassigning egress IP error: %v", err)
        }

go test -coverpkg=antrea.io/antrea/pkg/... -coverprofile=.coverage/coverage-integration.txt -covermode=atomic -cover antrea.io/antrea/test/integration/...
?       antrea.io/antrea/test/integration       [no test files]
ok      antrea.io/antrea/test/integration/agent 30.456s coverage: 11.8% of statements in antrea.io/antrea/pkg/...
ok      antrea.io/antrea/test/integration/ovs   36.092s coverage: 4.6% of statements in antrea.io/antrea/pkg/...

@tnqn

1.Add memberlist cluster in antrea-agent A cluster will be created in the background when the Egress feature is turned on. And the local Node will join all the other K8s Nodes in a memberlist cluster. Each Node in the cluster holds the same consistent hash ring for each ExternalIPPool, in order to distribute egress IPs equally among the selected Nodes (which are part of the memberlist cluster). When a Node leaves the cluster, its IPs are redistributed. When a Node joins the cluster, it's added to the hash ring and a small fraction of IPs are re-assigned to that Node. 2.Assign EgressIP to owner Node and update Egress status Assign a owner node for Egress which with a valid externalIPPool. When Egress has been assigned a owner node and egressIP has assigned, the Egress status will updated, Egress status is the name of the Node that holds the Egress IP. Signed-off-by: wenqiq <wenqiq@vmware.com>

jianjuns

LGTM.
Some nits on comments.

jianjuns · 2021-07-08T01:24:35Z

pkg/agent/memberlist/cluster.go

+	return nodes
+}
+
+// ShouldSelectEgress the local Node in the cluster holds the same consistent hash ring for each ExternalIPPool,


Probably you missed a verb after ShouldSelectEgress

-// ShouldSelectEgress the local Node in the cluster holds the same consistent hash ring for each ExternalIPPool, -// when selecting an owner Node for an Egress, the local Node labels must match an ExternalIPPool nodeSelectors. +// ShouldSelectEgress returns true if the local Node selected as the owner Node of the Egress, +// the local Node in the cluster holds the same consistent hash ring for each ExternalIPPool,

jianjuns · 2021-07-08T01:25:02Z

pkg/agent/memberlist/cluster.go

+}
+
+// ShouldSelectEgress the local Node in the cluster holds the same consistent hash ring for each ExternalIPPool,
+// when selecting an owner Node for an Egress, the local Node labels must match an ExternalIPPool nodeSelectors.


Need to rephrase this line too.

jianjuns · 2021-07-08T01:25:57Z

pkg/agent/memberlist/cluster.go

+	switch event {
+	case memberlist.NodeJoin, memberlist.NodeLeave:
+		// When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated.
+		// when a Node leave cluster, the Node may failed or deleted,


When a Node leaves
may have failed or have been deleted

- // When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated. - // when a Node leave cluster, the Node may failed or deleted, - // if Node has been deleted, affected ExternalIPPool should enqueue, deleteNode handler has processed - // if the Node failed, ExternalIPPools consistentHash maybe changed, affected ExternalIPPool should enqueue. + // When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated; + // when a Node leaves cluster, the Node may have failed or have been deleted, + // if the Node has been deleted, affected ExternalIPPool should be enqueued, and deleteNode handler has been executed, + // if the Node has failed, ExternalIPPools consistentHash maybe changed, and affected ExternalIPPool should be enqueued.

jianjuns · 2021-07-08T01:27:19Z

pkg/agent/memberlist/cluster.go

+	node, event := nodeEvent.Node, nodeEvent.Event
+	switch event {
+	case memberlist.NodeJoin, memberlist.NodeLeave:
+		// When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated.


If you do not like to capitalize "when" in the next sentence, probably change "." to ";"

jianjuns · 2021-07-08T01:28:21Z

pkg/agent/memberlist/cluster.go

+	case memberlist.NodeJoin, memberlist.NodeLeave:
+		// When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated.
+		// when a Node leave cluster, the Node may failed or deleted,
+		// if Node has been deleted, affected ExternalIPPool should enqueue, deleteNode handler has processed


should be enqueued
and deleteNode handler has been executed (?)

jianjuns · 2021-07-08T01:28:56Z

pkg/agent/memberlist/cluster.go

+		// When a Node joins cluster, all matched ExternalIPPools consistentHash should be updated.
+		// when a Node leave cluster, the Node may failed or deleted,
+		// if Node has been deleted, affected ExternalIPPool should enqueue, deleteNode handler has processed
+		// if the Node failed, ExternalIPPools consistentHash maybe changed, affected ExternalIPPool should enqueue.


and affected...
should be enqueued.

jianjuns · 2021-07-08T01:29:35Z

pkg/agent/memberlist/cluster.go

+		coreNode, err := c.nodeLister.Get(node.Name)
+		if err != nil {
+			if errors.IsNotFound(err) {
+				// Node has been deleted, deleteNode handler has processed.


and deleteNode handler has been executed

- // Node has been deleted, deleteNode handler has processed. + // Node has been deleted, and deleteNode handler has been executed.

jianjuns · 2021-07-08T01:30:06Z

pkg/agent/memberlist/cluster.go

+		return err
+	}
+
+	// updateConsistentHash refresh the consistentHashMap.


- // updateConsistentHash refresh the consistentHashMap. + // updateConsistentHash refreshes the consistentHashMap.

jianjuns · 2021-07-08T01:30:24Z

pkg/agent/memberlist/cluster.go

+			return err
+		}
+		aliveNodes := c.aliveNodes()
+		// Node alive and Node labels matches with ExternalIPPool nodeSelector.


matches with -> match

- // Node alive and Node labels matches with ExternalIPPool nodeSelector. + // Node alive and Node labels match ExternalIPPool nodeSelector.

jianjuns · 2021-07-08T01:32:02Z

pkg/agent/controller/egress/egress_controller.go

+	go c.cluster.Run(stopCh)
+
+	// The Egress has been deleted but assigned IP has not been deleted,
+	// agent should delete those IPs when it starts.


so agent..
Or change "," to "." in the previous line.

- // The Egress has been deleted but assigned IP has not been deleted, - // agent should delete those IPs when it starts. + // The Egress has been deleted but assigned IP has not been deleted, so agent should delete those IPs when it starts.

Signed-off-by: wenqiq <wenqiq@vmware.com>

tnqn

LGTM

tnqn · 2021-07-08T06:24:52Z

/test-all

tnqn · 2021-07-08T15:47:31Z

It seems all comments have been addressed. I will merge this to unblock other PRs. If there are new comments, @wenqiq or I can address them in new PRs.

wenqiq · 2021-07-08T16:41:02Z

It seems all comments have been addressed. I will merge this to unblock other PRs. If there are new comments, @wenqiq or I can address them in new PRs.

Thanks for your professional and careful reviews, very helpful and inspiring for me, also thank @antoninbas and @jianjuns, no further comments from me at the moment.

wenqiq force-pushed the egress-failover branch 4 times, most recently from 8536fd3 to dbe3f3d Compare May 19, 2021 05:41

tnqn reviewed May 19, 2021

View reviewed changes

wenqiq force-pushed the egress-failover branch 5 times, most recently from 1f2aa4b to 1e7621e Compare May 24, 2021 06:51

tnqn reviewed Jun 3, 2021

View reviewed changes

wenqiq force-pushed the egress-failover branch 10 times, most recently from 9f10586 to 386e98f Compare June 8, 2021 14:22

tnqn reviewed Jun 8, 2021

View reviewed changes

pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved

pkg/agent/controller/egress/egress_controller.go Outdated Show resolved Hide resolved

tnqn reviewed Jun 9, 2021

View reviewed changes

wenqiq force-pushed the egress-failover branch 5 times, most recently from c39a1f3 to 7d59d3a Compare June 11, 2021 02:47

tnqn reviewed Jun 11, 2021

View reviewed changes

wenqiq force-pushed the egress-failover branch from 0aed3c2 to 1264d91 Compare July 6, 2021 09:49

tnqn reviewed Jul 6, 2021

View reviewed changes

wenqiq force-pushed the egress-failover branch 2 times, most recently from 5f1c4ec to dbc7e0d Compare July 7, 2021 03:32

tnqn reviewed Jul 7, 2021

View reviewed changes

wenqiq changed the title ~~Add memberlist cluster in antrea-agent for egress auto failover support~~ Support EgressIP assigning and failover in antrea-agent Jul 7, 2021

wenqiq force-pushed the egress-failover branch 5 times, most recently from adfca93 to 585a3ea Compare July 7, 2021 15:10

tnqn added this to the Antrea v1.2 release milestone Jul 7, 2021

tnqn previously approved these changes Jul 7, 2021

View reviewed changes

wenqiq dismissed tnqn’s stale review via f04fd85 July 7, 2021 17:50

wenqiq force-pushed the egress-failover branch from f04fd85 to d88ca9d Compare July 8, 2021 00:38

jianjuns reviewed Jul 8, 2021

View reviewed changes

Address some comments

c1b970c

Signed-off-by: wenqiq <wenqiq@vmware.com>

tnqn approved these changes Jul 8, 2021

View reviewed changes

tnqn merged commit 5313dd7 into antrea-io:main Jul 8, 2021

wenqiq mentioned this pull request Aug 4, 2021

Update Egress status when configuring static Egress #2444

Merged

tnqn mentioned this pull request Aug 24, 2021

Support automatic failover for Egress #2128

Closed

6 tasks

Support EgressIP assigning and failover in antrea-agent #2186

Support EgressIP assigning and failover in antrea-agent #2186

Conversation

wenqiq commented May 18, 2021 • edited Loading

codecov-commenter commented May 18, 2021 • edited Loading

Codecov Report

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

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 commented Jul 7, 2021

tnqn commented Jul 7, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 7, 2021

antoninbas commented Jul 7, 2021

wenqiq commented Jul 7, 2021 • edited Loading

jianjuns 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

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

tnqn commented Jul 8, 2021

tnqn commented Jul 8, 2021

wenqiq commented Jul 8, 2021

wenqiq commented May 18, 2021 •

edited

Loading

codecov-commenter commented May 18, 2021 •

edited

Loading

wenqiq commented Jul 7, 2021 •

edited

Loading