antctl-query #10

jakesokol1 · 2020-07-29T00:51:40Z

No description provided.

antoninbas

@jakesokol1 we should be good to open the upstream PR after these comments are addressed. I didn't review the server part in details, since I did several reviews earlier.

You may have questions about some comments (e.g. the one about the Transform). Ping me on Slack if it is the case and we can have a Zoom.

pkg/antctl/antctl.go

pkg/antctl/command_definition.go

antoninbas · 2020-07-29T04:53:21Z

pkg/antctl/command_definition.go

 		// Do not limit the column length for a single column table.
 		// This is for the case a single column table can have long rows which cannot
 		// fit into a single line (one example is the ovsflows outputs).
 		widths[0] = 0
 	} else {
 		// Get the width of every column.
-		for j := 0; j < numColumns; j++ {
+		for j := 0; j < numCol; j++ {


maybe the renaming from Columns to Col is not strictly needed and just makes the diff larger?

I renamed to numCol for consistency with the refactored version (in getColumnWidths).

that's fine by me, but it looks like it should be numCols and not numCol. Otherwise it's not consistent with numRows.

pkg/controller/networkpolicy/store/appliedtogroup.go

test/e2e/infra/vagrant/Vagrantfile

test/e2e/infra/vagrant/push_antrea.sh

antoninbas · 2020-07-29T05:42:33Z

pkg/controller/querier/querier.go

+//TODO: expand this interface similarly to AgentQuerier
 type ControllerQuerier interface {
 	GetControllerInfo(controllInfo *v1beta1.AntreaControllerInfo, partial bool)
 }

+//TODO: implement interface methods
 type controllerQuerier struct {
 	networkPolicyInfoQuerier querier.ControllerNetworkPolicyInfoQuerier
+	endpointQuerier          networkpolicy.EndpointQuerier
 	apiPort                  int
 }

-func NewControllerQuerier(networkPolicyInfoQuerier querier.ControllerNetworkPolicyInfoQuerier, apiPort int) *controllerQuerier {
-	return &controllerQuerier{networkPolicyInfoQuerier: networkPolicyInfoQuerier, apiPort: apiPort}
+func NewControllerQuerier(networkPolicyInfoQuerier querier.ControllerNetworkPolicyInfoQuerier,
+	endPointQuerier networkpolicy.EndpointQuerier, apiPort int) *controllerQuerier {
+	return &controllerQuerier{networkPolicyInfoQuerier: networkPolicyInfoQuerier,
+		endpointQuerier: endPointQuerier, apiPort: apiPort}
 }

-// GetNodeSubnet gets current network policy info querier.
 func (cq controllerQuerier) getNetworkPolicyInfoQuerier() querier.ControllerNetworkPolicyInfoQuerier {
 	return cq.networkPolicyInfoQuerier
 }

+func (cq controllerQuerier) getEndpointQuerierReplier() networkpolicy.EndpointQuerier {
+	return cq.endpointQuerier
+}
+


with the current state of the code, we actually do not use this anywhere. So we should either remove your changes to this file, or have a small refactoring in cmd/antrea-controller/controller.go to leverage this.

The easiest solution is definitely to just remove this code, so I suggest we go this route. Keep this code somewhere in another branch (rebase-2 is fine) in case we need it later. In particular, I think the refactoring would involve moving createAPIServerConfig to pkg/apiserver, and making the instantiation of the Agent apiserver more similar to the instantiation of the Controller apiserver. Probably a bit of a tricky project at this stage.

I was also confused by the TODOs, but it's irrelevant if we just remove the code

I do need to include the changes in NewControllerQuerier correct?

Actually never mind, it is never used

Makefile

pkg/antctl/transform/endpoint/transform.go

antoninbas · 2020-07-30T00:07:42Z

pkg/controller/networkpolicy/store/addressgroup.go

+			if !ok {
+				return []string{}, nil
+			}
+			keys := make([]string, 0)


that's right my bad

antoninbas · 2020-07-30T00:09:30Z

pkg/antctl/command_definition.go

 		// Do not limit the column length for a single column table.
 		// This is for the case a single column table can have long rows which cannot
 		// fit into a single line (one example is the ovsflows outputs).
 		widths[0] = 0
 	} else {
 		// Get the width of every column.
-		for j := 0; j < numColumns; j++ {
+		for j := 0; j < numCol; j++ {


that's fine by me, but it looks like it should be numCols and not numCol. Otherwise it's not consistent with numRows.

antoninbas · 2020-07-30T00:10:36Z

pkg/controller/networkpolicy/controller_query_perf_test.go

+	_, querier := makeControllerAndEndpointQueryReplier(objs...)
+	// Everything is ready, now start timing.
+	start := time.Now()
+	// track execution time by calling query endpoint 1000 times on random pods


that's a shortcut (sed syntax) for: replace the "pods" spelling with "Pods"

antoninbas · 2020-07-30T00:11:33Z

pkg/antctl/transform/endpoint/transform.go

+	Name string
+}
+
+type Response struct {


@jakesokol1 this is my most important comment I think. Are you working on addressing it?

antoninbas

more comments

pkg/antctl/command_definition.go

pkg/controller/apiserver/handlers/handler_test.go

Makefile

pkg/controller/networkpolicy/controller_query_test.go

pkg/controller/querier/querier.go

antoninbas · 2020-07-31T20:34:10Z

pkg/controller/querier/querier.go

@@ -41,7 +41,6 @@ func NewControllerQuerier(networkPolicyInfoQuerier querier.ControllerNetworkPoli
 	return &controllerQuerier{networkPolicyInfoQuerier: networkPolicyInfoQuerier, apiPort: apiPort}
 }

-// GetNodeSubnet gets current network policy info querier.


don't remove the comment, fix the method name instead

antoninbas · 2020-07-31T20:36:10Z

hack/update-codegen-dockerized.sh

@@ -91,7 +91,7 @@ MOCKGEN_TARGETS=(
  "pkg/ovs/ovsctl OVSCtlClient"
  "pkg/agent/querier AgentQuerier"
  "pkg/controller/querier ControllerQuerier"
-  "pkg/querier AgentNetworkPolicyInfoQuerier"
+  "pkg/querier AgentNetworkPolicyInfoQuerier,ControllerNetworkPolicyInfoQuerier"


I don't think you need to generate a mock for ControllerNetworkPolicyInfoQuerier. If yes, where are you using it?

However, you do need to generate the mock for EndpointQuerier. I see that you have the generated mock file checked-in, but I don't know how it was generated since hack/update-codegen-dockerized.sh does not reference EndpointQuerier.

antoninbas · 2020-07-31T20:36:28Z

pkg/agent/cniserver/ipam/testing/mock_ipam.go

@@ -1,4 +1,4 @@
-// Copyright 2019 Antrea Authors
+// Copyright YEAR Antrea Authors


I am not sure how this happened, Ill change it back

Whenever a run make codegen it switches back to YEAR

Not able to reproduce locally. But I have observed that if I manually set it to YEAR and commit that change, then it will leave it as YEAR. Even if you don't explicitly set it to YEAR, that could happen if the script failed halfway because of some in correct changes.

antoninbas · 2020-07-31T20:37:57Z

pkg/controller/apiserver/handlers/handler_test.go

+import (
+	"encoding/json"
+	"github.com/vmware-tanzu/antrea/pkg/controller/networkpolicy"
+	v1 "k8s.io/api/admission/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+	"github.com/vmware-tanzu/antrea/pkg/controller/apiserver/handlers/endpoint"
+	queriermock "github.com/vmware-tanzu/antrea/pkg/controller/networkpolicy/testing"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)


we usually group imports as follows (you may want to fix it before opening the upstream PR, otherwise someone is likely to comment about it):

go standard imports

whiteline

all third-party imports

whiteline

antrea imports

Antctl query endpoint gives users the ability to filter network policies relevant to a certain endpoint. The user is able to query internal controller stores using the antrea controller apiserver through antctl. To answer queries in an efficient way, we add new indexers to the internal stores of the controller, incurring a small memory overhead (~20%, depends on structure of NetworkPolicies). Note, antctl query endpoint only works when run in the controller. We are working on getting it to work out of cluster. Fixes antrea-io#974

antoninbas reviewed Jul 29, 2020

View reviewed changes

antoninbas reviewed Jul 30, 2020

View reviewed changes

jakesokol1 force-pushed the antctl-query branch 2 times, most recently from b3e0c11 to 2aa69bd Compare July 31, 2020 18:57

antoninbas reviewed Jul 31, 2020

View reviewed changes

jakesokol1 force-pushed the antctl-query branch from d985769 to 1c92c61 Compare July 31, 2020 21:47

jakesokol1 force-pushed the antctl-query branch from e31feb0 to 7e1e4b3 Compare July 31, 2020 23:11

jakesokol1 added 6 commits August 4, 2020 14:55

address comments

bb6d158

move handler installation to pkg/apiserver/apiserver.go

c7c913e

address comments

7f932fd

move handler to apiserver

beee1a8

address comments

f035d3f

revert to index free implementation

bbc7096

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

antctl-query #10

antctl-query #10

jakesokol1 commented Jul 29, 2020

antoninbas left a comment

antoninbas Jul 29, 2020

jakesokol1 Jul 29, 2020

antoninbas Jul 30, 2020

antoninbas Jul 29, 2020

antoninbas Jul 29, 2020

jakesokol1 Jul 30, 2020

jakesokol1 Jul 30, 2020

antoninbas Jul 30, 2020

antoninbas Jul 30, 2020

antoninbas Jul 30, 2020

antoninbas Jul 30, 2020

antoninbas left a comment

antoninbas Jul 31, 2020

antoninbas Jul 31, 2020

antoninbas Jul 31, 2020

jakesokol1 Jul 31, 2020

jakesokol1 Jul 31, 2020

antoninbas Jul 31, 2020

antoninbas Jul 31, 2020

		@@ -1,4 +1,4 @@
		// Copyright 2019 Antrea Authors
		// Copyright YEAR Antrea Authors

antctl-query #10

Are you sure you want to change the base?

antctl-query #10

Conversation

jakesokol1 commented Jul 29, 2020

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

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