Open phyrog opened 12 months ago
We need to find a different way to update the containerd config, imports
does not merge key by key, but instead overwrites entire sections. See https://github.com/containerd/containerd/issues/5837#issuecomment-894840240 for more details on this.
Here's what NVIDIA is doing: https://github.com/NVIDIA/nvidia-container-toolkit/blob/807c87e057e13fbd559369b8fd722cc7a6f4e5bb/pkg/config/engine/containerd/config_v2.go#L27
@phyrog regarding the update of the containerd config.toml. The following idea comes to my mind:
Create a new TOML file that contains all the data found on a set of shim configuration files and then import only this merged file inside the containerd config.toml
. This gives flexibility about which runtimes classes we want to import onto the config.toml
and the advantage of selecting runtime configuration on demand
Deletions or updates could be done in the same way by generating the config file with updates keys (for update) or just by not selecting certain configs (deletion).
@crabarca Importing overwrites the whole [plugins."io.containerd.grpc.v1.cri"]
section, so we'd need to get these parts (e.g. the default runc
config) into that new config file as well before we import it. But yeah, I agree. This is probably the way to go.
config.toml
, get the imports
and plugins
fieldsplugins
config with shim configs and write to new fileimports
field, if necessary
3.1. Write updated config.toml
file, if necessaryOne caveat here is that if a user manually updates something in the plugins section of config.toml
, this will be overridden by our config, which can result in unexpected behavior.
Unfortunately neither https://github.com/BurntSushi/toml nor https://github.com/pelletier/go-toml support preserving comments when unmarshalling/marshalling, so if we modified config.toml
like this to add imports
, it would not preserve any comments, order, etc.
Edit: I think https://github.com/pelletier/go-toml in version 1 might be able to do it using the Tree
api, like NVIDIA does.
I went down the rabbit hole on both libraries to see if there were some detailed AST
structure that we could leverage to persist comments when doing a merge. The problem is that usually the TOML parsers store the file as a map of key=values
without an specific order and doesn't considers comments. Even if comments were parsed the following problem appears:
If we think more deeply about this problem, the key=values
allow a merge scenario out-of-the-box where conflicts can be resolved. This doesn't apply to comments as comments are not identifiable by a key or specific metadata. So merging it's hard as you don't know where to put the comments in the file in order for them to keep being relevant
To conclude, I would not go after trying to merge comments and just creating an intermediary config file which contains the merge of the shim configurations and won't contain comments, which then is going to be imported into config.toml
@crabarca I was mainly thinking about the libraries because we need to potentially modify config.toml
to add or update the imports
field to include our custom config file. I think we should find a way to update the file with the minimal possible modifications (i.e. preserving comments). In the worst case (we don't find a way to modify it with minimal changes), we could opt for creating a backup of config.toml
and then just not care about minimal changes.
~But I do think the Tree
API of pelletier/go-toml v1 that I linked to above parses and stores comments as well~ Edit: comments are just skipped
For the custom config file, I agree, preserving comments is not relevant.
Will close #39. For now used for tracking purposes.