Skip to content
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

Add support to set selection_method in GroupMod #43

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

wenyingd
Copy link

@wenyingd wenyingd commented Jun 29, 2023

Add support for setting selection_method, selection_method_param and fields in a GroupMod message, the supported selection_method including "hash" and "dp_hash". When using "dp_hash", fields is supposed to be empty. "fields" could use a mask value or not, all bits in the value should be 1 if no mask is specified.

Fix: #42

@wenyingd
Copy link
Author

I am a bit confused if we should bump up the version to 0.12.0 rather than 0.11.1 for this change, any idea @tnqn @antoninbas ?

// padding with 4 bytes.
SelectionMethod [16]byte
SelectionParam uint64
// Note, a valid field is supposed to configure with "HasMask==false", and its Value is to apply
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean "HasMask==false"? We can't use masked field here?

Copy link
Author

@wenyingd wenyingd Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means when we construct a MatchField object, the "HasMask" property must be set as false. So we only set "MatchField.Value", but not set "MatchField.Mask". The specified value is actually used to mask on the the given field in a packet by OVS.

@antoninbas
Copy link

I am a bit confused if we should bump up the version to 0.12.0 rather than 0.11.1 for this change, any idea @tnqn @antoninbas ?

Maybe 0.12.0 since you are adding a new public API?

@wenyingd
Copy link
Author

wenyingd commented Jul 3, 2023

Maybe 0.12.0 since you are adding a new public API?

Got it, updated.

@wenyingd
Copy link
Author

wenyingd commented Jul 3, 2023

@tnqn Could you help review this change?

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will update here once I verify the patch with my PR.

SelectionMethod [16]byte
SelectionParam uint64
// Note, a valid field is supposed to configure with MatchField.HasMask=false, and OVS will apply its
// Value to the packet field as a mask.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the comment copied from the original C code?

Copy link
Author

@wenyingd wenyingd Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirement is from OVS code, but the sentence is not directly copied from OVS code, "MatchField", "HasMask" are struct or property defined in libOpenflow

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding single quote to surround them?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Add support for setting selection_method, selection_method_param and fields in a
GroupMod message, the supported selection_method including "hash" and "dp_hash".
When using "dp_hash", fields is supposed to be empty. "fields" could use a mask
value or not, all bits in the value should be 1 if no mask is specified.

Signed-off-by: wenyingd <[email protected]>
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wenyingd wenyingd merged commit f52e481 into antrea-io:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GroupMod message should support selection_method and fields
4 participants