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

List OVS bridges to allow for OF version >1.0 #1539

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Conversation

pdumais
Copy link
Contributor

@pdumais pdumais commented Aug 20, 2023

There is a problem with the OVS library being used when retrieving a bridge. But since we just need to know if the bridge exists or not, and we dont actually need any details about it, then I changed it to list the bridges.

This now works with bridges that are created with ovs-vsctl set bridge s1 protocols=openflow13

nodes/ovs/ovs.go Outdated
@@ -54,7 +55,14 @@ func (n *ovs) CheckDeploymentConditions(_ context.Context) error {
goOvs.Sudo(),
)

if _, err := c.VSwitch.Get.Bridge(n.Cfg.ShortName); err != nil {
// We were previously doing c.VSwitch.Get.Bridge() but it doesn't work
// when the bridge has a propotocol version higher than 1.0
Copy link
Member

Choose a reason for hiding this comment

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

@pdumais do you have a link to the upstream issue about it? I wonder why this is happening because they use ovsctl in the backend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not see any issue about it. I tried specifying the version in New() but it still wasn't working. It has never worked ( I tried with a few older versions).

I did not troubleshoot their code that much. All I know is that are parsing json output but it yields a parsing error. When trying the OVS command manually it works. So I'm not sure where their library is not handling it corrently.

It has never worked. I tried with clab 0.42 and this issue was present then.

The nice thing about this fix is that it makes it version-agnostic. There is no need to know the bridge version when listing the bridges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hellt hellt changed the title Fix1538 List OVS bridges to allow for OF version >1.0 Aug 22, 2023
Pat Dumais and others added 2 commits August 22, 2023 13:55
# This is the 1st commit message:

Listing bridges

# This is the commit message srl-labs#2:

Listing bridges

# This is the commit message srl-labs#3:

Listing bridges
Co-authored-by: pdumais <github@dumais.io>
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #1539 (90776df) into main (b03c1fc) will decrease coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1539      +/-   ##
==========================================
- Coverage   48.68%   48.63%   -0.06%     
==========================================
  Files         132      132              
  Lines       12665    12672       +7     
==========================================
- Hits         6166     6163       -3     
- Misses       5799     5808       +9     
- Partials      700      701       +1     
Files Changed Coverage Δ
nodes/ovs/ovs.go 1.36% <0.00%> (-0.15%) ⬇️

... and 1 file with indirect coverage changes

@hellt hellt merged commit 6ef4b49 into srl-labs:main Aug 22, 2023
18 of 19 checks passed
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.

None yet

2 participants