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

Enable VRF in kernel configurations #3966

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

mestery
Copy link

@mestery mestery commented Dec 11, 2023

- 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)

C1691D65-F4EC-4569-BE2D-57CB66DD5B03_1_105_c

@mestery mestery mentioned this pull request Dec 11, 2023
@deitch
Copy link
Collaborator

deitch commented Dec 11, 2023

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?

@mestery
Copy link
Author

mestery commented Dec 11, 2023

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!

@deitch
Copy link
Collaborator

deitch commented Dec 11, 2023

Is there a way to test the built image in the CI?

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 update-component-sha.sh that will do that automatically, if you pass the input. I don't recall offhand how the kernel image tag is updated. All of the other packages use linuxkit pkg show-tag <dir>, but this might be different.

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.

To test VRF itself, add another test case to the ones above.

@barajus
Copy link

barajus commented Dec 19, 2023

Thank you for pushing this @mestery - I was busy and not available to fix my original request

@mestery
Copy link
Author

mestery commented Dec 19, 2023

I hope to get back to adding the CI tests as requested by @deitch this week yet.

@mestery
Copy link
Author

mestery commented Dec 20, 2023

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!

@deitch
Copy link
Collaborator

deitch commented Dec 21, 2023

Does linuxkit have tests for all the kernel options for which it enables?

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 linuxkit/kernel. So I am not sure how we do that.

I didn't write those kernel docs, I believe @rn did, maybe he knows. If you see it in the docs, that can help.

@mestery
Copy link
Author

mestery commented Feb 9, 2024

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.

@deitch
Copy link
Collaborator

deitch commented Feb 10, 2024

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.

@mestery
Copy link
Author

mestery commented Feb 25, 2024

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!

@deitch
Copy link
Collaborator

deitch commented Feb 26, 2024

@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>
@mestery
Copy link
Author

mestery commented Feb 26, 2024

@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.

sigh, it was an issue on my end, I have fixed it and it should be correct now. Thanks for your patience!

@deitch
Copy link
Collaborator

deitch commented Feb 26, 2024

No problem. Build is running.

@deitch
Copy link
Collaborator

deitch commented Feb 26, 2024

Built and pushed. Running CI.

@deitch deitch merged commit 5110210 into linuxkit:master Feb 26, 2024
22 checks passed
@mestery
Copy link
Author

mestery commented Feb 26, 2024

Thanks for the approval @deitch, looks like it passed all tests! 🥳

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.

Please enable the VRF kernel module in linuxkit
3 participants