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 agent API and antctl command to dump OVS groups #1984

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

jianjuns
Copy link
Contributor

antctl get of -G 1,10,2

FLOW
group_id=1,type=select,bucket=bucket_id:0,weight:100,actions=load:0xac640003->NXM_NX_REG3[],load:0x35->NXM_NX_REG4[0..15],load:0x2->NXM_NX_REG4[16..18],load:0x1->NXM_NX_REG0[19],resubmit(,42),bucket=bucket_id:1,weight:100,actions=load:0xac640002->NXM_NX_REG3[],load:0x35->NXM_NX_REG4[0..15],load:0x2->NXM_NX_REG4[16..18],load:0x1->NXM_NX_REG0[19],resubmit(,42)
group_id=2,type=select,bucket=bucket_id:0,weight:100,actions=load:0xac640003->NXM_NX_REG3[],load:0x23c1->NXM_NX_REG4[0..15],load:0x2->NXM_NX_REG4[16..18],load:0x1->NXM_NX_REG0[19],resubmit(,42),bucket=bucket_id:1,weight:100,actions=load:0xac640002->NXM_NX_REG3[],load:0x23c1->NXM_NX_REG4[0..15],load:0x2->NXM_NX_REG4[16..18],load:0x1->NXM_NX_REG0[19],resubmit(,42)

antoninbas
antoninbas previously approved these changes Apr 15, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -87,7 +87,7 @@ func dumpMatchedGroups(aq agentquerier.AgentQuerier, groupIDs []binding.GroupIDT
// number is invalid).
func getTableFlows(aq agentquerier.AgentQuerier, table string) ([]Response, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not introduced by this PR, but do you think you could rename table to tables to match the new getGroups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

@@ -330,6 +334,11 @@ var CommandList = &commandList{
usage: "Comma separated Antrea OVS flow table names or numbers",
shorthand: "T",
},
{
name: "groups",
usage: "Comma separated OVS group IDs. Use 'all' for dumping all groups",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
usage: "Comma separated OVS group IDs. Use 'all' for dumping all groups",
usage: "Comma separated OVS group IDs. Use 'all' to dump all groups",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 67 to 69
// There seems a bug in ovs-ofctl that dump-groups always returns all
// the groups when using Openflow13, even when the group ID is provided.
// As a workaround, we do not specify Openflow13 to run the command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR as well, but this comment looks incorrect. As per the documentation for dump-groups:

Only OpenFlow 1.5 and later support dumping a specific group.
Earlier versions of OpenFlow always dump all groups.

When we omit the OpenFlow version, it will default to OpenFlow10, but also enable the Nicira extensions apparently, which support dumping a single group. When specifying OpenFlow13 explicitly, it will not enable these extensions and as per the manual it does not support dumping a single group. As for specifying OpenFlow15, we do not enable this protocol on the bridge at the moment. So there is no bug in ovs-ofctl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for figuring this out! Updated the comments.
Still think ovs-ofctl should at least provide some hints when a group ID is specified with a OpenFlow version does not support that.

antoninbas
antoninbas previously approved these changes Apr 15, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-all

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Approving again after rebase

@jianjuns jianjuns merged commit 7e1cbf9 into antrea-io:main Apr 19, 2021
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.

3 participants