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

fix openvswitch port assignment #1534

Merged
merged 3 commits into from
Aug 19, 2023
Merged

fix openvswitch port assignment #1534

merged 3 commits into from
Aug 19, 2023

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented Aug 18, 2023

fix #1533

@steiler steiler added the bug Something isn't working label Aug 18, 2023
@steiler steiler self-assigned this Aug 18, 2023
@steiler steiler changed the title include openvswitch in the type check fix openvswitch port assignment Aug 18, 2023
@steiler
Copy link
Collaborator Author

steiler commented Aug 18, 2023

waiting for patrick to double check the build

@pdumais
Copy link
Contributor

pdumais commented Aug 18, 2023

when creating a lab, the ovsp port is created and added in the selected ovs bridge. That's good

if creating a lab that references a bridge that has not manually been created with ovs-vsctl, clab throws an error. But this is not a change in behaviour. ok

when destroying a lab, ovsp ports get deleted but their interface is not removed from the ovs bridge. Not a change in behaviour but it would be nice to fix that some day.

it is impossible to set the bridge to protocols=OpenFlow13 before launching clab. We can set the protocol version after clab
is launched. But this complicates automation scripts. It would be nice to get that fixed. This is not a change in behaviour but it is a bug. Want me to raise a separate ticket for that?

setting a default drop flow and then adding flows to patch likns together works as expected

So other than a few changes I'd like fixed, I'd say that this PR fixes the initial bug that I raised and behaviour is back to normal.

@pdumais
Copy link
Contributor

pdumais commented Aug 18, 2023

using client := ovs.New(goOvs.Sudo(), ovs.FlowFormat("OXM-OpenFlow14"), ovs.Protocols("OpenFlow14"))
instead of c := goOvs.New(goOvs.Sudo(),)
may help.

This can be tested by setting the bridge mode: ovs-vsctl set bridge ovsbr1 protocols=OpenFlow13 before creating the lab

@hellt hellt marked this pull request as ready for review August 19, 2023 21:04
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1534 (5fbdd62) into main (dd86c24) will decrease coverage by 0.18%.
Report is 2 commits behind head on main.
The diff coverage is 7.14%.

❗ Current head 5fbdd62 differs from pull request most recent head 394014e. Consider uploading reports for the commit 394014e to get more accurate results

@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
- Coverage   47.93%   47.75%   -0.18%     
==========================================
  Files         131      131              
  Lines       12622    12620       -2     
==========================================
- Hits         6050     6027      -23     
- Misses       5837     5860      +23     
+ Partials      735      733       -2     
Files Changed Coverage Δ
links/endpoint_bridge.go 41.93% <0.00%> (ø)
links/link.go 59.57% <ø> (+0.16%) ⬆️
nodes/ovs/ovs.go 1.51% <0.00%> (-0.82%) ⬇️
links/link_veth.go 78.84% <100.00%> (-1.69%) ⬇️
nodes/bridge/bridge.go 60.97% <100.00%> (-0.93%) ⬇️

... and 2 files with indirect coverage changes

@hellt hellt merged commit 2f9d291 into srl-labs:main Aug 19, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ovs support seems to have dissapeared in 0.44.0
3 participants