-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
Address sets for nftables and OVN #1728
base: main
Are you sure you want to change the base?
Conversation
Hi, is there anything I can do to help process the PR? |
Nope, it's next on my list to review, just taking a bit as it's a sizable PR :) |
I spent a couple of hours on cleaning up the branch, mostly rebasing everything on current main and re-slicing the code in our usual commit chunks. I also did a few minor code style tweaks here and there, but nothing major. I did notice a few major changes that I'll want to make to this branch, but I'll need to do that later, maybe tomorrow evening, maybe Thursday. It's mostly around the database, changing the name of the tables to line up with our usual pattern, using the DB generator instead of manual SQL functions and a few other similar tweaks. Anyway, I also need to do an actual review of the logic and manual testing of all of this. |
Don't worry about the test failures, it's expected at this stage :) |
Good to know, looking forward to learn from your modifications ! |
Haven't forgotten about this but have had to deal with some priority bugs. |
Been making some progress on this one, but working locally so haven't pushed anything yet. I've renamed all the ExternalIDs stuff to Config for consistency, renamed the tables to our usual pattern too and put the DB generator in place to generate all the DB access code. I'm now replacing all the DB function calls with the equivalent for the generated code. After that's done, I'll want to make sure we can get the tests behaving again before I actually review the content of the commits. |
Yes, at first glance I used ExternalIDs while reading OVN man pages however after I have seen that ACL for example had the same field under config... Forgot to change to comply but I should have done it. Concerning DB function I want to see how you do that as I don't get exactly what is the 'equivalent'. When I took a look at failed tests last week I saw that we have failure on the PATCH request test. I was able to get it to work on my computer but once I ran it in github action it wouldn't, so as everything else was ok in github actions I commented it out. So we may want to check other tests are behaving before tackling the patch issue. Thanks for the heads up! |
Just pushing an intermediate state. I've moved everything to the DB generator, code builds, static analysis looks clean, so I want to get an idea of what I broke before I clean things up and review the rest. |
HI, I took a look at the error:
I think the issue lies in the GetConfig function in |
Yeah, I know, but this is generated code. You can't change that file as it will get overwritten. I reached out to @masnax about the database generator and what we need to do to make it behave given the name of the tables involved. |
d493e64
to
b850a6f
Compare
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Isidore Reinhardt <pro.irhndt@4fk.fr>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Closes #1450
This pull request add address sets support for incus, more precisely it allows the use of named sets for nftables and address sets in OVN.
This is my first 'real' contribution and first go project. You may want to check important logic parts in nftables and ovn drivers.
Notes: