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

Switch OVN/OVS command line calls to libovsdb #255

Closed
stgraber opened this issue Nov 30, 2023 · 15 comments
Closed

Switch OVN/OVS command line calls to libovsdb #255

stgraber opened this issue Nov 30, 2023 · 15 comments
Assignees
Labels
Easy Good for new contributors Feature New feature, not a bug
Milestone

Comments

@stgraber
Copy link
Member

The OVN project maintains a Go package to access the databases, it's available at https://github.com/ovn-org/libovsdb

We should slowly transition our various calls to ovn-nbctl/ovn-sbctl to using libovsdb instead, if only to not be dependent on a specific version of the OVN tooling and to also allow for more efficient and race-free interactions with OVN.

@stgraber stgraber added Feature New feature, not a bug Easy Good for new contributors labels Nov 30, 2023
@stgraber stgraber added this to the incus-0.4 milestone Dec 19, 2023
@stgraber stgraber modified the milestones: incus-0.4, incus-0.5 Dec 20, 2023
@stgraber stgraber removed the Easy Good for new contributors label Jan 4, 2024
@stgraber stgraber self-assigned this Jan 4, 2024
@stgraber stgraber modified the milestones: incus-0.5, incus-0.6 Jan 19, 2024
@stgraber stgraber modified the milestones: incus-0.6, incus-0.7, incus-6.0 Feb 21, 2024
@stgraber stgraber modified the milestones: incus-6.0, soon Mar 26, 2024
@stgraber stgraber added the Easy Good for new contributors label Mar 27, 2024
@stgraber stgraber changed the title Switch OVN command line calls to libovsdb Switch OVN/OVS command line calls to libovsdb Mar 27, 2024
@Abhiram824
Copy link
Contributor

Hello! I am Abhi, a student for UT Austin. As a part of my final project in virtualization, I was interested in looking into this issue. I see that you have assigned this already to yourself, but if you need anyone to help out on it, I would be happy to do so!

@stgraber
Copy link
Member Author

Hey there,

This one is one of those issues that can be worked on by multiple people in parallel so long as we don't work on the exact same function at the same time :)

Basically there are a whole bunch of functions under internal/server/network/ovn/ovn_nb_actions.go that currently call o.nbctl. All of those need to be moved to using o.client which is the Go-native OVN client (libovsdbclient).

The way I've handled this work so far is by picking a function that's currently using o.nbctl, for example let's look at LogicalRouterAdd. Then I always go through the same steps:

  • Rename the function to follow our new naming convention, in this case, it would become CreateLogicalRouter
  • Add a new ctx context.Context argument as the first argument of the function
  • Actually replace the code within the function to use libovsdbclient. In this case, I'd have that function look very similar to CreateLogicalSwitch that I've already ported.
  • Commit that work as something like: incusd/network/ovn/nb: Port CreateLogicalRouter to OVSDB
  • Then update any existing use of the old function to use the new one and commit that as incusd/network/ovn: Update for function changes

Then send that as a pull request and pick the next function to port, rinse repeat until we're done with all of them :)

Feel free to go for the easier ones if a function doesn't seem obvious to map to the structs and actions within the new client logic, just leave them and I can pick them up later.

@DhruvNistala
Copy link
Contributor

Hi, I am also working with Abhiram on all the issues for this project. We implemented the CreateLogicalRouter function based on the implementation of the CreateLogicalSwitch function. I created a Draft PR at #750. Let me know if any additional changes need to be made

Additionally, could I be assigned to all the issues that Abhiram is as well?

@stgraber
Copy link
Member Author

Thanks, I'll take a look shortly!

I've assigned you to this issue. GitHub only lets me assign issues to people who commented on the issue so you're going to need to post a comment on the other issues so I can assign you there too.

@dhruvvnistala
Copy link
Contributor

Thanks, I've left a comment on the other issues accordingly.

@Abhiram824
Copy link
Contributor

Hello, we added some more functions in. CreateLogicalRouterSNAT seemed more involved and required sifting through some documentation to add code in, so please let us know if anything does not look right. We created the draft PR at #809

@Abhiram824
Copy link
Contributor

Abhiram824 commented May 4, 2024

Hey I saw in #812 that you combined deleteLogicalRouter for DNATSNAT and SNAT rules, should I do the same for the CreateLogicalRouter?

@stgraber
Copy link
Member Author

stgraber commented May 5, 2024

Yeah, LogicalRouterDNATSNATAdd can probably be dropped and CreateLogicalRouterSNAT be renamed to CreateLogicalRouterNAT and made to handle both type of rules.

@stgraber
Copy link
Member Author

stgraber commented May 9, 2024

@Abhiram824 @DhruvNistala hello, do you still intend to send more changes for this issue or should I clear the assignee field?

@Abhiram824
Copy link
Contributor

Hi, we were still intending on working on the issue

@stgraber
Copy link
Member Author

Sounds good!

@Abhiram824
Copy link
Contributor

Awesome and btw I am out of town rn but will pick things back up in a week

@Abhiram824
Copy link
Contributor

I'm working on replacing LogicalRouterRouteAdd. If the route is a discard route do I set the NextHop to be "discard" or do I need to store this information in the "options" field.

@stgraber
Copy link
Member Author

According to documentation, that goes into NextHop

@stgraber stgraber modified the milestones: soon, incus-6.3 Jun 5, 2024
@stgraber
Copy link
Member Author

I've been porting some of those functions over the past few days, we're now down to just 32 calls left and I've got a batch of 8 or so in progress, slowly getting there :)

stgraber added a commit to stgraber/incus that referenced this issue Jun 26, 2024
Closes lxc#255

Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
@hallyn hallyn closed this as completed in a9aacbd Jun 26, 2024
stgraber added a commit that referenced this issue Jun 28, 2024
Closes #255

Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Sponsored-by: Luizalabs (https://luizalabs.com)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Good for new contributors Feature New feature, not a bug
Development

No branches or pull requests

4 participants