Update traceflow graph message for service destinations #1607

ZhangYW18 · 2020-12-03T05:31:10Z

Add destination pod name & IP in load balancing stage when destination is a service.

Example graph:

antrea-bot · 2020-12-03T05:31:23Z

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-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/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-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-12-03T06:08:31Z

Codecov Report

Merging #1607 (d3d495a) into master (9d3d10b) will decrease coverage by 1.46%.
The diff coverage is 53.45%.

@@            Coverage Diff             @@
##           master    #1607      +/-   ##
==========================================
- Coverage   63.31%   61.85%   -1.47%     
==========================================
  Files         170      181      +11     
  Lines       14250    16656    +2406     
==========================================
+ Hits         9023    10302    +1279     
- Misses       4292     5309    +1017     
- Partials      935     1045     +110

Flag	Coverage Δ
e2e-tests	`37.10% <25.71%> (?)`
kind-e2e-tests	`52.94% <51.71%> (-2.44%)`	⬇️
unit-tests	`40.55% <12.30%> (-0.73%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
cmd/antrea-agent/options.go	`26.89% <0.00%> (+6.17%)`	⬆️
...gent/controller/noderoute/node_route_controller.go	`46.60% <ø> (+0.13%)`	⬆️
.../agent/flowexporter/connections/conntrack_linux.go	`29.48% <0.00%> (+0.32%)`	⬆️
pkg/agent/stats/collector.go	`92.04% <ø> (-5.69%)`	⬇️
pkg/controller/networkpolicy/tier.go	`100.00% <ø> (+10.00%)`	⬆️
pkg/features/antrea_features.go	`16.66% <ø> (ø)`
pkg/ovs/openflow/ofctrl_builder.go	`60.94% <0.00%> (-1.23%)`	⬇️
pkg/ovs/openflow/ofctrl_group.go	`48.57% <0.00%> (-4.56%)`	⬇️
pkg/ovs/openflow/ofctrl_action.go	`51.26% <5.71%> (-10.10%)`	⬇️
... and 82 more

mengdie-song · 2020-12-03T09:48:25Z

pkg/graphviz/traceflow.go

@@ -177,8 +177,14 @@ func getTraceflowMessage(o *opsv1alpha1.Observation) string {
 	if o.Component == opsv1alpha1.NetworkPolicy && len(o.NetworkPolicy) > 0 {
 		str += "\nNetpol: " + o.NetworkPolicy
 	}
+	if len(o.Pod) > 0 {
+		str += "\nTo: " + o.Pod


After taking a look at the example graph, I would suggest to use "To Pod" here.

mengdie-song

Thanks for making the enhancement. From my point, a general suggestion would be adding a little bit details for each field shown on the graph to avoid any confusion.

pkg/graphviz/traceflow.go

tnqn

Thanks for the improvement. However the destination is a little confusing to me as the service name, not sure if others have the same feeling.

jianjuns

Do you think we should show the Pod name in the destination box (either just Pod name, or both Service and Pod names)?

jianjuns · 2020-12-07T22:25:12Z

Thanks for the improvement. However the destination is a little confusing to me as the service name, not sure if others have the same feeling.

Just saw @tnqn's reply after adding the same comment.

ZhangYW18 · 2020-12-08T09:35:06Z

Do you think we should show the Pod name in the destination box (either just Pod name, or both Service and Pod names)?

Maybe we can replace service name with pod name, and show service name below:

namespaceName/podName
service: serviceName

ZhangYW18 · 2020-12-08T09:50:43Z

Have updated the IP-related names as well as the example graph.

mengdie-song · 2020-12-08T10:11:15Z

pkg/graphviz/traceflow.go

+		str += "\nTo: " + o.Pod
+	}
+	if o.Action != opsv1alpha1.Dropped && len(o.TranslatedDstIP) > 0 {
+		str += "\nTranslated Destination IP: " + o.TranslatedDstIP


I am not sure the exact meaning of TranslatedDstIP here, maybe @gran-vmv can give some details for this field, so that we can enhance UI description for this.
Will it always be Pod IP? Or in the case where destination is service, it will be service IP instead?

AFAIK, TranslatedDstIP is only set for service access and it's the destination pod ip.

Got it, thanks Quan.

ZhangYW18 · 2020-12-10T02:54:41Z

@gran-vmv Would you review this patch or not?

tnqn · 2020-12-10T04:22:43Z

Do you think we should show the Pod name in the destination box (either just Pod name, or both Service and Pod names)?

Maybe we can replace service name with pod name, and show service name below:
namespaceName/podName
service: serviceName

@ZhangYW18 have you applied this change? I didn't see it from the example and the code.

ZhangYW18 · 2020-12-10T05:00:17Z

Do you think we should show the Pod name in the destination box (either just Pod name, or both Service and Pod names)?

Maybe we can replace service name with pod name, and show service name below:
namespaceName/podName
service: serviceName
@ZhangYW18 have you applied this change? I didn't see it from the example and the code.

No, I didn't. I'm still waiting for more suggestions.

tnqn · 2020-12-10T06:57:55Z

maybe you can go ahead with the approach of including both service and pod information.

ZhangYW18 · 2020-12-10T08:32:18Z

Have updated the destination name in service case.

jianjuns

The new design looks good to me.

tnqn

LGTM, thanks

tnqn · 2020-12-11T03:58:29Z

/test-all

tnqn · 2020-12-11T07:38:42Z

/skip-all since this touches octant plugin code only.

tnqn · 2020-12-11T07:39:40Z

@mengdie-song before merging it, do you have other comments since you reviewed this as well?

mengdie-song

LGTM

vmwclabot added the cla-not-required label Dec 3, 2020

ZhangYW18 marked this pull request as draft December 3, 2020 05:31

ZhangYW18 requested review from gran-vmv, tnqn and mengdie-song December 3, 2020 09:18

ZhangYW18 added this to the Antrea v0.12.0 release milestone Dec 3, 2020

ZhangYW18 force-pushed the tf-ui branch from f18e9f1 to e75658c Compare December 3, 2020 09:20

ZhangYW18 marked this pull request as ready for review December 3, 2020 09:20

ZhangYW18 force-pushed the tf-ui branch from e75658c to e06a735 Compare December 3, 2020 09:26

mengdie-song reviewed Dec 3, 2020

View reviewed changes

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

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

tnqn reviewed Dec 3, 2020

View reviewed changes

jianjuns reviewed Dec 7, 2020

View reviewed changes

ZhangYW18 force-pushed the tf-ui branch from e06a735 to 9411633 Compare December 8, 2020 09:49

mengdie-song reviewed Dec 8, 2020

View reviewed changes

update traceflow graph message for service

d3d495a

ZhangYW18 force-pushed the tf-ui branch from 9411633 to d3d495a Compare December 10, 2020 08:30

jianjuns reviewed Dec 10, 2020

View reviewed changes

tnqn approved these changes Dec 11, 2020

View reviewed changes

mengdie-song approved these changes Dec 15, 2020

View reviewed changes

tnqn merged commit fedf14c into antrea-io:master Dec 15, 2020

antoninbas pushed a commit that referenced this pull request Dec 23, 2020

update traceflow graph message for service (#1607)

d8012b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update traceflow graph message for service destinations #1607

Update traceflow graph message for service destinations #1607

ZhangYW18 commented Dec 3, 2020 •

edited

Loading

antrea-bot commented Dec 3, 2020

codecov-io commented Dec 3, 2020 •

edited

Loading

mengdie-song Dec 3, 2020

mengdie-song left a comment

tnqn left a comment

jianjuns left a comment

jianjuns commented Dec 7, 2020

ZhangYW18 commented Dec 8, 2020

ZhangYW18 commented Dec 8, 2020 •

edited

Loading

mengdie-song Dec 8, 2020

tnqn Dec 15, 2020

mengdie-song Dec 15, 2020

ZhangYW18 commented Dec 10, 2020

tnqn commented Dec 10, 2020

ZhangYW18 commented Dec 10, 2020

tnqn commented Dec 10, 2020

ZhangYW18 commented Dec 10, 2020

jianjuns left a comment

tnqn left a comment

tnqn commented Dec 11, 2020

tnqn commented Dec 11, 2020

tnqn commented Dec 11, 2020

mengdie-song left a comment

Update traceflow graph message for service destinations #1607

Update traceflow graph message for service destinations #1607

Conversation

ZhangYW18 commented Dec 3, 2020 • edited Loading

antrea-bot commented Dec 3, 2020

codecov-io commented Dec 3, 2020 • edited Loading

Codecov Report

mengdie-song Dec 3, 2020

Choose a reason for hiding this comment

mengdie-song left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns commented Dec 7, 2020

ZhangYW18 commented Dec 8, 2020

ZhangYW18 commented Dec 8, 2020 • edited Loading

mengdie-song Dec 8, 2020

Choose a reason for hiding this comment

tnqn Dec 15, 2020

Choose a reason for hiding this comment

mengdie-song Dec 15, 2020

Choose a reason for hiding this comment

ZhangYW18 commented Dec 10, 2020

tnqn commented Dec 10, 2020

ZhangYW18 commented Dec 10, 2020

tnqn commented Dec 10, 2020

ZhangYW18 commented Dec 10, 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 Dec 11, 2020

tnqn commented Dec 11, 2020

tnqn commented Dec 11, 2020

mengdie-song left a comment

Choose a reason for hiding this comment

ZhangYW18 commented Dec 3, 2020 •

edited

Loading

codecov-io commented Dec 3, 2020 •

edited

Loading

ZhangYW18 commented Dec 8, 2020 •

edited

Loading