Add CLI command to get memberlist state #4611

Atish-iaf · 2023-02-07T16:48:22Z

Add an antrea agent command antctl get memberlist to get the state of memberlist cluster of antrea-agent.

root@kind-worker2:/# antctl get memberlist
NODE               IP         STATUS
kind-control-plane 172.18.0.4 Alive 
kind-worker        172.18.0.3 Alive 
kind-worker2       172.18.0.2 Alive

Fixes #4601

Signed-off-by: Kumar Atish [email protected]

codecov · 2023-02-07T17:05:21Z

Codecov Report

Merging #4611 (5659835) into main (9b860d8) will decrease coverage by 0.15%.
The diff coverage is 85.36%.

❗ Current head 5659835 differs from pull request most recent head 6366fd5. Consider uploading reports for the commit 6366fd5 to get more accurate results

@@            Coverage Diff             @@
##             main    #4611      +/-   ##
==========================================
- Coverage   69.78%   69.64%   -0.15%     
==========================================
  Files         401      402       +1     
  Lines       59529    58388    -1141     
==========================================
- Hits        41545    40662     -883     
+ Misses      15166    14928     -238     
+ Partials     2818     2798      -20

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.33% <74.35%> (-0.05%)`	⬇️
integration-tests	`34.45% <ø> (+0.06%)`	⬆️	Carriedforward from 6366fd5
kind-e2e-tests	`47.17% <74.35%> (+0.11%)`	⬆️	Carriedforward from 6366fd5
unit-tests	`59.79% <70.73%> (-0.01%)`	⬇️	Carriedforward from 6366fd5

*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%> (ø)`
pkg/antctl/antctl.go	`50.00% <ø> (ø)`
pkg/agent/apiserver/handlers/memberlist/handler.go	`86.20% <86.20%> (ø)`
pkg/agent/apiserver/apiserver.go	`71.57% <100.00%> (+0.30%)`	⬆️
pkg/agent/querier/querier.go	`87.09% <100.00%> (-1.04%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`83.33% <100.00%> (-1.44%)`	⬇️
pkg/support/dump.go	`62.01% <100.00%> (+0.42%)`	⬆️
...g/agent/controller/serviceexternalip/controller.go	`69.62% <0.00%> (-11.95%)`	⬇️
pkg/agent/controller/egress/egress_controller.go	`75.27% <0.00%> (-9.96%)`	⬇️
pkg/agent/proxy/topology.go	`72.72% <0.00%> (-9.10%)`	⬇️
... and 50 more

docs/antctl.md

docs/support-bundle-guide.md

tnqn · 2023-02-09T09:06:33Z

pkg/agent/apiserver/handlers/memberlist/handler.go

+	Status   string `json:"status,omitempty"`
+}
+
+func generateResponse(node v1.Node, aliveNodes sets.String) Response {


Suggested change

func generateResponse(node v1.Node, aliveNodes sets.String) Response {

func generateResponse(node *v1.Node, aliveNodes sets.String) Response {

to avoid copy

tnqn · 2023-02-09T09:15:44Z

pkg/agent/apiserver/handlers/memberlist/handler.go

+func HandleFunc(aq querier.AgentQuerier) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		var memberlist []Response
+		allNodes, err := aq.GetK8sClient().CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})


The agent already caches all Nodes in memory, it could use NodeInformer's Lister to get all Nodes, intead of making an API call, which is not cheap in scale cluster.

tnqn · 2023-02-09T09:16:23Z

pkg/agent/apiserver/handlers/memberlist/handler.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		var memberlist []Response
+		allNodes, err := aq.GetK8sClient().CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})
+		if err != nil {


After switching to NodeInformer, it would never fail, so error could be ignored.

tnqn · 2023-02-09T09:18:19Z

pkg/agent/querier/querier.go

@@ -55,6 +58,7 @@ type agentQuerier struct {
 	networkPolicyInfoQuerier querier.AgentNetworkPolicyInfoQuerier
 	apiPort                  int
 	nplRange                 string
+	memberlistCluster        *memberlist.Cluster


Use Interface instead of concrete type

tnqn · 2023-02-09T09:20:24Z

pkg/antctl/antctl.go

+			aliases: []string{"ml"},
+			short:   "Print state of memberlist cluster",
+			long:    "Print state of memberlist cluster of Antrea agent",
+			example: `  Get a memberlist


Since there is only one usage, this example looks verbose, could be deleted

deleted, thanks.

docs/support-bundle-guide.md

tnqn · 2023-02-13T13:49:34Z

pkg/agent/apiserver/handlers/memberlist/handler_test.go

+	node1 = v1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: "node1"},
+		Status: v1.NodeStatus{
+			Addresses: []v1.NodeAddress{
+				{
+					Address: "172.16.0.11",
+				},
+			},
+		},
+	}
+
+	expectedContent = []Response{
+		{
+			NodeName: "node1",
+			IP:       "172.16.0.11",
+			Status:   "Alive",
+		},
+	}


