Implement the antctl framework #208

weiqiangt · 2019-12-08T06:27:49Z

Resolves #203.

jianjuns

Thanks for working on this.
Just got time to do a quick scan of the change but not in-depth review yet. I put some questions, but I think it could help review if you could add some comments to explain more the concepts in the code.

Makefile

jianjuns · 2019-12-10T19:32:25Z

build/yamls/base/antctl.yml

+  name: antctl
+rules:
+  - nonResourceURLs:
+      - /apis/antctl.antrea.io/v1


Could we use some generic naming instead of indicating this is for antctl only? Like clusterinfo, or controllerinfo/agentinfo or nodeinfo?
@tnqn: what you think?

Agree, they should be named with what they are, instead of who use them.

Thanks guys, but could you give me some suggestions for the name?
This endpoint was used for accessing antctl server side and we will add commands not related to info, like support-bundle.

I think we could use clusterinfo or now (or controllerinfo and agentinfo if we want to separate controller and agent/node).

Not sure about support-bundle. What API groups K8s uses for logging/metrics, etc.?

Changed to clusterinfo.antrea.tanzu.vmware.
The metrics in Kubelet does not have API group, it's a bare endpoint.

cmd/antctl/main.go

cmd/antrea-agent/agent.go

pkg/antctl/commandbundle.go

pkg/antctl/commandoption.go

pkg/antctl/handlers/version.go

build/yamls/antrea.yml

cmd/antctl/main.go

cmd/antrea-agent/agent.go

pkg/antctl/server.go

Makefile

jianjuns · 2019-12-17T01:57:27Z

build/yamls/base/antctl.yml

+  name: antctl
+rules:
+  - nonResourceURLs:
+      - /apis/antctl.antrea.io/v1


I think we could use clusterinfo or now (or controllerinfo and agentinfo if we want to separate controller and agent/node).

Not sure about support-bundle. What API groups K8s uses for logging/metrics, etc.?

.dockerignore

pkg/antctl/commandbundle.go

cmd/antrea-agent/agent.go

cmd/antrea-controller/controller.go

pkg/antctl/client.go

antoninbas

Sorry for the late review.

One global review comment is that more detailed comments would be useful and sometimes even comments for private functions are useful IMO. A lot of the function comments are just a translation of the function name into a short english sentence at the moment and I don't think that's very useful. For example:

