-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide generic API for OVS register match/set value #2455
Conversation
2d1eb2c
to
0767bc3
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3d49a27
to
d53795c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach. Just a few minor comments.
And in the commit description:
lease reg0[4..15] for other usages.
lease -> release
pkg/ovs/openflow/ofctrl_action.go
Outdated
@@ -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])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is a good idea to save the string for the predefined regs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add "name" as a property in RegField
, but I don't know how to leverage this property..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about saving the LearnField?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get your point about "saving the LearnField". Here LearnField
is a type defined in ofnet.
pkg/ovs/openflow/interfaces.go
Outdated
name string | ||
} | ||
|
||
// RegMark uses a RegField to cache a fixed value or value enumeration. The RegMark is used to indicate the traffic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
9d2ffa2
to
ed0d1ed
Compare
/test-all |
1 similar comment
/test-all |
/test-networkpolicy |
/test-ipv6-networkpolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
cad46af
to
4a289b7
Compare
/test-all |
30e42d3
to
bfc34b2
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just some suggestions on CustomReason comments.
43030ca
to
6bc4a10
Compare
/test-all |
/test-windows-networkpolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pkg/agent/openflow/fields.go
Outdated
// Fields using CT label. | ||
var ( | ||
// Field to store the ingress rule ID. | ||
IngressRuleCTLabelField = binding.NewCtLabelField(0, 31, "ingressRuleCtLabel") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/ovs/openflow/interfaces.go
Outdated
MatchCTMark(value uint32, mask *uint32) FlowBuilder | ||
MatchCTLabelRange(high, low uint64, bitRange Range) FlowBuilder | ||
MatchCTMarkData(mark *CtMark) FlowBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
78353a9
to
f112fed
Compare
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>
/test-all |
/test-ipv6-e2e |
/test-ipv6-e2e |
1 similar comment
/test-ipv6-e2e |
/test-ipv6-only-e2e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: wenyingd wenyingd@vmware.com