-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Allow sections of Plugins to be merged, and not overwritten as entire sections. #9982
base: main
Are you sure you want to change the base?
Allow sections of Plugins to be merged, and not overwritten as entire sections. #9982
Conversation
Hi @rayburgemeestre. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Can you add to the test the cases for maps (such as runtimes) and arrays (such as pod_annotations). Those are the areas I think we should have the merging behavior specifically documented and tests to show it is consistent. |
/ok-to-test |
Certainly, I've added a commit, hopefully it comes close to what you had in mind @dmcgowan. If not please let me know 😅 |
Could you fix linter errors? |
f3004cb
to
9aee752
Compare
… sections. See for rationale the Pull Request description. Added unit test to demonstrate the difference of this change. Signed-off-by: Ray Burgemeestre <rayb@nvidia.com>
…ons. Signed-off-by: Ray Burgemeestre <rayb@nvidia.com>
9aee752
to
8dcfd5e
Compare
Hi all, This is a spin-off from the following PR #7347 (which I suggest we close, since I'm afraid the long history there, and non-essential improvements that I've included obfuscate the PR.). My hope is, that this more concise PR is easier to review.
This PR focuses on a very small change targetting 2.0.0, and I've included a unit test to demonstrate the difference it makes. Without the code change, the unit test would fail with: make-test-fail.txt
Summary
The change proposed in this PR allows various drop-in configuration toml files to configure parts of a Plugin.
For example, starting with the following containerd
config.toml
, it includes a bunch of potential drop-in configs:Let's say we have two drop-ins in that directory:
Current behavior
The current code in main/2.0.0 would overwrite in this case the whole CRI plugin with the path from
registry.toml
, and thecni.toml
file would not have any effect. Since lexicographicallyregistry.toml
comes aftercni.toml
. The configuration effectively in containerd would be:Note the entire
[cni]
block missing.A workaround for the drop-in to have the desired effect, we would have to merge / combine both configurations into a single drop-in file, as follows:
Then containerd would indeed have both configuration changes. But the two files had to be merged into one.
Proposed behavior
We think it would be more convenient if every component can just focus on writing their own drop-in configuration for containerd. The change in this PR should fix just that, and the two files
cni.toml
andregistry.toml
actually do result in the following.This example is already a real example, we have certain conditions that could result in either file to be written
cni.toml
and/orregistry.toml
, or none. But we agree it's a trivial one, but it makes a nice example to start with.Motivation
The NVIDIA GPU operator and Kata Containers write their own drop-in configuration file for containerd.
These also cannot co-exist currently in 2.0.0 without this suggested change. We'd have to again merge these toml files together, which gets into a complicated mess. In our case each Kubernetes operator typically configure their hosts with a drop-in configuration file for containerd using an init container.
Conclusion
We're currently at containerd version 1.7.x, but at some point we'll start using 2.0.x, and then it would be really great if we could let go of our patch(es) and just use the upstream containerd from that point on.
Please let me know if this PR is an improvement compared to the old one. And whether or not we can get this merged for 2.0.x.?
In other PRs, such as my old PR, there seems to be agreement on the usefulness of this change, and at the same time some understandably hesitation, but now that there is a 2.0 release on the horizon, it is perhaps 'safer' to make this change?