Use DSCP instead of tun_metadata0 in Traceflow #1466

gran-vmv · 2020-10-30T11:31:40Z

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and
removes the tunnel/encap mode restriction for inter-Node Traceflow.

This PR closes #1357

antrea-bot · 2020-10-30T11:31:55Z

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-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-10-30T11:35:30Z

Codecov Report

Merging #1466 into master will increase coverage by 0.23%.
The diff coverage is 87.93%.

@@            Coverage Diff             @@
##           master    #1466      +/-   ##
==========================================
+ Coverage   68.24%   68.47%   +0.23%     
==========================================
  Files         165      165              
  Lines       13107    13118      +11     
==========================================
+ Hits         8945     8983      +38     
+ Misses       3226     3195      -31     
- Partials      936      940       +4

Flag	Coverage Δ
integration-tests	`45.64% <16.27%> (-0.17%)`	⬇️
kind-e2e-tests	`55.00% <87.93%> (+0.13%)`	⬆️
unit-tests	`42.38% <4.65%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/agent.go	`50.14% <ø> (+0.28%)`	⬆️
pkg/controller/traceflow/controller.go	`67.74% <ø> (+2.58%)`	⬆️
pkg/agent/controller/traceflow/packetin.go	`64.17% <57.14%> (+15.36%)`	⬆️
pkg/agent/openflow/client.go	`68.10% <83.33%> (+0.24%)`	⬆️
...agent/controller/traceflow/traceflow_controller.go	`82.60% <100.00%> (+5.97%)`	⬆️
pkg/agent/openflow/pipeline.go	`82.92% <100.00%> (+0.32%)`	⬆️
...ver/registry/controlplane/nodestatssummary/rest.go	`50.00% <0.00%> (-50.00%)`	⬇️
pkg/agent/stats/collector.go	`91.95% <0.00%> (-5.75%)`	⬇️
...kg/controller/networkpolicy/store/networkpolicy.go	`77.58% <0.00%> (-3.45%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`71.14% <0.00%> (-0.80%)`	⬇️
... and 5 more

tnqn

LGTM overall, some minor comments

pkg/agent/openflow/client.go

pkg/controller/traceflow/controller.go

pkg/agent/controller/traceflow/packetin.go

pkg/agent/openflow/pipeline.go

antoninbas · 2020-11-02T21:28:51Z

pkg/agent/openflow/pipeline.go

+	// OfTraceflowMarkRange stores dataplaneTag at range 2-7 in DSCP field of IP header.
+	// IPv4/v6 DSCP (Bits 2-7) Field supports exact match only.
+	OfTraceflowMarkRange = binding.Range{2, 7}


I find this a bit confusing. The DSCP field is bits 2 through 7 of the IP ToS field. I don't think "range 2-7 in DSCP field" makes sense.

We are only using this constant in the code below:

packetOutBuilder = packetOutBuilder.AddLoadAction(binding.NxmFieldIPTos, uint64(dataplaneTag), OfTraceflowMarkRange)

it may make more sense to define the constant locally where it is used, rather than here in the middle of the "register ranges"

BTW, isn't there an OVS field for DSCP we can use directly? OXM_OF_IP_DSCP? @wenyingd

Renamed to traceflowMarkToSRange, move to a separate section and updated comments.
I prefer to store all ranges in same place, because it is easy to query.

Yes, OXM_OF_IP_DSCP is what we want, will test with this

Tested OXM_OF_IP_DSCP, still requires range{0, 5}. We don't need to move to this.

pkg/controller/traceflow/controller.go

jianjuns · 2020-11-02T22:06:15Z

FYI. - I think OVS folks are evaluating the tun_metadata fix and promised to give us an update today. It might still be possible to have the fix for Antrea 0.11.

tnqn

Code LGTM, but would let team decide whether we go this way or wait for OVS fix.

vicky-liu · 2020-11-04T01:55:30Z

OVS made a fix on kernel. https://lore.kernel.org/netdev/1604448694-19351-1-git-send-email-yihung.wei@gmail.com/T/#u

vicky-liu · 2020-11-04T02:15:42Z

@jianjuns @antoninbas , The fix made by OVS team requires the latest kernel version, I think it's a strict limitation for users to enable Antrea Traceflow. I still prefer to leverage DSCP to support Traceflow for both encap and no-encap. Please share your concerns about this solution.

antoninbas · 2020-11-04T02:20:23Z

@vicky-liu that was my original concern, that the fix would require a patch to the OVS kernel datapath

yes, we should move forward with the DSCP workaround

jianjuns · 2020-11-04T03:13:14Z

Sure. Let us go the PR for 0.11. We can decide if we want to change to DSCP + INT in future releases.

jianjuns

Do you plan to support TF for NoEncap and NetworkPolicyOnly mode? If so, we should update the AgentOptions.validate(), and also the antrea-agent.conf.

pkg/controller/traceflow/controller.go

pkg/ovs/openflow/interfaces.go

pkg/agent/controller/traceflow/packetin.go

pkg/agent/agent.go

jianjuns · 2020-11-04T03:26:38Z

pkg/agent/controller/traceflow/packetin.go

+				return nil, nil, err
+			}
+		}
+		if outputPort == config.DefaultTunOFPort || outputPort == config.HostGatewayOFPort {


Question - if the destination IP of the TF packet is indeed the gw0 IP, what should be the expected behavior here? And the action should be forwarded or delivered?

Currently we restrict destination must be Pod. If the destination is other IP, we cannot identify if the packet is properly delivered.

In TraceflowSpec the destination could be an IP? And if it is the Pod IP, I think we still track "delivered"?

In theory we can handle the case destination IP is gw0 IP too. If you want not to support that in this PR, probably add a comment in InstallTraceflowFlows() to explain that.

Even when the IP is an external one, I think antrea-controller should handle it (as we know it is forwarded through gw0), and not to report timeout failure.

We can handle this case by checking whether the dstIP is in Pod cache to identify if the action is Delivered or Forwarded, but I think we should submit another patch after this patch merged.

pkg/agent/openflow/pipeline.go

antrea-bot · 2020-11-04T04:12:26Z

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).

