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

Tagging & cleanup of individual OVS settings #158

Merged
merged 18 commits into from
Aug 28, 2020

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Aug 17, 2020

Description

This PR implements the "Common tags for all device types" section of the spec.
Whenever any OVS setting is applied inside the ovsdb, this code adds another setting for the corresponding interface inside the external-ids table, e.g.:

  • ovs-vsctl set-ssl client-key client-cert ca-cert
  • ovs-vsctl set open_vswitch . external-ids:netplan/global/set-ssl=client-key,client-cert,ca-cert
    or:
  • ovs-vsctl set Bridge ovs0 other-config:some-key=42
  • ovs-vsctl set Bridge ovs0 external-ids:netplan/other-config/some-key=42
    or:
  • ovs-vsctl set Port ovs1 lacp=off
  • ovs-vsctl set Port ovs1 external-ids:netplan/lacp=off

The netplan settings tag (netplan/<column>[/<key>]=<value>) marks the settings (column[:key]=value) inside ovsdb state, so that it can be cleaned up via netplan apply --only-ovs-cleanup. The cleanup will only happen if the setting and value are exactly the same as set/tagged by netplan. If the setting was touched manually (or by another process) netplan cleanup will not touch it. This way we can make sure to always start with a clean state before executing the OVS setup commands and submitting the new config/settings to ovsdb.

Additionally, this PR fixes the creation and tagging of OVS patch_ports and the setup of global OpenFlow protocols.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Closes an open bug in Launchpad.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2020

Codecov Report

Merging #158 into openvswitch-support will decrease coverage by 1.83%.
The diff coverage is 44.73%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           openvswitch-support     #158      +/-   ##
=======================================================
- Coverage                94.96%   93.12%   -1.84%     
=======================================================
  Files                        9        9              
  Lines                     2561     2618      +57     
=======================================================
+ Hits                      2432     2438       +6     
- Misses                     129      180      +51     
Impacted Files Coverage Δ
src/openvswitch.c 39.30% <43.24%> (-9.84%) ⬇️
src/parse.c 97.64% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e71a77...1e8d51b. Read the comment docs.

@slyon slyon marked this pull request as ready for review August 26, 2020 12:53
@slyon slyon requested a review from sil2100 August 26, 2020 12:53
Copy link
Collaborator

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

Ok, this was yet another big and tiring review, but thank you for preparing this changeset! I had a few minor inline comments but other than that it's all good to go (but none of those are blocking issues).

In overall I'm a bit annoyed that we even have to do such things like the settings tagging, because this introduces extra complexity. You did a really good job in dealing with it, but it's still so sad that we have to go this extra mile to get things cleaned up and tracked properly. Not much we can do about it though.

Let's get this merged soon!

src/openvswitch.c Outdated Show resolved Hide resolved
src/openvswitch.c Show resolved Hide resolved
src/openvswitch.c Outdated Show resolved Hide resolved
@slyon
Copy link
Collaborator Author

slyon commented Aug 28, 2020

Thanks a lot Lukasz! I know this got pretty big, unfortunately... All of this OVS handling has so many special cases, which need to be handled individually. Yeah.. I'm not sure this is the best way to go long term, as almost every 2nd OVS setting is special in any way and needs individual tagging and cleanup. But I cannot think of any other solution at the moment.

So I fixed all you inline comments and re-run all unit- and integration-tests. This brought up one little issue (in the non-ovs integration tests), with netplan apply failing if OVS was not installed. Fixed that as well.

I will now merge this PR.

@slyon slyon merged commit 5e92509 into openvswitch-support Aug 28, 2020
@slyon slyon deleted the slyon/ovs-tag-settings branch February 25, 2021 11:02
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