// applyToCommand applies the CommandOption to a cobra.Command with the Client.
func (co *CommandOption) applyToCommand(cmd *cobra.Command, client *Client) {

If you look at K8s source code, most of the time you will see some actually useful information in the function comments (e.g. https://github.com/kubernetes/client-go/blob/master/tools/clientcmd/client_config.go#L541).

Another comment is that it would probably be useful to have some skeleton of documentation to accompany this PR (not detailed, just where can you expect to run the CLI binary for client / controller). We can then improve it over time.

Makefile

test/e2e/infra/vagrant/push_antrea.sh

test/e2e/antctl_test.go

pkg/antctl/commandoption.go

pkg/antctl/definitions.go

build/yamls/antrea-ipsec.yml

build/yamls/antrea.yml

build/yamls/base/controller.yml

cmd/antctl/main.go

pkg/antctl/client.go

pkg/antctl/command_list.go

pkg/antctl/handlers/interface.go

tnqn · 2020-01-10T09:51:43Z

Besides, I see server side code is still under pkg/antctl, do you plan to change it as @jianjuns suggested, or leave it to later improvement, or we want to keep it as is?

Co-Authored-By: Quan Tian <[email protected]>

weiqiangt · 2020-01-13T02:57:58Z

Besides, I see server side code is still under pkg/antctl, do you plan to change it as @jianjuns suggested, or leave it to later improvement, or we want to keep it as is?

I would like to refactor this in another PR.

Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt · 2020-01-14T09:26:11Z

/test-all

weiqiangt · 2020-01-14T13:37:49Z

Hi folks, thanks a lot for reviewing this PR. I have addressed all comments and created issues to track several future works of it. The document for the antctl will also be added in a separate PR(it was picked out from this PR).

tnqn

LGTM

antoninbas

Thanks for adding comments.

I mostly found typos, but also left a few other review comments.

pkg/antctl/client.go

pkg/antctl/antctl.go

pkg/antctl/handlers/version.go

test/e2e/antctl_test.go

Co-Authored-By: Antonin Bas <[email protected]>

Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt · 2020-01-15T17:02:30Z

/test-all

antoninbas

LGTM

jianjuns · 2020-01-16T00:05:06Z

Again, I believe we should decouple API server from antctl, but we can merge the current implementation, and change later (maybe after commands for runtime info are done).

weiqiangt · 2020-01-16T01:58:58Z

Again, I believe we should decouple API server from antctl, but we can merge the current implementation, and change later (maybe after commands for runtime info are done).

Copied.

This comment has been minimized.

Sign in to view

vmwclabot added the cla-not-required label Dec 8, 2019

weiqiangt force-pushed the weiqiangt/antctl-framework branch 4 times, most recently from 6374811 to 968ecbb Compare December 9, 2019 02:20

weiqiangt requested review from wenyingd, mengdie-song, antoninbas and tnqn December 9, 2019 02:44

jianjuns reviewed Dec 10, 2019

View reviewed changes

mengdie-song reviewed Dec 11, 2019

View reviewed changes

build/yamls/antrea.yml Outdated Show resolved Hide resolved

mengdie-song reviewed Dec 11, 2019

View reviewed changes

cmd/antctl/main.go Show resolved Hide resolved

mengdie-song reviewed Dec 11, 2019

View reviewed changes

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

mengdie-song reviewed Dec 11, 2019

View reviewed changes

pkg/antctl/server.go Outdated Show resolved Hide resolved

weiqiangt force-pushed the weiqiangt/antctl-framework branch 3 times, most recently from 066c81f to f33dbbe Compare December 13, 2019 02:51

jianjuns reviewed Dec 17, 2019

View reviewed changes

weiqiangt force-pushed the weiqiangt/antctl-framework branch 3 times, most recently from 9587a34 to b31ecd1 Compare December 17, 2019 06:25

tnqn reviewed Dec 17, 2019

View reviewed changes

antoninbas reviewed Dec 17, 2019

View reviewed changes

weiqiangt force-pushed the weiqiangt/antctl-framework branch from 37291fa to 93bc175 Compare December 18, 2019 15:41

This comment has been minimized.

Sign in to view

weiqiangt force-pushed the weiqiangt/antctl-framework branch 3 times, most recently from 751d804 to 783f8bb Compare December 23, 2019 04:41

weiqiangt force-pushed the weiqiangt/antctl-framework branch from e641ae0 to a04f639 Compare January 10, 2020 09:27

tnqn reviewed Jan 10, 2020

View reviewed changes

Apply suggestions from code review

44dd4e4

Co-Authored-By: Quan Tian <[email protected]>

weiqiangt force-pushed the weiqiangt/antctl-framework branch from d276077 to 91eb97f Compare January 13, 2020 07:59

Address review comments

cb6da3d

Signed-off-by: Weiqiang TANG <[email protected]>

weiqiangt force-pushed the weiqiangt/antctl-framework branch from 91eb97f to cb6da3d Compare January 13, 2020 11:53

weiqiangt requested review from antoninbas, tnqn, jianjuns, mengdie-song and vicky-liu January 14, 2020 13:37

tnqn previously approved these changes Jan 14, 2020

View reviewed changes

antoninbas reviewed Jan 14, 2020

View reviewed changes

Apply suggestions from code review

ba4bbe0

Co-Authored-By: Antonin Bas <[email protected]>

weiqiangt dismissed stale reviews from tnqn via ba4bbe0 January 15, 2020 15:55

weiqiangt and others added 3 commits January 16, 2020 00:12

Update pkg/antctl/command_definition.go

3e66db4

Co-Authored-By: Antonin Bas <[email protected]>

Apply suggestions from code review

2a40a7e

Co-Authored-By: Antonin Bas <[email protected]>

Address comments

054e19c

Signed-off-by: Weiqiang TANG <[email protected]>

antoninbas approved these changes Jan 15, 2020

View reviewed changes

weiqiangt merged commit 36e5e35 into antrea-io:master Jan 16, 2020

weiqiangt deleted the weiqiangt/antctl-framework branch January 16, 2020 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the antctl framework #208

Implement the antctl framework #208

weiqiangt commented Dec 8, 2019

This comment has been minimized.

jianjuns left a comment

jianjuns Dec 10, 2019

tnqn Dec 12, 2019

weiqiangt Dec 13, 2019

jianjuns Dec 17, 2019

weiqiangt Dec 17, 2019 •

edited

Loading

jianjuns Dec 17, 2019

antoninbas left a comment

This comment has been minimized.

tnqn commented Jan 10, 2020

weiqiangt commented Jan 13, 2020

weiqiangt commented Jan 14, 2020

weiqiangt commented Jan 14, 2020

tnqn left a comment

antoninbas left a comment

weiqiangt commented Jan 15, 2020

antoninbas left a comment

jianjuns commented Jan 16, 2020

weiqiangt commented Jan 16, 2020

Implement the antctl framework #208

Implement the antctl framework #208

Conversation

weiqiangt commented Dec 8, 2019

This comment has been minimized.

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns Dec 10, 2019

Choose a reason for hiding this comment

tnqn Dec 12, 2019

Choose a reason for hiding this comment

weiqiangt Dec 13, 2019

Choose a reason for hiding this comment

jianjuns Dec 17, 2019

Choose a reason for hiding this comment

weiqiangt Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

jianjuns Dec 17, 2019

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

This comment has been minimized.

tnqn commented Jan 10, 2020

weiqiangt commented Jan 13, 2020

weiqiangt commented Jan 14, 2020

weiqiangt commented Jan 14, 2020

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

weiqiangt commented Jan 15, 2020

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Jan 16, 2020

weiqiangt commented Jan 16, 2020

weiqiangt Dec 17, 2019 •

edited

Loading