Provide generic API for OVS register match/set value #2455

wenyingd · 2021-07-23T05:23:24Z

Use RegMark/RegField in OpenFlow Match or Load actions instead of passing the register ID and range.
The usage for OVS registers(including xxreg) should be predefined in regmarks.go.
Use reg0[0..3] to indicate the source of packet, and release reg0[4..15] for other usages.

Signed-off-by: wenyingd wenyingd@vmware.com

codecov-commenter · 2021-07-23T08:40:29Z

Codecov Report

Merging #2455 (b8636eb) into main (a9552a1) will increase coverage by 4.78%.
The diff coverage is 78.29%.

@@            Coverage Diff             @@
##             main    #2455      +/-   ##
==========================================
+ Coverage   60.38%   65.16%   +4.78%     
==========================================
  Files         282      283       +1     
  Lines       22411    25773    +3362     
==========================================
+ Hits        13532    16796    +3264     
+ Misses       7452     7429      -23     
- Partials     1427     1548     +121

Flag	Coverage Δ
e2e-tests	`55.75% <74.03%> (?)`
kind-e2e-tests	`47.41% <69.85%> (+0.05%)`	⬆️
unit-tests	`41.96% <12.91%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/ovs/openflow/ofctrl_group.go	`45.76% <55.55%> (-0.19%)`	⬇️
pkg/ovs/openflow/ofctrl_nxfields.go	`61.70% <61.70%> (ø)`
pkg/ovs/openflow/ofctrl_action.go	`68.50% <74.28%> (+18.08%)`	⬆️
pkg/ovs/openflow/ofctrl_builder.go	`62.50% <81.81%> (+4.45%)`	⬆️
pkg/agent/controller/traceflow/packetin.go	`69.75% <83.33%> (+4.77%)`	⬆️
pkg/agent/openflow/pipeline.go	`83.17% <84.34%> (+8.31%)`	⬆️
pkg/agent/controller/networkpolicy/packetin.go	`71.21% <87.50%> (+0.38%)`	⬆️
.../flowexporter/connections/conntrack_connections.go	`82.44% <100.00%> (+1.14%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`81.02% <100.00%> (+0.79%)`	⬆️
pkg/agent/openflow/client.go	`70.16% <100.00%> (+11.65%)`	⬆️
... and 278 more

jianjuns

I like the approach. Just a few minor comments.

And in the commit description:

lease reg0[4..15] for other usages.

lease -> release

pkg/agent/controller/networkpolicy/packetin.go

pkg/ovs/openflow/interfaces.go

pkg/agent/openflow/regmarks.go

jianjuns · 2021-07-27T01:09:50Z

pkg/ovs/openflow/ofctrl_action.go

@@ -470,7 +488,7 @@ func (a *ofLearnAction) MatchLearnedDstIPv6() LearnAction {
 }

 // MatchReg makes the learned flow to match the data in the reg of specific range.
-func (a *ofLearnAction) MatchReg(regID int, data uint32, rng Range) LearnAction {
+func (a *ofLearnAction) MatchReg(regID int, data uint32, rng *Range) LearnAction {
 	toField := &ofctrl.LearnField{Name: fmt.Sprintf("NXM_NX_REG%d", regID), Start: uint16(rng[0])}


Do you think it is a good idea to save the string for the predefined regs?

I could add "name" as a property in RegField, but I don't know how to leverage this property..

How about saving the LearnField?

I didn't get your point about "saving the LearnField". Here LearnField is a type defined in ofnet.

pkg/ovs/openflow/interfaces.go

jianjuns · 2021-07-27T18:35:11Z

pkg/ovs/openflow/interfaces.go

+	name  string
+}
+
+// RegMark uses a RegField to cache a fixed value or value enumeration. The RegMark is used to indicate the traffic


How about: "RegMark is a value saved in a RegField"?

Do you feel RegFieldValue is easier to understand? I have no strong preference. Would let you decide.

Personally, I like RegMark. I think mark is always a fixed value to indicate something and value needs to use a field to store, which is what I want to use this object to express. RegFieldValue sounds more like any value we set in a RegField.

pkg/ovs/openflow/interfaces.go

wenyingd · 2021-07-30T09:45:32Z

/test-all

wenyingd · 2021-08-02T01:09:01Z

/test-all

wenyingd · 2021-08-03T02:27:46Z

/test-networkpolicy
/test-windows-networkpolicy
/test-ipv6-all
/test-ipv6-only-all
/test-all-features-conformance

wenyingd · 2021-08-03T09:18:05Z

/test-ipv6-networkpolicy
/test-ipv6-only-e2e
/test-windows-networkpolicy

jianjuns

LGTM.

pkg/ovs/openflow/interfaces.go

wenyingd · 2021-08-05T10:07:29Z

/test-all

wenyingd · 2021-08-11T12:12:41Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

pkg/agent/openflow/fields.go

GraysonWu

LGTM overall, just some suggestions on CustomReason comments.

wenyingd · 2021-08-12T03:08:46Z

/test-all
/test-ipv6-all
/test-ipv6-only-all
/test-integration

wenyingd · 2021-08-12T07:36:11Z

/test-windows-networkpolicy

GraysonWu

LGTM

pkg/agent/openflow/fields.go

tnqn · 2021-08-16T11:04:21Z

pkg/agent/openflow/fields.go

+// Fields using CT label.
+var (
+	// Field to store the ingress rule ID.
+	IngressRuleCTLabelField = binding.NewCtLabelField(0, 31, "ingressRuleCtLabel")


You didn't name CT marks FromGatewayCTMarkField and relevant functions NewCtMarkField, why adding "Field" suffic to CT labels? I would expect them to be named consistently: NewCTMark, NewCTLabel, FromGatewayCTMark, IngressRuleCTLabel

In my thought, Mark and Field are different concepts. A field only requires a bit range, but no fixed value, and Antrea can use the Field to match/load any value. But for a Mark, it represents a fixed value and the value uses a field. For ctLabel, I didn't think it is used as a mark for fixed value, we only use it as a field for temp value in a connection.

Besides, ct_mark is the field name used in OVS, like ct_label is a field name in OVS. It is weird to use CTMarkMark to represent a special mark using part bits of ct_mark But if you insist, I would change it.

Got it. I misunderstood FromGatewayCTMark, but I see in the latest code you have removed Field from CTLabel struct, which may bring confusion and make it hard to extend CTMark in the future. When reg, ct_mark, ct_label are all fields of OVS, the fields and values are named in different ways:

reg ct_mark ct_label

Field Struct RegField - CTLabel

Value Struct RegMark CTMark -

It would be hard to understand that XXXCTMark is the struct of value while XXXCTLabel is the struct of field. And how we name the field struct of ct_mark in the future when we want to match a value on several bits like reg and ct_label? I think we cannot say ct_mark will only be used as a whole because it would be a waste of the limited connection level storage. In theory FromGatewayCTMark and ServiceCTMark should just use two bits, instead of 32 bits.

I think your original code makes more sense and we can name the struct of ct_mark related field to CTMarkField in the future, just like CTLabelField.

pkg/agent/openflow/regmarks.go

tnqn · 2021-08-16T11:34:15Z

pkg/ovs/openflow/interfaces.go

 	MatchCTMark(value uint32, mask *uint32) FlowBuilder
-	MatchCTLabelRange(high, low uint64, bitRange Range) FlowBuilder
+	MatchCTMarkData(mark *CtMark) FlowBuilder


But haven't you already changed the signatures of many methods? how can the interface's consumer upgrade to new version without changing their code? For example, MatchRegRange expected (regID int, data uint32, rng Range) but now expect (regID int, data uint32, rng *Range)

pkg/ovs/openflow/interfaces.go

1. Use RegMark/RegField in OpenFlow Match or Load actions instead of passing the register ID and range. 2. The usage for OVS registers(including xxreg) should be predefined in regmarks.go. 3. Use reg0[0..3] to indicate the source of packet, and release reg0[4..15] for other usages. Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd · 2021-08-16T15:17:32Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

wenyingd · 2021-08-17T00:55:25Z

/test-ipv6-e2e
/test-ipv6-only-e2e
/test-windows-conformance
/test-windows-networkpolicy
/test-windows-ovs

wenyingd · 2021-08-17T06:44:00Z

/test-ipv6-e2e
/test-ipv6-only-e2e

wenyingd · 2021-08-17T10:23:30Z

/test-ipv6-e2e
/test-ipv6-only-e2e

wenyingd · 2021-08-17T11:59:35Z

/test-ipv6-only-e2e

tnqn

LGTM

wenyingd force-pushed the issue_2433 branch 3 times, most recently from 2d1eb2c to 0767bc3 Compare July 23, 2021 07:44

wenyingd force-pushed the issue_2433 branch 4 times, most recently from 3d49a27 to d53795c Compare July 23, 2021 13:38

wenyingd requested review from jianjuns, antoninbas, Dyanngg and tnqn July 26, 2021 06:55

jianjuns reviewed Jul 27, 2021

View reviewed changes

pkg/agent/controller/networkpolicy/packetin.go Show resolved Hide resolved

pkg/ovs/openflow/interfaces.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/interfaces.go Show resolved Hide resolved

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

jianjuns reviewed Jul 27, 2021

View reviewed changes

wenyingd force-pushed the issue_2433 branch from d53795c to 2b541f0 Compare July 27, 2021 13:54

jianjuns reviewed Jul 27, 2021

View reviewed changes

wenyingd force-pushed the issue_2433 branch 4 times, most recently from 9d2ffa2 to ed0d1ed Compare July 30, 2021 07:55

wenyingd requested a review from jianjuns August 3, 2021 11:57

jianjuns reviewed Aug 3, 2021

View reviewed changes

pkg/ovs/openflow/interfaces.go Outdated Show resolved Hide resolved

wenyingd force-pushed the issue_2433 branch 3 times, most recently from cad46af to 4a289b7 Compare August 5, 2021 09:20

wenyingd force-pushed the issue_2433 branch 2 times, most recently from 30e42d3 to bfc34b2 Compare August 11, 2021 12:11

wenyingd requested a review from tnqn August 11, 2021 12:12

GraysonWu reviewed Aug 11, 2021

View reviewed changes

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

GraysonWu reviewed Aug 11, 2021

View reviewed changes

pkg/agent/openflow/fields.go Show resolved Hide resolved

GraysonWu reviewed Aug 11, 2021

View reviewed changes

wenyingd force-pushed the issue_2433 branch 2 times, most recently from 43030ca to 6bc4a10 Compare August 12, 2021 02:10

wenyingd requested a review from GraysonWu August 12, 2021 11:00

GraysonWu previously approved these changes Aug 12, 2021

View reviewed changes

tnqn reviewed Aug 16, 2021

View reviewed changes

wenyingd dismissed GraysonWu’s stale review via 7e3a339 August 16, 2021 14:05

wenyingd force-pushed the issue_2433 branch 3 times, most recently from 78353a9 to f112fed Compare August 16, 2021 15:00

wenyingd force-pushed the issue_2433 branch from f112fed to b8636eb Compare August 16, 2021 15:14

wenyingd requested a review from tnqn August 16, 2021 15:22

tnqn approved these changes Aug 18, 2021

View reviewed changes

tnqn merged commit 43837c6 into antrea-io:main Aug 18, 2021

wenyingd deleted the issue_2433 branch October 8, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide generic API for OVS register match/set value #2455

Provide generic API for OVS register match/set value #2455

wenyingd commented Jul 23, 2021 •

edited

Loading

codecov-commenter commented Jul 23, 2021 •

edited

Loading

jianjuns left a comment

jianjuns Jul 27, 2021

wenyingd Jul 27, 2021

jianjuns Jul 27, 2021

wenyingd Jul 29, 2021

jianjuns Jul 27, 2021

wenyingd Jul 29, 2021

jianjuns Aug 3, 2021

wenyingd commented Jul 30, 2021

wenyingd commented Aug 2, 2021

wenyingd commented Aug 3, 2021

wenyingd commented Aug 3, 2021

jianjuns left a comment

wenyingd commented Aug 5, 2021

wenyingd commented Aug 11, 2021

GraysonWu left a comment

wenyingd commented Aug 12, 2021

wenyingd commented Aug 12, 2021

GraysonWu left a comment

tnqn Aug 16, 2021 •

edited

Loading

wenyingd Aug 16, 2021

wenyingd Aug 16, 2021

tnqn Aug 16, 2021

tnqn Aug 16, 2021

wenyingd commented Aug 16, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

tnqn left a comment

Provide generic API for OVS register match/set value #2455

Provide generic API for OVS register match/set value #2455

Conversation

wenyingd commented Jul 23, 2021 • edited Loading

codecov-commenter commented Jul 23, 2021 • edited Loading

Codecov Report

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

wenyingd commented Jul 30, 2021

wenyingd commented Aug 2, 2021

wenyingd commented Aug 3, 2021

wenyingd commented Aug 3, 2021

jianjuns left a comment

Choose a reason for hiding this comment

wenyingd commented Aug 5, 2021

wenyingd commented Aug 11, 2021

GraysonWu left a comment

Choose a reason for hiding this comment

wenyingd commented Aug 12, 2021

wenyingd commented Aug 12, 2021

GraysonWu left a comment

Choose a reason for hiding this comment

tnqn Aug 16, 2021 • 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

wenyingd commented Aug 16, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

wenyingd commented Aug 17, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Jul 23, 2021 •

edited

Loading

codecov-commenter commented Jul 23, 2021 •

edited

Loading

tnqn Aug 16, 2021 •

edited

Loading