Enhance Antrea-Octant-Plugin #913

ZhangYW18 · 2020-07-06T07:01:56Z

Enhance Traceflow graph color theme.
Before:

After:

Merge Antrea plugins into one plugin: Antrea Octant Plugin

Add two tabs in the Overview page of the plugin;
Move the original traceflow plugin page into the folder "Antrea".

Change the layout of Antrea Overview page
Before: Put all tabs together and show them in Overview page

After: Put all CRD forms in one page (this format fits with the default Octant Homepage)

antrea-bot · 2020-07-06T07:02:10Z

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.

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

mengdie-song · 2020-07-07T02:27:21Z

I like this simple color theme. For the dropped case, I actually prefer a red one and it is more like orange to me. But others may comment your preference.

ZhangYW18 · 2020-07-07T03:05:03Z

I like this simple color theme. For the dropped case, I actually prefer a red one and it is more like orange to me. But others may comment your preference.

If I change the colors in recent days, it may not correspond to the screenshots I provide to others, which will be used in seminars. Will this cause a problem?

mengdie-song · 2020-07-10T04:04:51Z

I like this simple color theme. For the dropped case, I actually prefer a red one and it is more like orange to me. But others may comment your preference.

If I change the colors in recent days, it may not correspond to the screenshots I provide to others, which will be used in seminars. Will this cause a problem?

I don't think it will be a big problem. But I think we can get more opinions on both the layout and color theme. @tnqn @jianjuns @antoninbas

jianjuns · 2020-07-10T04:47:18Z

To be frank I am not good at colors at all. But if you really like my opinion, I do prefer the latter version you have (simpler colors).

antoninbas · 2020-07-10T05:09:20Z

@ZhangYW18 don't worry about making adjustments if needed (e.g. making the drop case redder).
The new color theme looks good!

ZhangYW18 · 2020-07-13T02:25:36Z

Changed the color of drop case to pink. @mengdie-song @tnqn

mengdie-song · 2020-07-13T04:04:00Z

@ZhangYW18 Thanks for addressing my comment. Let's change the drop rectangle edge color to red as we discussed.

A problem I have noticed is that the focus will always be the last tab which is "Trace List". Unlike Controller or Agent, we may not have traceflow CRDs at start. I am not sure if we can change the focus to the first tab ("Controller") so that we will never get a blank list. Could you please also investigate this?

I see you put all the tabs on the navigation bar "Overview". It looks good for now because we only have four sub tabs at this time, but we may need to think more on the layout if we want to add more functions later.

ZhangYW18 · 2020-07-13T09:20:59Z

@ZhangYW18 Thanks for addressing my comment. Let's change the drop rectangle edge color to red as we discussed.

A problem I have noticed is that the focus will always be the last tab which is "Trace List". Unlike Controller or Agent, we may not have traceflow CRDs at start. I am not sure if we can change the focus to the first tab ("Controller") so that we will never get a blank list. Could you please also investigate this?

I see you put all the tabs on the navigation bar "Overview". It looks good for now because we only have four sub tabs at this time, but we may need to think more on the layout if we want to add more functions later.

I have changed the focus of "Overview" page to the first tab.

For the second issue, could we show all Antrea information in one tab in a unified Antrea Overview page, just like the Octant Cluster Overview page?

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

pkg/graphviz/traceflow.go

mengdie-song · 2020-07-14T02:47:27Z

@ZhangYW18 You can try the one tab way you mentioned here and we can see if it looks better. Anyway, the current layout seems fine since we only have four tabs now. I have added some other minor comments, please take a look.

ZhangYW18 · 2020-07-15T01:43:14Z

Here are two possible layouts of the Overview page for Antrea Octant Plugin:

Put all tabs together and show them in Overview page:

But we cannot always put all tabs together considering there may be more pages to show in the future.
Therefore, we are thinking about a new format:
Put all CRD forms in one page (this format fits with the default Octant Homepage) :

I'm not sure which one is better. Could I get your opinion?
@mengdie-song

mengdie-song · 2020-07-15T01:57:15Z

@ZhangYW18 Thanks for addressing my comments and listing these two options!
I prefer the second one which puts all CRD forms in one page since it fits with the default Octant Homepage and we can always get the traceflow UI from navigation bar on the left.
The first option which puts all the tabs on the overview page looks a little bit redundant to me since we have already put them on the left column. Also, we may add more features on UI in the future and we cannot put each of them as a tab on overview page.