gran-vmv · 2020-11-04T05:17:15Z

/test-all

antoninbas

minor comments only, lgtm

pkg/agent/agent.go

pkg/agent/controller/traceflow/packetin.go

pkg/agent/openflow/pipeline.go

antoninbas · 2020-11-06T01:41:02Z

pkg/agent/openflow/pipeline.go

+func (c *client) traceflowL2ForwardOutputFlows(dataplaneTag uint8, category cookie.Category) []binding.Flow {
+	flows := []binding.Flow{}
+	// Output and SendToController if output port is tunnel or gateway port.
+	// The gw0 IP as Traceflow destination is not supported.


we should open an issue for this, in order to document the limitation

Submitted #1500 for this enhancement.

pkg/controller/traceflow/controller.go

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and remove the tunnel/encap mode restriction for inter-Node Traceflow.

gran-vmv · 2020-11-06T04:55:59Z

/test-all

gran-vmv · 2020-11-06T06:32:05Z

/test-networkpolicy

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and remove the tunnel/encap mode restriction for inter-Node Traceflow.

jianjuns · 2020-11-10T02:08:51Z

Do you plan to support TF for NoEncap and NetworkPolicyOnly mode? If so, we should update the AgentOptions.validate(), and also the antrea-agent.conf.

Seems I forgot to confirm this one. Could you answer? @gran-vmv

gran-vmv · 2020-11-10T02:12:47Z

Do you plan to support TF for NoEncap and NetworkPolicyOnly mode? If so, we should update the AgentOptions.validate(), and also the antrea-agent.conf.

Seems I forgot to confirm this one. Could you answer? @gran-vmv

I checked Traceflow in noEncap mode, and it can work. I think AgentOptions.validate() doesn't validate traceflow options, and the validation in agent/traceflow/traceflow_comtroller is removed by this PR.

jianjuns · 2020-11-10T02:18:56Z

Then at least change the yaml and documentation.

gran-vmv · 2020-11-10T02:28:01Z

Then at least change the yaml and documentation.

Currently no changes needed for yaml and doc.
In current Traceflow doc, the low level tech is not mentioned such as tun_metadata/DSCP, and the interNode restriction is not mentioned too. After this PR merged, the interNode restriction is removed and we don't need to change this unless we want to add some details into this doc.

jianjuns · 2020-11-10T03:25:18Z

Ok. You are right. I thought we commented in the yaml that TF requires encap and Geneve.

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and remove the tunnel/encap mode restriction for inter-Node Traceflow.

gran-vmv added the status/WIP Work in progress label Oct 30, 2020

vmwclabot added the cla-not-required label Oct 30, 2020

gran-vmv force-pushed the tf-dscp branch from f6ec611 to 0085c26 Compare November 2, 2020 08:36

gran-vmv requested review from jianjuns, wenyingd, tnqn and antoninbas November 2, 2020 09:22

gran-vmv removed the status/WIP Work in progress label Nov 2, 2020

tnqn reviewed Nov 2, 2020

View reviewed changes

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

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

antoninbas reviewed Nov 2, 2020

View reviewed changes

gran-vmv force-pushed the tf-dscp branch 2 times, most recently from 06d430d to 88e864e Compare November 3, 2020 02:48

gran-vmv requested review from antoninbas and tnqn November 3, 2020 05:51

tnqn reviewed Nov 3, 2020

View reviewed changes

jianjuns reviewed Nov 4, 2020

View reviewed changes

gran-vmv force-pushed the tf-dscp branch from 88e864e to afae4de Compare November 4, 2020 04:12

gran-vmv requested review from tnqn and jianjuns November 4, 2020 09:40

gran-vmv added this to the Antrea v0.11.0 release milestone Nov 5, 2020

antoninbas reviewed Nov 6, 2020

View reviewed changes

gran-vmv force-pushed the tf-dscp branch 2 times, most recently from c9e6634 to 7a67b5f Compare November 6, 2020 04:08

Use DSCP instead of tun_metadata0 in Traceflow

b37bf6f

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and remove the tunnel/encap mode restriction for inter-Node Traceflow.

gran-vmv force-pushed the tf-dscp branch from 7a67b5f to b37bf6f Compare November 6, 2020 04:55

jianjuns approved these changes Nov 6, 2020

View reviewed changes

gran-vmv merged commit 619b678 into antrea-io:master Nov 6, 2020

gran-vmv mentioned this pull request Nov 6, 2020

Set tun_metadata0 for non-Traceflow traffics #1410

Closed

antoninbas pushed a commit that referenced this pull request Nov 11, 2020

Use DSCP instead of tun_metadata0 in Traceflow (#1466)

c540240

This patch changes Traceflow dataplaneTag from tun_metadata0 to DSCP, and remove the tunnel/encap mode restriction for inter-Node Traceflow.

gran-vmv deleted the tf-dscp branch January 21, 2021 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DSCP instead of tun_metadata0 in Traceflow #1466

Use DSCP instead of tun_metadata0 in Traceflow #1466

gran-vmv commented Oct 30, 2020 •

edited

Loading

antrea-bot commented Oct 30, 2020

codecov-io commented Oct 30, 2020 •

edited

Loading

tnqn left a comment

antoninbas Nov 2, 2020

gran-vmv Nov 3, 2020

gran-vmv Nov 3, 2020

jianjuns commented Nov 2, 2020

tnqn left a comment

vicky-liu commented Nov 4, 2020

vicky-liu commented Nov 4, 2020

antoninbas commented Nov 4, 2020

jianjuns commented Nov 4, 2020

jianjuns left a comment

jianjuns Nov 4, 2020

gran-vmv Nov 4, 2020

jianjuns Nov 4, 2020

gran-vmv Nov 4, 2020

antrea-bot commented Nov 4, 2020

gran-vmv commented Nov 4, 2020

antoninbas left a comment

antoninbas Nov 6, 2020

gran-vmv Nov 6, 2020

gran-vmv commented Nov 6, 2020

gran-vmv commented Nov 6, 2020

jianjuns commented Nov 10, 2020

gran-vmv commented Nov 10, 2020

jianjuns commented Nov 10, 2020

gran-vmv commented Nov 10, 2020

jianjuns commented Nov 10, 2020

Use DSCP instead of tun_metadata0 in Traceflow #1466

Use DSCP instead of tun_metadata0 in Traceflow #1466

Conversation

gran-vmv commented Oct 30, 2020 • edited Loading

antrea-bot commented Oct 30, 2020

codecov-io commented Oct 30, 2020 • edited Loading

Codecov Report

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

jianjuns commented Nov 2, 2020

tnqn left a comment

Choose a reason for hiding this comment

vicky-liu commented Nov 4, 2020

vicky-liu commented Nov 4, 2020

antoninbas commented Nov 4, 2020

jianjuns commented Nov 4, 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

antrea-bot commented Nov 4, 2020

gran-vmv commented Nov 4, 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

gran-vmv commented Nov 6, 2020

gran-vmv commented Nov 6, 2020

jianjuns commented Nov 10, 2020

gran-vmv commented Nov 10, 2020

jianjuns commented Nov 10, 2020

gran-vmv commented Nov 10, 2020

jianjuns commented Nov 10, 2020

gran-vmv commented Oct 30, 2020 •

edited

Loading

codecov-io commented Oct 30, 2020 •

edited

Loading