Move the expected response to the test to be self-contained. Expected response is not likely to be reused.

It should include a dead node to cover the negaive case.

tnqn · 2023-02-13T13:58:18Z

pkg/agent/querier/querier.go

@@ -42,6 +45,8 @@ type AgentQuerier interface {
 	GetOVSCtlClient() ovsctl.OVSCtlClient
 	GetProxier() proxy.Proxier
 	GetNetworkPolicyInfoQuerier() querier.AgentNetworkPolicyInfoQuerier
+	GetAliveNodes() sets.String


I feel the AgentQuerier is perhaps a bad interface which mixes too many things which breaks a few OOP rules. I understand the change is following existing code, however, it should use the same style to return a ClusterInterface and let the client to call its GetAliveNodes interface to be consistent, instead of playing a proxy here.

tnqn · 2023-02-14T14:22:42Z

hack/update-codegen-dockerized.sh

@@ -41,7 +41,7 @@ MOCKGEN_TARGETS=(
  "pkg/agent/cniserver/ipam IPAMDriver testing"
  "pkg/agent/flowexporter/connections ConnTrackDumper,NetFilterConnTrack testing"
  "pkg/agent/interfacestore InterfaceStore testing"
-  "pkg/agent/memberlist Memberlist testing"
+  "pkg/agent/memberlist Interface,Memberlist . mock_memberlist.go"


I don't get why this change is made. Is it related to my comment here #4611 (comment)? If yes, I think it's misunderstood. I meant querier interface should provide a method to return memberlist.Interface, instead of providing a method GetAliveNodes and playing as a proxy.

@tnqn
This change is because :-

I needed to mock Interface of memberlist package which has AliveNodes method in order to write unit test (pkg/agent/apiserver/handlers/memberlist/handler_test.go) [Line 75 and 78].

Now, after mocking Interface of memberlist package in testing sub package i was getting cyclic import issue, so to avoid that I created mock in the memberlist package itself.

Now, mock file was created as mock_memberlist_test.go, I changed that to mock_memberlist.go inorder to make methods exportable as I needed it in unit test (handler_test.go).

Could you describe the cyclic import? I don't see it from the imports even if the file is in testing package.

Hi @tnqn
If we create mock for interfaces Memberlist and Interface in a file inside testing subpackage, it would import "antrea.io/antrea/pkg/agent/memberlist" as it is required for mock of method AddClusterEventHandler of interafce Interface.
Now, we get cyclic import error in cluster.go and cluster_test.go as cluster_test.go imports this testing subpackage.

OK, now I see there is a MockMemberlist in the testing package. Normally the testing has a mock implementation of the interface its parent package provides, and it's intended to be used by the interface's consumers in their tests, and the parent package wouldn't consume its own testing (and the testing package will likely consume the parent package because of the type import).

However, just moving the two mock structs to the parent package would cause other "problems":
If the mock structs are put in a file with "_test.go" suffix, they won't be exported and we can't use MockInterface.
If the mock structs are put in a file without "_test.go" suffix, they will be compiled and linked into the product code unnecessarily.

I would suggest to follow the mode that keeps the mock implementation of the target interface in testing package, and keeps the mock implementation of the dependent interface in "mock_memberlist_test.go" in the parent package as it's not intended to be consumed out of the package.

@tnqn done, thanks for the suggestion.

tnqn · 2023-02-21T02:09:36Z

pkg/agent/apiserver/handlers/memberlist/handler_test.go

+	clientset := fake.NewSimpleClientset()
+	informerFactory := informers.NewSharedInformerFactory(clientset, 0)
+	nodeInformer := informerFactory.Core().V1().Nodes()
+
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+
+	informerFactory.Start(stopCh)
+	informerFactory.WaitForCacheSync(stopCh)
+	informerFactory.Core().V1().Nodes().Informer().GetIndexer().Add(&node1)
+	informerFactory.Core().V1().Nodes().Informer().GetIndexer().Add(&node2)


It looks neater to pass the two objects when creating the clientset. It needs an "informer" or "lister" to be created first by calling nodeInformer.Informer() or nodeInformer.Lister(). But I feel AgentQuerier just needs NodeLister so maybe change the passed type to resolve the it.

done, thanks!

tnqn · 2023-02-21T02:10:27Z

pkg/agent/apiserver/handlers/memberlist/handler_test.go

+	sort.Slice(received, func(i, j int) bool {
+		return received[i].NodeName < received[j].NodeName
+	})
+	assert.Equal(t, expectedResponse, received)


assert.ElementsMatch

Add antrea agent command `antctl get memberlist` to get state of memberlist cluster of antrea agent. Fixes antrea-io#4601 Signed-off-by: Kumar Atish <[email protected]>

tnqn

LGTM, thanks

tnqn · 2023-02-21T06:39:20Z

/test-all

tnqn · 2023-02-21T08:06:57Z

/skip-e2e which failed on known flaky test

jianjuns · 2023-03-20T17:29:58Z

docs/antctl.md

+
+### Showing memberlist state
+
+`antctl` agent command `get memberlist` (or `get ml`) print the state of memberlist cluster of Antrea Agent.


print -> prints

done here https://github.com/antrea-io/antrea/pull/4698/files#diff-928cdf9bcf3327f17c73dd3ff180a6b203db4721313394d8e78f99da64bf190c

Atish-iaf force-pushed the antctl-get-memberlist branch from ddc941b to c61a892 Compare February 7, 2023 18:20

Atish-iaf changed the title ~~Add CLI command to get memberlist state~~ [WIP] Add CLI command to get memberlist state Feb 8, 2023

Atish-iaf force-pushed the antctl-get-memberlist branch from c61a892 to 4b9d035 Compare February 8, 2023 07:44

Atish-iaf marked this pull request as ready for review February 9, 2023 07:29

Atish-iaf requested a review from tnqn February 9, 2023 07:29

Atish-iaf changed the title ~~[WIP] Add CLI command to get memberlist state~~ Add CLI command to get memberlist state Feb 9, 2023

tnqn reviewed Feb 9, 2023

View reviewed changes

Atish-iaf force-pushed the antctl-get-memberlist branch from 4b9d035 to ebb6519 Compare February 9, 2023 18:52

Atish-iaf requested a review from tnqn February 9, 2023 19:20

Atish-iaf added this to the Antrea v1.11 release milestone Feb 9, 2023

tnqn reviewed Feb 13, 2023

View reviewed changes

Atish-iaf force-pushed the antctl-get-memberlist branch 2 times, most recently from 98474c0 to 187eff5 Compare February 14, 2023 05:33

Atish-iaf requested a review from tnqn February 14, 2023 06:00

tnqn reviewed Feb 14, 2023

View reviewed changes

Atish-iaf requested a review from tnqn February 17, 2023 17:41

Atish-iaf force-pushed the antctl-get-memberlist branch from 187eff5 to 161996e Compare February 20, 2023 19:10

tnqn reviewed Feb 21, 2023

View reviewed changes

Add CLI command to get memberlist state

6366fd5

Add antrea agent command `antctl get memberlist` to get state of memberlist cluster of antrea agent. Fixes antrea-io#4601 Signed-off-by: Kumar Atish <[email protected]>

Atish-iaf force-pushed the antctl-get-memberlist branch from 161996e to 6366fd5 Compare February 21, 2023 06:09

Atish-iaf requested a review from tnqn February 21, 2023 06:35

tnqn approved these changes Feb 21, 2023

View reviewed changes

tnqn added area/component/antctl Issues or PRs releated to the command line interface component action/release-note Indicates a PR that should be included in release notes. labels Feb 21, 2023

tnqn merged commit 8eb5c8d into antrea-io:main Feb 21, 2023

luolanzone mentioned this pull request Feb 27, 2023

TestAntctl failed on windows testbed #4659

Closed

jianjuns reviewed Mar 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI command to get memberlist state #4611

Add CLI command to get memberlist state #4611

Atish-iaf commented Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

tnqn Feb 9, 2023

tnqn Feb 9, 2023

tnqn Feb 9, 2023

tnqn Feb 9, 2023

tnqn Feb 9, 2023

Atish-iaf Feb 9, 2023 •

edited

Loading

tnqn Feb 13, 2023

Atish-iaf Feb 14, 2023

tnqn Feb 13, 2023

tnqn Feb 14, 2023

Atish-iaf Feb 14, 2023

tnqn Feb 15, 2023

Atish-iaf Feb 15, 2023 •

edited

Loading

tnqn Feb 20, 2023

Atish-iaf Feb 20, 2023

tnqn Feb 21, 2023

Atish-iaf Feb 21, 2023

tnqn Feb 21, 2023

tnqn left a comment

tnqn commented Feb 21, 2023

tnqn commented Feb 21, 2023

jianjuns Mar 20, 2023

Atish-iaf Mar 23, 2023

	func generateResponse(node v1.Node, aliveNodes sets.String) Response {
	func generateResponse(node *v1.Node, aliveNodes sets.String) Response {


		### Showing memberlist state

		`antctl` agent command `get memberlist` (or `get ml`) print the state of memberlist cluster of Antrea Agent.

Add CLI command to get memberlist state #4611

Add CLI command to get memberlist state #4611

Conversation

Atish-iaf commented Feb 7, 2023

codecov bot commented Feb 7, 2023 • 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

Choose a reason for hiding this comment

Atish-iaf Feb 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

Atish-iaf Feb 15, 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Feb 21, 2023

tnqn commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2023 •

edited

Loading

Atish-iaf Feb 9, 2023 •

edited

Loading

Atish-iaf Feb 15, 2023 •

edited

Loading