@jianjuns @antoninbas @tnqn Could you share your opinions on these two overview page design?

ZhangYW18 · 2020-07-15T02:19:03Z

@ZhangYW18 Thanks for addressing my comments and listing these two options!
I prefer the second one which puts all CRD forms in one page since it fits with the default Octant Homepage and we can always get the traceflow UI from navigation bar on the left.
The first option which puts all the tabs on the overview page looks a little bit redundant to me since we have already put them on the left column. Also, we may add more features on UI in the future and we cannot put each of them as a tab on overview page.

@jianjuns @antoninbas @tnqn Could you share your opinions on these two overview page design?

Actually the two cases share the same navigation bar and the navigation bar can be folded. I have updated the images.

jianjuns · 2020-07-20T21:45:42Z

/test-all

I assume this PR is ready to be merged?

jianjuns · 2020-07-20T22:35:22Z

/test-all

ZhangYW18 · 2020-07-21T02:07:51Z

/test-all

I assume this PR is ready to be merged?

Yes, it is.

plugins/octant/cmd/antrea-octant-plugin/antrea_info.go

tnqn · 2020-07-22T05:11:43Z

I noticed there are some differences in tab title and link.
the name of the plugin should be "Antrea" instead of "Antrea Information";
the name of the overview page should be "Overview" instead of "Antrea Overview";
the title of the overview page should be "Antrea" instead of "Antrea Overview", but this is minor.
By clicking the plugin, the overview page should be displayed instead of an empty page.

ZhangYW18 · 2020-07-22T07:35:27Z

I noticed there are some differences in tab title and link.
the name of the plugin should be "Antrea" instead of "Antrea Information";
the name of the overview page should be "Overview" instead of "Antrea Overview";
the title of the overview page should be "Antrea" instead of "Antrea Overview", but this is minor.
By clicking the plugin, the overview page should be displayed instead of an empty page.

All fixed. Updated the image in the description as well.

mengdie-song · 2020-07-22T08:29:59Z

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

-			log.Printf("Failed to generate content from CRD, lastTfName %v, err: %s", a.lastTfName, err)
-			return component.ContentResponse{}, nil
+			log.Printf("Failed to get latest CRD, last traceflow name: %s, err: %s", lastTf.Name, err)
+			log.Printf("Using traceflow results cache, traceflow name: %s", lastTf.Name)


I believe we can merge these two logs into one.
@tnqn We change the logic here to handle the case that Traceflow CRD is cleaned up. But I am not sure what we want to show in this case, a blank graph or a previous traceflow graph. Could you also share your opinion here?

I don't have preference on this, both work for me.

OK, then we can keep the logic here for now to show the previous trace graph if we can not successfully get the CRD.

mengdie-song · 2020-07-22T08:32:42Z

You can also remove target "antrea-traceflow-plugin" from Makefile in this patch.

tnqn · 2020-07-22T08:37:34Z

The name of the Traceflow tab (the pop up window that has 4 links) should be "Traceflow" instead of "Traceflow Info".

ZhangYW18 · 2020-07-22T09:23:08Z

The name of the Traceflow tab (the pop up window that has 4 links) should be "Traceflow" instead of "Traceflow Info".

Updated.

ZhangYW18 · 2020-07-22T09:24:23Z

You can also remove target "antrea-traceflow-plugin" from Makefile in this patch.

Updated.

jianjuns · 2020-07-22T22:26:26Z

/test-all

mengdie-song

LGTM

tnqn

LGTM, will merge after all verification done

tnqn · 2020-07-24T12:57:52Z

/skip-windows-networkpolicy
/skip-windows-conformance

mengdie-song reviewed Jul 7, 2020

View reviewed changes

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

ZhangYW18 force-pushed the traceflow branch from ca14ff1 to 2b5c079 Compare July 7, 2020 03:49

antoninbas mentioned this pull request Jul 9, 2020

Update octant-plugin installation guide #914

Merged

ZhangYW18 force-pushed the traceflow branch 2 times, most recently from 0eb8fed to 9a12b43 Compare July 13, 2020 02:15

ZhangYW18 force-pushed the traceflow branch from 9a12b43 to 8a8bb37 Compare July 13, 2020 03:53

ZhangYW18 force-pushed the traceflow branch 2 times, most recently from 6ff407e to 053c012 Compare July 13, 2020 09:17