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

Jump to bottom

Ovs debug enhancements #2051

Closed
wants to merge 9 commits into from
Closed

Ovs debug enhancements #2051

wants to merge 9 commits into from

Conversation

orgcandman
Copy link
Contributor

@orgcandman orgcandman commented May 7, 2020 •
edited

These represent enhancements to the OvS debug plugin.

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname email@example.com?
  • If this commit closes an existing issue, is the line Closes: #ISSUENUMBER included in an independent line? -- no issues in the issue tracker being resolved
  • If this commit resolves an existing pull request, is the line Resolves: #PRNUMBER included in an independent line?

@pmoravec
Copy link
Contributor

pmoravec commented May 8, 2020

Two generic comments additionally to the code comments:

  1. cant some commits be squashed? The granularity of changes is so low :) (but I am fine with the current approach)

  2. I see various commands removed and others added. Can't the removed ones be important for some other use case? (i.e. ideally some other OVS guy shall review the change).

@bmr-cymru
Copy link
Member

Commits look fine to me (I far prefer things broken out, especially when it comes to bisecting - it's not like we are charged per commit :-) ).

@orgcandman
Copy link
Contributor Author

Two generic comments additionally to the code comments:

1. cant some commits be squashed? The granularity of changes is so low :) (but I am fine with the current approach)

I think the style has been to use commits which are granular. I tried to keep changes grouped so that the most impactful are separate commits that can be reverted.

2. I see various commands removed and others added. Can't the removed ones be important for some other use case? (i.e. ideally some other OVS guy shall review the change).

The stuff that was removed is added using commands that will actually work. For example, the ovs-dpctl commands aren't going to show hardware offload flows, or userspace dp flows. But ovs-appctl dpctl/show system@ovs-system and ovs-appctl dpctl/show netdev@ovs-netdev (which is discovered in dpctl/dump-dps command) will show all of that. None of the removed commands are lost - all are recorded, but with more information. And many of these commands have existed since release 2.6, so it's a many year backward compatibility. I'll ask another OvS contributor to review, though.

kevintraynor
kevintraynor reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@kevintraynor kevintraynor left a comment

Choose a reason for hiding this comment

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

Hi - I ran these changes on a system with ovs-dpdk running and compared to sos master. Few comments on them

sos/report/plugins/openvswitch.py Show resolved Hide resolved
sos/report/plugins/openvswitch.py Show resolved Hide resolved
sos/report/plugins/openvswitch.py Show resolved Hide resolved
sos/report/plugins/openvswitch.py Show resolved Hide resolved
@orgcandman
[openvswitch] only check mempool information for dpdk-init=true
e94a1d3 
Ensures that the get-mempool-info command will be present.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] add support for OpenFlow 1.4 and 1.5
d37cc7c 
The bridge protocol support has been extended for some time,
so ensure that we actually pull this protocol information
if it is configured that way.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] pull additional bridge information
d6fee83 
Pulls additional flow stats, features, forwarding information and mac
layer information.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] capture all datapath data
9bc0633 
This includes flows, ct stats, frag stats, and conntrack information
for all of the datapath types that are active.  Additionally, pull the
tunnel information for datapaths.

Stop using 'ovs-dpctl' in favor of 'ovs-appctl dpctl/*' - for multiple
reasons.  The 'ovs-dpctl' command doesn't support the userspace
datapath.  'ovs-dpctl' cannot pull all attribute types (including
type=offloaded) in newer OVS versions.  Also, the default attribute
type is 'all' so adding type=offloaded is redundant.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] ensure -t 5 for ovs-vsctl where needed
7f4f2a4 
Most instances of ovs-vsctl were run without a previous check.  If
the vswitchd is in a hung state, the command may never timeout.  Ensure
that most places are using the timeout option.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] List important dpdk related directories
70d4ef1 
Sometimes the permissions or selinux attributes are modified in an
unexpected way.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] Add LACP stats
f338dc9 
Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] pull cfm, qos, and bond info
6f12d70 
The connectivity fault mgmt, qos and bond data can all be retrieved
on a per-port basis.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
[openvswitch] poll dpdk status from ifaces and ports
8e651b1 
It's possible that a deployment is using ovs-bonds rather than
dpdk bond ports.  In that case, it's required to pull the iface
information as well.

Signed-off-by: Aaron Conole <aconole@redhat.com>
@orgcandman
Copy link
Contributor Author

I've rebased and pushed after addressing all comments.

pmoravec
pmoravec approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@pmoravec pmoravec left a comment

Choose a reason for hiding this comment

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

Ignore the Travis build failure, some infrastructure issue.

Was checking if the

ovs-vsctl -t 5 get Open_vSwitch . other_config:dpdk-init

command really returns something that starts with

"true"

including the quote marks - per various example outputs it does seem so, i.e. the "suspicious" test in assignment of check_dpdk is correct.

kevintraynor
kevintraynor reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@kevintraynor kevintraynor left a comment

Choose a reason for hiding this comment

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

One additional comment I thought of (should have put in last version)

sos/report/plugins/openvswitch.py Show resolved Hide resolved
@kevintraynor
Copy link
Contributor

ACK for this series

@orgcandman orgcandman requested a review from bmr-cymru May 18, 2020 12:30
bmr-cymru
bmr-cymru approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@bmr-cymru bmr-cymru left a comment

Choose a reason for hiding this comment

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

Ack to the series.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevintraynor kevintraynor kevintraynor left review comments

@bmr-cymru bmr-cymru bmr-cymru approved these changes

@TurboTurtle TurboTurtle TurboTurtle approved these changes

@pmoravec pmoravec pmoravec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@orgcandman @pmoravec @bmr-cymru @kevintraynor @TurboTurtle