Add Traceflow Octant-Plugin #841

ZhangYW18 · 2020-06-16T15:52:56Z

Add a Traceflow octant-plugin for Antrea.

Some sample Traceflow graphs:

Some screenshots of the plugin:

Note:
This PR depends on below PR, the commit is included. If reviewers have any comments on Traceflow CRD, please go to below link:
#660 Traceflow CRD

antrea-bot · 2020-06-16T15:53:08Z

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-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance 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.

mengdie-song · 2020-06-17T01:50:18Z

Actually, this patch only relies on traceflow CRD definition patch and has no dependency with the other two traceflow implementation patches. I will update this part.

pkg/graphviz/traceflow.go

mengdie-song · 2020-06-17T13:25:29Z

cmd/traceflow-plugin/main.go

+	}
+
+	switch actionName {
+	case addTfAction:


We need to update this part of logic.

Add the field name and so that we can trace several times without deleting the previous CRD.

Support more protocol and our previous hard code is just for function verification.

We need to update this part of logic.

Add the field name and so that we can trace several times without deleting the previous CRD.

Support more protocol and our previous hard code is just for function verification.

For the first point, what if the user inputs duplicate traceflow name? As I have not found a good way to inform users like pop-ups, now I just replace the old traceflow with the new one.

Yiwei's point makes sense to me. @gran-vmv Do you think we need this extra field? Or maybe we could keep the previous logic by constructing the name by source and destination, then if a duplicate one already exists, we can always delete and recreate a new one?

Yiwei's point makes sense to me. @gran-vmv Do you think we need this extra field? Or maybe we could keep the previous logic by constructing the name by source and destination, then if a duplicate one already exists, we can always delete and recreate a new one?

I think we can use hash code. Add a random string at the end of each traceflow name.

Can we use timestamp for this?

@ZhangYW18 Please also check when Pod name contains dot "."

Updated traceflow name to "srcPod-dstPod-YYYYMMDD-HHMMSS".
Regex check for strings is also added.

mengdie-song

I have added some comments, please take a look.
We can check in this one after #838.
In this way, we will use the latest octant which has fixed graphviz performance issues. And since we can also fix the position of source graph and destination graph now, traceflow-plugin can be changed to auto refresh graphviz.

ZhangYW18 · 2020-06-22T00:50:57Z

Actually, this patch only relies on traceflow CRD definition patch and has no dependency with the other two traceflow implementation patches. I will update this part.

Have updated the description.

antrea-bot · 2020-06-22T00:52:32Z

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-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

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

gran-vmv

Please remove commit "Implemented Antrea traceflow controller and agent" from this PR since there is no dependency.
And fix the failed checks except starting with "jenkins-".

pkg/graphviz/traceflow.go

mengdie-song · 2020-06-23T07:38:57Z

The change to move octant-plugins to separate folder has been checked in. Please move traceflow octant-plugin to the same folder. This patch should have no dependencies with other commit now.

build/yamls/antrea-octant.yml

jianjuns

I still prefer a single plugin, but if that means too big a design change, we can make the change later.

Besides this, the PR looks good to me. Just have some minor comments.

plugins/octant/Makefile

plugins/octant/cmd/traceflow-plugin/main.go

pkg/graphviz/traceflow.go

plugins/octant/cmd/antrea-traceflow-plugin/main.go

mengdie-song · 2020-07-01T04:29:36Z

plugins/octant/cmd/antrea-traceflow-plugin/main.go

+}
+
+// This is antrea-traceflow-plugin.
+// The plugin needs to run with octant version v0.10.2 or later.


Let's remove this line since we will run it with 0.13.1

mengdie-song · 2020-07-01T04:34:18Z

plugins/octant/cmd/antrea-traceflow-plugin/main.go

