Implement antctl agent-info #318

lzhecheng · 2020-01-16T07:29:11Z

Issue #275

Output should be like this:

{
  "metadata": {
    "name": "antrea-k8s",
    "selfLink": "/apis/clusterinformation.antrea.tanzu.vmware.com/v1beta1/antreaagentinfos/",
    "uid": "11111111",
    "resourceVersion": "11111111",
    "generation": 36,
    "creationTimestamp": "2020-02-04T10:50:20Z"
  },
  "version": "v0.3.0-dev-4452e66.dirty",
  "podRef": {
    "kind": "Pod",
    "namespace": "kube-system",
    "name": "antrea-agent-0"
  },
  "nodeRef": {
    "kind": "Node",
    "name": "node-k8s1-1"
  },
  "nodeSubnet": [
    "192.168.1.0/24"
  ],
  "ovsInfo": {
    "version": "1.0",
    "bridgeName": "br-int",
    "flowTable": {
      "0": 5,
      "10": 7,
    }
  },
  "networkPolicyControllerInfo": {},
  "localPodNum": 2,
  "agentConditions": [
    {
      "type": "AgentHealthy",
      "status": "True",
      "lastHeartbeatTime": "2020-02-04T11:25:20Z"
    },
  ]
}

antrea-bot · 2020-01-16T07:29:28Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-all: to trigger all tests.
/skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

lzhecheng · 2020-01-16T07:35:12Z

/test-all

lzhecheng · 2020-01-16T08:33:09Z

/test-all

antoninbas

Would be great to be able to merge this before the 0.3.0 release (Jan 22nd), this way we would have at least one useful CLI command.

antoninbas · 2020-01-16T18:46:28Z

pkg/antctl/handlers/agentinfo.go

+var _ Factory = new(AgentInfo)
+
+// ComponentAgentInfoResponse describes the internal response struct of agent-info command.
+// It only contains agent's node subnet and connection status of controller, OVSDB and openflow.


I think you can remove this second line of comment: what is included in the response is clear from the structure definition and we should not have to update the struct-level comment every time we add some extra information to the response. However, you may want to add some comments to the struct members when needed (e.g.: "Subnet is the subnet range from the Pod CIDR allocated to the Node" or something like this).

I agree. Updated.

antoninbas · 2020-01-16T18:53:43Z

pkg/antctl/handlers/agentinfo.go

