-
Notifications
You must be signed in to change notification settings - Fork 347
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
Use libovsdb bindings for collecting ovs metrics #4637
base: master
Are you sure you want to change the base?
Conversation
5508c97
to
288bf29
Compare
@@ -0,0 +1,17 @@ | |||
package ops |
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.
So I would probably setup a different package for ovn and ovs ops: ops/ovn
and ops/ovs
Also no need to qualify the methods themselves, if needed we can rely on package qualification: ops.ListOVSBridges
vs ovsops.ListBridges
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 have updated the code moving the ovs related ops under libovsdb/ops and also fixed the naming.
I can work on moving ovn related ops to ops/ovn as a different PR.
) | ||
|
||
// Get OpenvSwitch table from the cache | ||
func GetOVSOpenvSwitchTable(ovsClient libovsdbclient.Client) (*vswitchd.OpenvSwitch, error) { |
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.
nit: I would probably leave this at GetOpenVSwitch
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 have updated the naming
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 how about GetOpenvSwitch
? This not getting the OpenvSwitch table, just the single row that happens to be in that table. Maybe you and I are thinking in something different with Table
. If so, do let me know.
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.
Fixed. Thanks for catching this misnomer, I started off with this getting the full table and changed later to single row.
client.WithTable(&vswitchd.OpenvSwitch{}), | ||
client.WithTable(&vswitchd.Bridge{}), | ||
client.WithTable(&vswitchd.Port{}), | ||
client.WithTable(&vswitchd.Interface{}), |
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 guess that metrics is not the best amortization of the extra memory consumption this brings. But I also guess as we further migrate things to libovsdb it would make more sense. What are the plans for further pieces? This is marked as WIP, do you plan to merge this before the other pieces come? Trying to get a sense of a timeline.
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.
Yes, I am planning to start the migration to libovsdb with metrics first.
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.
Would it be possible to have and merge all at once? Each bit could be a different commit here, and I would do a staggered review of each without waiting for the full set. The only problem would be rebases, but we don't expect a lot of changes on the OVS calls that we make.
It is more convenient for our downstream but I understand it might be too much to ask.
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.
It will be difficult to address all at once. I have added another commit to this though, to address those OVN metrics using vsctl.
288bf29
to
5c89cbe
Compare
0d4cebf
to
e5a8ef0
Compare
e5a8ef0
to
36749bd
Compare
Known flake: #4474 |
2d596e2
to
07a8be1
Compare
07a8be1
to
2b8e445
Compare
Second commit LGTM |
Signed-off-by: nithyar <[email protected]>
Signed-off-by: nithyar <[email protected]>
2b8e445
to
9e0e301
Compare
What this PR does and why is it needed
This PR creates an ovsdb client for a node's vswitch database at startup and monitors selected tables. OVS metrics is collected periodically from these tables instead of executing ovs-vsctl commands, for performance benefits.
Which issue(s) this PR fixes
Fixes #
Special notes for reviewers
How to verify it
Details to documentation updates
Description for the changelog
Does this PR introduce a user-facing change?