Fix e2e TestAntctl on windows testbed #4663

Atish-iaf · 2023-02-28T18:53:40Z

TestAntctl/testAntctlControllerRemoteAccess failed
on all windows testbed because it cannot get
output of antctl get memberlist while collecting
supportbundle from outside on windows testbed as
memberlist is not running.

This PR handles the above case as success on linux
and writes output to file that memberlist is not running.

On Windows, don't collect memberlist as it never runs on Windows.

Fixes #4659

Atish-iaf · 2023-02-28T18:58:30Z

/test-windows-e2e

XinShuYang · 2023-03-01T05:14:47Z

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

Atish-iaf · 2023-03-01T06:39:51Z

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

XinShuYang · 2023-03-01T07:51:40Z

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

Atish-iaf · 2023-03-01T09:28:43Z

/test-windows-e2e

luolanzone · 2023-03-01T14:39:53Z

pkg/support/dump.go

@@ -293,7 +295,13 @@ func (d *agentDumper) DumpAgentInfo(basedir string) error {
 }

 func (d *agentDumper) DumpMemberlist(basedir string) error {
-	return dumpAntctlGet(d.fs, d.executor, "memberlist", basedir)
+	var ml []memberlist.Response


Could you add some unit tests for the change?

tnqn · 2023-03-01T15:39:19Z

pkg/support/dump.go

@@ -293,7 +295,13 @@ func (d *agentDumper) DumpAgentInfo(basedir string) error {
 }

 func (d *agentDumper) DumpMemberlist(basedir string) error {
-	return dumpAntctlGet(d.fs, d.executor, "memberlist", basedir)
+	var ml []memberlist.Response


The real issue is memberlistCluster is nil on windows. The change in the PR can't resolve the problem.

The problem also exists on Linux when Egress feature is disabled. memberlist.HandleFunc should check whether GetMemberlistCluster() return nil before calling its AliveNodes method. If the interface is nil, it should return a proper http code and message to client indicating memberlist is not running.

Atish-iaf · 2023-03-02T03:00:21Z

/test-windows-all

Atish-iaf · 2023-03-02T17:31:36Z

/test-windows-all

Atish-iaf · 2023-03-03T01:12:55Z

/test-windows-e2e

tnqn · 2023-03-03T06:17:42Z

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

+		if aq.GetMemberlistCluster() == nil {
+			http.Error(w, "memberlist is not running", http.StatusServiceUnavailable)
+			return
+		}
 		var memberlist []Response
 		allNodes, _ := aq.GetNodeLister().List(labels.Everything())
 		aliveNodes := aq.GetMemberlistCluster().AliveNodes()


Suggested change

if aq.GetMemberlistCluster() == nil {

http.Error(w, "memberlist is not running", http.StatusServiceUnavailable)

return

}

var memberlist []Response

allNodes, _ := aq.GetNodeLister().List(labels.Everything())

aliveNodes := aq.GetMemberlistCluster().AliveNodes()

memberlisterCluster := aq.GetMemberlistCluster()

if memberlisterCluster == nil {

http.Error(w, "memberlist is not running", http.StatusServiceUnavailable)

return

}

var memberlist []Response

allNodes, _ := aq.GetNodeLister().List(labels.Everything())

aliveNodes := memberlisterCluster.AliveNodes()

tnqn · 2023-03-03T06:35:43Z

pkg/support/dump.go

+	handler := memberlist.HandleFunc(d.aq)
+	req, err := http.NewRequest(http.MethodGet, "", nil)
+	if err != nil {
+		return err
+	}
+	recorder := httptest.NewRecorder()
+	handler.ServeHTTP(recorder, req)
+	var received []memberlist.Response
+	if recorder.Code == http.StatusOK {
+		err = json.Unmarshal(recorder.Body.Bytes(), &received)
+		if err != nil {
+			return err
+		}
+	}
+	return writeYAMLFile(d.fs, filepath.Join(basedir, "memberlist"), "memberlist", received)


Why is it still needed?

because windows tests fail with same reason if we keep existing code (even after making changes in pkg/agent/apiserver/handlers/memberlist/handler.go to check for memberlist is running or not). I am not aware of the reason for this but i guess there maybe some history of running antctl cmds on windows as I see DumpAgentInfo func in dump.go also uses similar approach instead of calling dumpAntctlGet function.

We should understand why it fails if it's not supposed to. I don't know why DumpAgentInfo implements it in a different way but at least it doesn't create a new http handler and client to fetch the result, which looks redundant.

I think the failure on windows of the command is still expected, as the server the http response code is http.StatusServiceUnavailable, which will be converted an error by exec.Cmd. The underlying difference is, previously antrea-agent panic if it receives this http request, now it returns an error to client to indicate memberlist is not running. I think the code should handle the failure as an expected case just like what you did in L308.

Got it, thanks.

@tnqn Do you know the reason for Use internal queriers instead of antctl to retrieve networkpolicies and agentinfo. ? as written in commit message of this PR #820

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

Atish-iaf · 2023-03-05T22:31:10Z

/test-windows-all

Atish-iaf · 2023-03-06T00:30:31Z

/test-windows-all

Atish-iaf · 2023-03-06T05:51:35Z

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

Atish-iaf · 2023-03-06T06:50:35Z

/test-windows-proxyall-e2e

Atish-iaf · 2023-03-06T07:33:26Z

/test-windows-proxyall-e2e

Atish-iaf · 2023-03-07T16:27:58Z

/test-windows-all

tnqn · 2023-03-07T17:24:24Z

pkg/support/dump.go

@@ -293,7 +293,11 @@ func (d *agentDumper) DumpAgentInfo(basedir string) error {
 }

 func (d *agentDumper) DumpMemberlist(basedir string) error {
-	return dumpAntctlGet(d.fs, d.executor, "memberlist", basedir)
+	err := dumpAntctlGet(d.fs, d.executor, "memberlist", basedir)
+	if err != nil && strings.Contains(err.Error(), "error when dumping memberlist:") {


This would just ignore all errors as error returned by dumpAntctlGet always include this string. Can it get the target message "memberlist is not running" or the target status code http.StatusServiceUnavailable from the error in some way?

dumpAntctlGet returns broadly two kinds of error -

1A. error due to memberlist not running , status code http.StatusServiceUnavailable.
1B. error when encoding Memberlist to json, status code http.StatusInternalServerError
These two cases mentioned above will fall in this if case.

error when writing memberlist to file.
This case will NOT fall in this if case.

@tnqn Do you suggest to catch only case 1A in this if case (which writes no data to file)?

EDIT : done, thanks!

Only 1A indicates things work correctly, so we should process 1A like success.

Atish-iaf · 2023-03-08T02:17:52Z

/test-windows-all

pkg/support/dump.go

tnqn

LGTM

XinShuYang · 2023-03-08T06:56:54Z

/test-windows-all

XinShuYang · 2023-03-08T09:29:53Z

TestAntctl e2e test still failed.

tnqn · 2023-03-08T09:42:16Z

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

@@ -48,9 +48,14 @@ func generateResponse(node *v1.Node, aliveNodes sets.String) Response {
 // HandleFunc returns the function which can handle queries issued by the memberlist command.
 func HandleFunc(aq querier.AgentQuerier) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
+		memberlistCluster := aq.GetMemberlistCluster()
+		if memberlistCluster == nil {


An interface assigned with a concrete type's value will not be nil regardless whether the concrete type is nil or not.
Try this code: https://go.dev/play/p/rkyXVVwDjw1.

The nil check should use reflect.ValueOf(multiclusterInterface).IsNil()

Atish-iaf · 2023-03-08T11:55:41Z

/test-windows-all

tnqn · 2023-03-09T03:47:26Z

/test-windows-e2e

tnqn · 2023-03-09T03:48:25Z

/test-windows-containerd-e2e

tnqn · 2023-03-09T03:49:15Z

/test-windows-proxyall-e2e

XinShuYang · 2023-03-09T06:12:52Z

/test-windows-proxyall-e2e

XinShuYang · 2023-03-09T06:28:00Z

/test-windows-proxyall-e2e

TestAntctl/testAntctlControllerRemoteAccess failed on all windows testbed because it cannot get output of `antctl get memberlist` while collecting supportbundle from outside on windows testbed as memberlist is not running. This PR handles the above case as success on linux and writes output to file that memberlist is not running. On Windows, don't collect memberlist as it never runs on Windows. Fixes antrea-io#4659 Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

Atish-iaf · 2023-03-09T07:14:42Z

/test-windows-proxyall-e2e

tnqn

LGTM

tnqn · 2023-03-09T08:43:04Z

/skip-all
The windows e2e failed because of other failures, TestAntctl/testAntctlControllerRemoteAccess has succeded.

TestAntctl/testAntctlControllerRemoteAccess failed on all windows testbed because it cannot get output of `antctl get memberlist` while collecting supportbundle from outside on windows testbed as memberlist is not running. This PR handles the above case as success on linux and writes output to file that memberlist is not running. On Windows, don't collect memberlist as it never runs on Windows. Fixes antrea-io#4659 Signed-off-by: Kumar Atish <atish.iaf@gmail.com>

Atish-iaf changed the title ~~Fix e2e TestAntctl on windows testbed~~ [WIP] Fix e2e TestAntctl on windows testbed Feb 28, 2023

Atish-iaf marked this pull request as ready for review March 1, 2023 13:29

Atish-iaf changed the title ~~[WIP] Fix e2e TestAntctl on windows testbed~~ Fix e2e TestAntctl on windows testbed Mar 1, 2023

Atish-iaf requested review from luolanzone and XinShuYang March 1, 2023 14:15

luolanzone added the area/test/e2e Issues or PRs related to Antrea specific end-to-end testing. label Mar 1, 2023

luolanzone requested a review from wenyingd March 1, 2023 14:17

luolanzone reviewed Mar 1, 2023

View reviewed changes

tnqn reviewed Mar 1, 2023

View reviewed changes

Atish-iaf marked this pull request as draft March 2, 2023 01:18

Atish-iaf force-pushed the antctlFix branch from 594eaa4 to f0bdd93 Compare March 2, 2023 01:22

wenyingd added this to the Antrea v1.11 release milestone Mar 2, 2023

Atish-iaf force-pushed the antctlFix branch from f0bdd93 to ab415d8 Compare March 2, 2023 14:24

Atish-iaf marked this pull request as ready for review March 3, 2023 02:28

Atish-iaf requested a review from tnqn March 3, 2023 05:48

tnqn reviewed Mar 3, 2023

View reviewed changes

Atish-iaf marked this pull request as draft March 5, 2023 22:09

Atish-iaf force-pushed the antctlFix branch from ab415d8 to 0f59383 Compare March 5, 2023 22:17

Atish-iaf force-pushed the antctlFix branch from 0f59383 to 7edb7e8 Compare March 6, 2023 00:25

Atish-iaf marked this pull request as ready for review March 6, 2023 06:50

Atish-iaf requested a review from tnqn March 6, 2023 06:51

Atish-iaf force-pushed the antctlFix branch from 7edb7e8 to 874f96f Compare March 7, 2023 16:23

tnqn reviewed Mar 7, 2023

View reviewed changes

Atish-iaf force-pushed the antctlFix branch from 874f96f to 38b9783 Compare March 8, 2023 01:59

tnqn reviewed Mar 8, 2023

View reviewed changes

pkg/support/dump.go Outdated Show resolved Hide resolved

pkg/support/dump.go Outdated Show resolved Hide resolved

Atish-iaf force-pushed the antctlFix branch from 38b9783 to c72496e Compare March 8, 2023 04:55

tnqn approved these changes Mar 8, 2023

View reviewed changes

tnqn requested changes Mar 8, 2023

View reviewed changes

Atish-iaf force-pushed the antctlFix branch from c72496e to 18ddb08 Compare March 8, 2023 10:58

Atish-iaf force-pushed the antctlFix branch from 18ddb08 to 3e9221d Compare March 9, 2023 06:16

Atish-iaf force-pushed the antctlFix branch from 3e9221d to 44e7ce7 Compare March 9, 2023 06:35

tnqn approved these changes Mar 9, 2023

View reviewed changes

tnqn merged commit dd564e6 into antrea-io:main Mar 9, 2023

Fix e2e TestAntctl on windows testbed #4663

Fix e2e TestAntctl on windows testbed #4663

Conversation

Atish-iaf commented Feb 28, 2023 • edited Loading

Atish-iaf commented Feb 28, 2023

XinShuYang commented Mar 1, 2023 • edited Loading

Atish-iaf commented Mar 1, 2023

XinShuYang commented Mar 1, 2023

Atish-iaf commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atish-iaf commented Mar 2, 2023

Atish-iaf commented Mar 2, 2023

Atish-iaf commented Mar 3, 2023

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 commented Mar 5, 2023

Atish-iaf commented Mar 6, 2023

Atish-iaf commented Mar 6, 2023

Atish-iaf commented Mar 6, 2023

Atish-iaf commented Mar 6, 2023

Atish-iaf commented Mar 7, 2023

Choose a reason for hiding this comment

Atish-iaf Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Atish-iaf commented Mar 8, 2023

tnqn left a comment

Choose a reason for hiding this comment

XinShuYang commented Mar 8, 2023

XinShuYang commented Mar 8, 2023

Choose a reason for hiding this comment

Atish-iaf commented Mar 8, 2023

tnqn commented Mar 9, 2023

tnqn commented Mar 9, 2023

tnqn commented Mar 9, 2023

XinShuYang commented Mar 9, 2023

XinShuYang commented Mar 9, 2023

Atish-iaf commented Mar 9, 2023

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 9, 2023

Atish-iaf commented Feb 28, 2023 •

edited

Loading

XinShuYang commented Mar 1, 2023 •

edited

Loading

Atish-iaf Mar 7, 2023 •

edited

Loading