-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enable VRF in kernel configurations #3966
Conversation
I kicked off CI, but I don't think that actually will test anything. It isn't like the other packages, where you can just run a script and it updates dependencies. The docs are here; can you see what needs to be done to update the various packages so tests catch it all, and it gets published? |
Thanks for the help here! Is there a way to test the built image in the CI? Since this is modifying the kernel config, the only way to test it is to run the resulting kernel image and verify that the VRF configuration is in the kernel which was built. I can try to that locally following those instructions and verify it, but I wasn't clear if you wanted a way to test it in CI as well. Well let you know once I've verified it locally! |
Yes. The test/cases/ dir contains all of the test cases. Each of them gets run for the specific test. All you need for regression tests is to update any tests that use it with the correct image tag. There is a script
To test VRF itself, add another test case to the ones above. |
Thank you for pushing this @mestery - I was busy and not available to fix my original request |
I hope to get back to adding the CI tests as requested by @deitch this week yet. |
Hi @deitch, I am a bit confused now as to what you want me to test here. This PR is simply enabling the VRF code in the kernel. Does linuxkit have tests for all the kernel options for which it enables? I can confirm that running a container engine on a kernel with this enabled does in fact allow me to create VRFs. I need more guidance on why a test is needed for this, and also how I would do that. Thanks! |
No, it doesn't. I cannot image how we would. If this is niche enough that we do not need a special test for it, we can let it go. We still need to update the tests to reference the new kernel, so it is testing the right thing. And we would need to publish the kernel first. That is in the docs I referenced. Although, looking in the tests here, I see that they reference the semver of the kernel, e.g. linuxkit/kernel:5.15.27. Obviously changing a config but keeping the same kernel version shouldn't bump the tag on I didn't write those kernel docs, I believe @rn did, maybe he knows. If you see it in the docs, that can help. |
Hey, just getting back to this now @deitch. So as it stands now, this is a very niche change. Is there a chance that we could get get @rn to help figure out if we can merge this and what the next steps are? I don't think we need to bump the version. I'd love to get this in, as there are a lot of benefits to enabling VRFs in containers for network testing. |
Hi @mestery I did some major rework of the kernel directory in the last few weeks, fixed the makefile up, and even added 6.6.13. I haven't gotten around to making the tests pick up the local builds so CI will test even local changes, but imagine it isn't too far off. I saw you put the changes in all kernels (all 5.x kernels). Do you plan on having it with those? Or just 6.x going forward? We are trying to avoid updates to earlier kernels, if we can. I also significantly cleaned up the docs on kernel builds, so it should be easier to understand. Short form: if 6.x is enough, then make the change just there, and try to follow the build process for that. If it builds, comment here, and I can push out and run CI. |
100dc97
to
5a611ef
Compare
Hi @deitch, thanks for the detailed information! I moved the config change to the latest kernel, removed it from the older one, ran the build (which worked!), and force pushed the branch. Appreciate the help, hopefully you can push it out and run CI and we can merge this now. Thanks again! |
@mestery unless there is a need in the 5.x series, can you remove it and add it solely to the 6.6.x series? The ones prior to 5.15.x are now deprecated here (still available, but not doing additional work), and 5.15.x can be a pain to build. |
Signed-off-by: Kyle Mestery <mestery@mestery.com>
5a611ef
to
bdc15ae
Compare
sigh, it was an issue on my end, I have fixed it and it should be correct now. Thanks for your patience! |
No problem. Build is running. |
Built and pushed. Running CI. |
Thanks for the approval @deitch, looks like it passed all tests! 🥳 |
- What I did
Enabled VRF configuration in linuxkit kernels.
- How I did it
Similar to #3777, I simply enabled this configuration.
Closes #3965
- How to verify it
Boot a container on a kernel with VRF enabled, and create a VRF.
- Description for the changelog
Enables CONFIG_NET_VRF in the linuxkit kernel, which allows for the creation of VRFs in containers.
- A picture of a cute animal (not mandatory but encouraged)