+// It only contains agent's node subnet and connection status of controller, OVSDB and openflow.
+type ComponentAgentInfoResponse struct {
+	ControllerConn string `json:"controllerConn,omitempty" yaml:"controllerConn,omitempty"`
+	OVSDBConn      string `json:"ovsdbConn,omitempty" yaml:"ovsdbConn,omitempty"`


Do you think we can do something like this, which is more in line with the K8s style for serializable structs:

type ConnStatus string const ( ConnStatusUp ConnStatus = "UP" ConnStatusDown ConnStatus = "DOWN" ) type ComponentAgentInfoResponse struct { ControllerConn ConnStatus `json:"controllerConn,omitempty" yaml:"controllerConn,omitempty"` // ... }

This makes it clear that the "*Conn" fields can only be set to "UP" or "DOWN" and helps document the response IMO.

Yes, it is better. I have updated the code. I put ConnStatus at package monitor because maybe other cli will need this.

antoninbas · 2020-01-16T18:54:02Z

pkg/antctl/handlers/agentinfo.go

+type AgentInfo struct{}
+
+// Handler returns the function which can handle queries issued by agent-info commands,
+// the handler function populate component's agnet-info to the ComponentAgentInfoResponse.


typo: s/agnet-info/agent-info

antoninbas · 2020-01-16T18:54:22Z

pkg/antctl/handlers/agentinfo_test.go

+		controllerConn     string
+		ovsdbConn          string
+		ofConn             string
+		subent             string


typo: s/subent/subnet (here and below)

antoninbas · 2020-01-16T18:55:30Z

pkg/antctl/antctl.go

+		{
+			Use:                 "agent-info",
+			Short:               "Print agent's information",
+			Long:                "Print agent's information of the antctl.",


What does "of the antctl" mean here? Maybe we can come up with a better help message.

Updated the message.

weiqiangt · 2020-01-17T03:38:35Z

pkg/antctl/antctl.go

+			GroupVersion:        &systemGroup,
+			TransformedResponse: reflect.TypeOf(transformedAgentInfoResponse{}),
+			Agent:               true,
+			Controller:          false,


nit: no need to specify negative values.

Thanks. I have removed "Controller" and "AddTransform" fields.

lzhecheng · 2020-01-17T05:30:14Z

/test-all

jianjuns · 2020-01-18T01:00:46Z

pkg/antctl/handlers/agentinfo.go

+// ComponentAgentInfoResponse describes the internal response struct of agent-info command.
+// The first three fields refer to connection between the agent and controller, OVSDB and
+// openflow. Subnet refers to the subnet range from the Pod CIDR allocated to the Node.
+type ComponentAgentInfoResponse struct {


Is it possible to reuse the AgentInfo CRD struct?
@mengdie-song

That covers more than what we need here.

I know it is more than what this PR supports, but does it make sense to show all in the AgentInfo CRD with this get agent-info cmd?

I got what you mean now. Shall we just dump these important info for this milestone ? I will discuss with Mengdie about dumping all info afterwards ?

Ok to me if we can catch the 0.3.0 release, but I do hope we revisit all CLI design items after 0.3.0.
Ideally we should keep API/CLI compatible across some number of releases, but maybe not a big deal at this stage.

@jianjuns @lzhecheng Sorry the late review. We can dump all the information in AntreaAgentInfo via this API. Seems this patch now focuses more on the condition information.
Maybe this CLI output can follow the struct AntreaAgentInfo, put these condition information in a subcategory like Agent Conditions?

Since CRD will only be updated every minute, the information stored in CRD is not up to date.
I would suggest to follow https://github.com/vmware-tanzu/antrea/blob/master/pkg/monitor/monitor.go#L267 to get all needed information by CLI itself.

@jianjuns @mengdie-song Thank you. I use struct AntreaAgentInfo as response now and updated the code.

pkg/antctl/antctl.go

lzhecheng · 2020-01-20T01:56:32Z

/test-all

lzhecheng · 2020-01-20T02:30:14Z

/test-all

tnqn

LGTM, will let @jianjuns give the approval since he has made a change request about the response.

tnqn · 2020-01-20T11:18:00Z

pkg/antctl/handlers/agentinfo_test.go

+	"net/http/httptest"
+	"testing"
+
+	monitor "github.com/vmware-tanzu/antrea/pkg/monitor"


The alias is not needed, though generated code has it.

It seems generated code doesn’t have ConnStatus.

Sorry, I didn't get your mean, I mean the alias "monitor" is not needed

My bad. I misunderstood. Alias removed.

lzhecheng · 2020-01-21T05:12:43Z

/test-all

antoninbas

LGTM
It seems that at the moment we can only query the agent-info from within the agent Pod. Is there any plan to support getting that information from outside the cluster using the kubeconfig and the Node's name?

antctl -k <path to kubeconfig> agent-info <node name>

pkg/antctl/handlers/agentinfo.go

lzhecheng · 2020-01-21T22:18:15Z

@antoninbas yes, we are going to implement remote mode for antctl. These implemented are planned for this milestone.

lzhecheng · 2020-01-22T00:17:09Z

/test-all

lzhecheng · 2020-02-03T08:31:12Z

@jianjuns can we merge this local mode command ? I think the redesign is more about remote mode.

jianjuns · 2020-02-03T20:01:15Z

@jianjuns can we merge this local mode command ? I think the redesign is more about remote mode.
Remote mode can be implemented separately, and I assume wont impact much your current code. However, I would suggest you to consider changing to reuse the Agent monitor CRD struct and show CRD fields. Could you evaluate that?

Last, I hope after the Agent/Controller info commands, you guys can revisit the design, and decouple API server implementation from antctl, as (again and again) the API will be consumed by other clients besides antctl, and we need to make it generic. If we continue to add more and more commands based on the current framework, it will be become harder to change later.
@weiqiangt @tnqn

jianjuns

@lzhecheng : can you provide an example output of this command?

jianjuns · 2020-02-05T00:40:51Z

pkg/antctl/handlers/agentinfo.go

+var _ Factory = new(AgentInfo)
+
+// AgentInfo is the implementation of the Factory interface for the agent-info command.
+type AgentInfo struct{}


Humm...
Question - what is the convention of this handler struct. Do we assume it includes the real result data and how about other commands? If so, should we add AntreaAgentInfo into the struct?

I have put an example in the PR description.

This struct doesn't contain data or result. It is only for command handler below:
func (v *AgentInfo) Handler(aq monitor.AgentQuerier, cq monitor.ControllerQuerier) http.HandlerFunc

mengdie-song · 2020-02-05T01:40:53Z

pkg/monitor/querier.go

@@ -151,6 +152,16 @@ func (monitor *agentMonitor) GetAgentConditions(ovsConnected bool) []v1beta1.Age
 	}
 }

+// GetAgentInfo gets the antrea agent information.
+func (monitor *agentMonitor) GetAgentInfo() *v1beta1.AntreaAgentInfo {
+	info, err := monitor.client.ClusterinformationV1beta1().AntreaAgentInfos().Get(monitor.GetSelfNode().Name, metav1.GetOptions{})


As I have mentioned previously, you cannot get the latest information for CLI if you query CRD directly.
In my point of view, CLI should get the latest information rather than the information which may be a minute late.
If so, you can extract the AntreaAgentInfo construction part of code here from monitor.go.
@jianjuns @tnqn What do you think?

I would second what @mengdie-song said.

Thank you Mengdie and Jianjun! I have updated the code.

tnqn · 2020-02-06T02:15:40Z

pkg/antctl/handlers/agentinfo.go

+// the handler function populate component's agent-info to the response.
+func (v *AgentInfo) Handler(aq monitor.AgentQuerier, cq monitor.ControllerQuerier) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		var m *v1beta1.AntreaAgentInfo


why is m the variable name? what is it standing for?

Thank you for reminding me. I changed it to "info" and I think it makes more sense.

tnqn · 2020-02-06T03:07:10Z

pkg/monitor/querier.go

+// GetAgentInfo gets the antrea agent information.
+func (monitor *agentMonitor) GetAgentInfo() *v1beta1.AntreaAgentInfo {
+	return monitor.getAntreaAgentInfo(monitor.GetSelfNode().Name)
+}


The methods GetAgentInfo, getAntreaAgentInfo, getAgentCRD in monitor would be confusing, maybe you can just use GetAgentInfo as name of getAntreaAgentInfo method.

Yes, I changed them to GetAgentInfo().

tnqn · 2020-02-06T07:25:09Z

pkg/monitor/monitor.go

@@ -264,12 +264,12 @@ func (monitor *agentMonitor) getAgentCRD(crdName string) *v1beta1.AntreaAgentInf
 	return agentCRD
 }

-func (monitor *agentMonitor) createAgentCRD(crdName string) (*v1beta1.AntreaAgentInfo, error) {
+func (monitor *agentMonitor) GetAgentInfo(name string) *v1beta1.AntreaAgentInfo {


I don't see the argument name is necessary, it's kept as name field of agentMonitor, why we are passing it and have to make clients fetch the name via another method first? So do createAgentCRD

tnqn

LGTM

lzhecheng · 2020-02-06T10:37:38Z

/test-all

jianjuns · 2020-02-06T22:53:52Z

In the outputs, the "metadata" field looks strange, which should not be needed for CLI outputs, and it is also confusing like we are dumping a K8s resource. Do we have a way to exclude a field?
If not, maybe we should consider to define a new struct, almost copying the AgentInfo CRD but no the metadata field (sorry I suggested to reuse AgentInfo but I did not know we will end up print all fields).

lzhecheng · 2020-02-07T02:11:40Z

In the outputs, the "metadata" field looks strange, which should not be needed for CLI outputs, and it is also confusing like we are dumping a K8s resource. Do we have a way to exclude a field?
If not, maybe we should consider to define a new struct, almost copying the AgentInfo CRD but no the metadata field (sorry I suggested to reuse AgentInfo but I did not know we will end up print all fields).

@jianjuns I think we'd better use a new struct. TypeMeta and ObjectMeta only have json omitempty tag but we have other output format like yaml, so leaving these value unset won't help.

jianjuns · 2020-02-07T04:24:12Z

@jianjuns I think we'd better use a new struct. TypeMeta and ObjectMeta only have json omitempty tag but we have other output format like yaml, so leaving these value unset won't help.

Ok. If no way to filter the fields, probably let us still create a new struct. Sorry for the confusion.

jianjuns

LGTM

tnqn

LGTM

tnqn · 2020-02-13T08:39:59Z

/test-all

jianjuns · 2020-02-15T01:13:30Z

@lzhecheng : do you plan to make any further changes for this PR? I hope it can get into 0.4.0.

lzhecheng · 2020-02-15T04:00:37Z

@lzhecheng : do you plan to make any further changes for this PR? I hope it can get into 0.4.0.

@jianjuns I am ok to merge it. But there will be change after the antctl framework is refactored. If you want to have this command for 0.4.0 and then we can merge it. Shall we ?

lzhecheng · 2020-02-15T04:00:50Z

/test-networkpolicy

tnqn · 2020-02-17T06:54:41Z

It failed on #386. If it keeps failing, you can rebase on master.

/test-networkpolicy

vmwclabot added the cla-not-required label Jan 16, 2020

lzhecheng changed the title ~~Implement antctl agent-info~~ WIP: Implement antctl agent-info Jan 16, 2020

lzhecheng force-pushed the agentinfo branch from b6af33e to ccf0031 Compare January 16, 2020 08:08

lzhecheng changed the title ~~WIP: Implement antctl agent-info~~ Implement antctl agent-info Jan 16, 2020

antoninbas reviewed Jan 16, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch from ccf0031 to 35c3274 Compare January 17, 2020 02:56

weiqiangt reviewed Jan 17, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch from 35c3274 to 0f3d654 Compare January 17, 2020 05:16

jianjuns reviewed Jan 18, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch 2 times, most recently from 09fb408 to 4809274 Compare January 20, 2020 01:55

lzhecheng requested review from weiqiangt and vicky-liu January 20, 2020 01:57

lzhecheng force-pushed the agentinfo branch from 4809274 to 9408433 Compare January 20, 2020 02:07

tnqn reviewed Jan 20, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch from 9408433 to 4452e66 Compare January 21, 2020 03:40

antoninbas reviewed Jan 21, 2020

View reviewed changes

pkg/antctl/handlers/agentinfo.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the agentinfo branch from 55a0fa2 to 92058db Compare January 21, 2020 22:25

antoninbas mentioned this pull request Jan 22, 2020

Publish antctl binaries for different platforms in releases #312

Closed

lzhecheng force-pushed the agentinfo branch from 61f0de7 to 341fac0 Compare February 4, 2020 11:32

jianjuns reviewed Feb 5, 2020

View reviewed changes

mengdie-song reviewed Feb 5, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch 2 times, most recently from 4fa65b1 to bb8aae1 Compare February 5, 2020 02:51

tnqn reviewed Feb 6, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch 2 times, most recently from 926fa8f to 3368d94 Compare February 6, 2020 07:18

tnqn reviewed Feb 6, 2020

View reviewed changes

lzhecheng force-pushed the agentinfo branch from 3368d94 to 42cd680 Compare February 6, 2020 09:21

lzhecheng requested review from mengdie-song and tnqn February 6, 2020 09:24

lzhecheng force-pushed the agentinfo branch from 42cd680 to a00582f Compare February 6, 2020 09:31

tnqn previously approved these changes Feb 6, 2020

View reviewed changes

Implement antctl agent-info

c4ffdda

lzhecheng dismissed tnqn’s stale review via c4ffdda February 7, 2020 02:32

lzhecheng force-pushed the agentinfo branch from a00582f to c4ffdda Compare February 7, 2020 02:32

jianjuns approved these changes Feb 11, 2020

View reviewed changes

tnqn approved these changes Feb 13, 2020

View reviewed changes

lzhecheng merged commit d742467 into antrea-io:master Feb 21, 2020

lzhecheng deleted the agentinfo branch February 21, 2020 00:26

Implement antctl agent-info #318

Implement antctl agent-info #318

Conversation

lzhecheng commented Jan 16, 2020 • edited Loading

antrea-bot commented Jan 16, 2020

lzhecheng commented Jan 16, 2020

lzhecheng commented Jan 16, 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

Choose a reason for hiding this comment

lzhecheng commented Jan 17, 2020

Choose a reason for hiding this comment

lzhecheng Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng Jan 20, 2020 • 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

lzhecheng commented Jan 20, 2020

lzhecheng commented Jan 20, 2020

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

lzhecheng Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

lzhecheng commented Jan 21, 2020

antoninbas left a comment

Choose a reason for hiding this comment

lzhecheng commented Jan 21, 2020

lzhecheng commented Jan 22, 2020

lzhecheng commented Feb 3, 2020

jianjuns commented Feb 3, 2020

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

tnqn left a comment

Choose a reason for hiding this comment

lzhecheng commented Feb 6, 2020

jianjuns commented Feb 6, 2020

lzhecheng commented Feb 7, 2020

jianjuns commented Feb 7, 2020

jianjuns left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Feb 13, 2020

jianjuns commented Feb 15, 2020

lzhecheng commented Feb 15, 2020 • edited Loading

lzhecheng commented Feb 15, 2020

tnqn commented Feb 17, 2020

lzhecheng commented Jan 16, 2020 •

edited

Loading

lzhecheng Jan 20, 2020 •

edited

Loading

lzhecheng Jan 20, 2020 •

edited

Loading

lzhecheng Jan 21, 2020 •

edited

Loading

lzhecheng commented Feb 15, 2020 •

edited

Loading