+		component.NewFormFieldHidden("action", showGraphAction),
+	}}
+	genGraph := component.Action{
+		Name:  "Generate Trace Graph",


Since we can support automatically refreshment for the latest traceflow, do you think it is better that we change this to " Generate Historical Trace Graph"?

historical?

Since we can support automatically refreshment for the latest traceflow, do you think it is better that we change this to " Generate Historical Trace Graph"?

“LatestTfName” will change to not only the name of the most recently created traceflow, but also the name of the most recently shown traceflow. If so, we have to modify this part of logic.

Do you mean "past"/"previous" instead of "historical"?

I don't think we need to change this part of logic. It is just that "Start New Trace" will actually create a CRD and generate its trace graph. "Generate Historical Trace Graph" will generate the trace graph for any traceflow CRD given its name whether it is the latest one or not. BTW, do yo think maybe "lastTfName" can represent better than "latestTfName"? And we will always show graphviz for "lastTfName".

I don't think we need to change this part of logic. It is just that "Start New Trace" will actually create a CRD and generate its trace graph. "Generate Historical Trace Graph" will generate the trace graph for any traceflow CRD given its name whether it is the latest one or not. BTW, do yo think maybe "lastTfName" can represent better than "latestTfName"? And we will always show graphviz for "lastTfName".

"Latest" implies the most recently created one. As far as I'm concerned, we do need to change the name.

pkg/graphviz/traceflow.go

mengdie-song · 2020-07-02T02:58:04Z

plugins/octant/cmd/antrea-traceflow-plugin/main.go

@@ -0,0 +1,404 @@
+package main


Header added.

tnqn · 2020-07-02T03:04:31Z

Could we not use the pink color, and perhaps less colors overall, then it would look more serious..
But I'm definitely not good at this, just personal feeling.

mengdie-song · 2020-07-02T03:07:05Z

@ZhangYW18 Minor comments for copyright. The change looks good to me overall.
I agree with folks that single plugin would be better, but you can change this later considering the schedule and effort.

tnqn · 2020-07-02T03:09:18Z

@ZhangYW18 Minor comments for copyright. The change looks good to me overall.
I agree with folks that single plugin would be better, but you can change this later considering the schedule and effort.
We can merge the current traceflow UI implementation after traceflow implementation patch.

Will both of them be released in release package? I'm a little concerned about the number of bits given each one will have multiple platform versions

mengdie-song · 2020-07-02T03:16:27Z

@tnqn I am thinking only include antrea-octant-plugin for now. Since we plan to merge this antrea-traceflow-plugin to antrea-octant-plugin later and traceflow is disabled by default right now, we can provide the antrea-octant-plugin with both features later, maybe 0.9? What do you think?

tnqn · 2020-07-02T03:18:31Z

@tnqn I am thinking only include antrea-octant-plugin for now. Since we plan to merge this antrea-traceflow-plugin to antrea-octant-plugin later and traceflow is disabled by default right now, we can provide the antrea-octant-plugin with both features later, maybe 0.9? What do you think?

Sounds good to me, thanks

ZhangYW18 · 2020-07-02T05:16:28Z

Could we not use the pink color, and perhaps less colors overall, then it would look more serious..
But I'm definitely not good at this, just personal feeling.

Maybe switch some light colors to dark colors can make things better. What do you think of this color theme?

antoninbas · 2020-07-02T19:35:32Z

/skip-all

jianjuns · 2020-07-02T23:11:19Z

Per discussions offline with @tnqn @antoninbas @McCodeman, merge this PR to be included in 0.8.0. We should continue to add some user documentation (and maybe some demo recording/screenshots in the documents).

mengdie-song reviewed Jun 17, 2020

View reviewed changes

pkg/graphviz/traceflow.go Outdated Show resolved Hide resolved

mengdie-song reviewed Jun 17, 2020

View reviewed changes

pkg/graphviz/traceflow.go Show resolved Hide resolved

mengdie-song reviewed Jun 17, 2020

View reviewed changes

gran-vmv requested review from gran-vmv, tnqn and jianjuns June 18, 2020 10:17

ZhangYW18 force-pushed the traceflow-0616 branch from 8e8f736 to 0090edd Compare June 22, 2020 00:52

ZhangYW18 force-pushed the traceflow-0616 branch from 0090edd to 4d1ac6b Compare June 22, 2020 08:09

gran-vmv reviewed Jun 22, 2020

View reviewed changes

pkg/graphviz/traceflow.go Outdated Show resolved Hide resolved

ZhangYW18 force-pushed the traceflow-0616 branch 2 times, most recently from f204e91 to 833e4fd Compare June 23, 2020 07:37

ZhangYW18 force-pushed the traceflow-0616 branch from 833e4fd to 8fafe48 Compare June 23, 2020 07:38

mengdie-song reviewed Jun 23, 2020

View reviewed changes

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

ZhangYW18 force-pushed the traceflow-0616 branch 11 times, most recently from 0d518f0 to 968a6d5 Compare June 29, 2020 04:42

ZhangYW18 force-pushed the traceflow-0616 branch from aafb8ca to 6b986af Compare June 30, 2020 08:21

jianjuns reviewed Jul 1, 2020

View reviewed changes

ZhangYW18 force-pushed the traceflow-0616 branch 3 times, most recently from 34ff6c0 to 3277bca Compare July 1, 2020 03:14

mengdie-song reviewed Jul 1, 2020

View reviewed changes

plugins/octant/cmd/antrea-traceflow-plugin/main.go Outdated Show resolved Hide resolved

mengdie-song reviewed Jul 1, 2020

View reviewed changes

ZhangYW18 force-pushed the traceflow-0616 branch 3 times, most recently from 3f4fa46 to 5a4b477 Compare July 1, 2020 07:24

mengdie-song reviewed Jul 2, 2020

View reviewed changes

pkg/graphviz/traceflow.go Show resolved Hide resolved

mengdie-song reviewed Jul 2, 2020

View reviewed changes

ZhangYW18 force-pushed the traceflow-0616 branch from 5a4b477 to 77b53c0 Compare July 2, 2020 03:06

ZhangYW18 force-pushed the traceflow-0616 branch from 77b53c0 to 7327e8e Compare July 2, 2020 03:14

Add traceflow octant-plugin

e176138

ZhangYW18 force-pushed the traceflow-0616 branch from 7327e8e to e176138 Compare July 2, 2020 03:16

jianjuns approved these changes Jul 2, 2020

View reviewed changes

jianjuns merged commit 0448530 into antrea-io:master Jul 2, 2020

jianjuns mentioned this pull request Jul 2, 2020

Fix go.mod tidiness checking #904

Merged

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020

Add traceflow octant-plugin (antrea-io#841)

7708a6a

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 23, 2020

Add traceflow octant-plugin (antrea-io#841)

004ccdc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Traceflow Octant-Plugin #841

Add Traceflow Octant-Plugin #841

ZhangYW18 commented Jun 16, 2020 •

edited

Loading

antrea-bot commented Jun 16, 2020

mengdie-song commented Jun 17, 2020 •

edited

Loading

mengdie-song Jun 17, 2020

ZhangYW18 Jun 22, 2020

mengdie-song Jun 22, 2020

ZhangYW18 Jun 22, 2020

gran-vmv Jun 22, 2020

gran-vmv Jun 22, 2020

ZhangYW18 Jul 1, 2020

mengdie-song left a comment

ZhangYW18 commented Jun 22, 2020

antrea-bot commented Jun 22, 2020

gran-vmv left a comment •

edited

Loading

mengdie-song commented Jun 23, 2020

jianjuns left a comment

mengdie-song Jul 1, 2020

ZhangYW18 Jul 1, 2020

mengdie-song Jul 1, 2020 •

edited

Loading

jianjuns Jul 1, 2020

ZhangYW18 Jul 1, 2020

ZhangYW18 Jul 1, 2020

mengdie-song Jul 1, 2020

ZhangYW18 Jul 1, 2020

mengdie-song Jul 2, 2020

ZhangYW18 Jul 2, 2020

tnqn commented Jul 2, 2020

mengdie-song commented Jul 2, 2020 •

edited

Loading

tnqn commented Jul 2, 2020

mengdie-song commented Jul 2, 2020

tnqn commented Jul 2, 2020

ZhangYW18 commented Jul 2, 2020 •

edited

Loading

antoninbas commented Jul 2, 2020

jianjuns commented Jul 2, 2020

Add Traceflow Octant-Plugin #841

Add Traceflow Octant-Plugin #841

Conversation

ZhangYW18 commented Jun 16, 2020 • edited Loading

antrea-bot commented Jun 16, 2020

mengdie-song commented Jun 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengdie-song left a comment

Choose a reason for hiding this comment

ZhangYW18 commented Jun 22, 2020

antrea-bot commented Jun 22, 2020

gran-vmv left a comment • edited Loading

Choose a reason for hiding this comment

mengdie-song commented Jun 23, 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

mengdie-song Jul 1, 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

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 commented Jul 2, 2020

mengdie-song commented Jul 2, 2020 • edited Loading

tnqn commented Jul 2, 2020

mengdie-song commented Jul 2, 2020

tnqn commented Jul 2, 2020

ZhangYW18 commented Jul 2, 2020 • edited Loading

antoninbas commented Jul 2, 2020

jianjuns commented Jul 2, 2020

ZhangYW18 commented Jun 16, 2020 •

edited

Loading

mengdie-song commented Jun 17, 2020 •

edited

Loading

gran-vmv left a comment •

edited

Loading

mengdie-song Jul 1, 2020 •

edited

Loading

mengdie-song commented Jul 2, 2020 •

edited

Loading

ZhangYW18 commented Jul 2, 2020 •

edited

Loading