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

pre-install-payload: avoid using sed to edit the containerd's configuration.toml #304

Open
wainersm opened this issue Dec 13, 2023 · 5 comments

Comments

@wainersm
Copy link
Member

This is an idea that @stevenhorsman brought on our CI working group meeting today, inspired on recent changes of kata-deploy to adopt the tomlq tool (see kata-containers/kata-containers@dd9f5b0).

In commit 603a555 we introduced a handler for the disable_snapshot_annotations in containerd's configuration.toml (see https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/scripts/reqs-deploy.sh#L188). This handler is fairly complex and error prone because it uses grep/sed commands. A better alternative would be use a specific tool for editing toml like tomlq.

If we adopt tomlq then it will be needed to get it installed in pre-install-payload image (https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/Dockerfile).

Cc @fidencio @mkulke

@wainersm
Copy link
Member Author

An alternative is to re-write https://github.com/confidential-containers/operator/blob/main/install/pre-install-payload/scripts/reqs-deploy.sh in either Go or Rust which should have libs for editing toml files.

@mkulke
Copy link

mkulke commented Dec 14, 2023

My suggestion wouldn't be to rewrite reqs-deploy.sh but have a small, dedicated CLI tool to adjust the the containerd configuration (instead of relying on sed or yq or other tools). The rust project uses toml-edit for the cargo command.

So, specifically the logic in: remove_nydus_snapshotter_from_containerd + configure_nydus_snapshotter_for_containerd. We can write unit-tests for it, have nice error messages, etc.

@fidencio
Copy link
Member

So, specifically the logic in: remove_nydus_snapshotter_from_containerd + configure_nydus_snapshotter_for_containerd. We can write unit-tests for it, have nice error messages, etc.

I'm not against that, but we end up with yet another tomlq / toml editor..
I'd prefer, I think, improving one of the 7201 options that are not working well for our needs.

@mkulke
Copy link

mkulke commented Dec 20, 2023

I'm not against that, but we end up with yet another tomlq / toml editor.. I'd prefer, I think, improving one of the 7201 options that are not working well for our needs.

I don't think that would be case. the tools are largely ok, imo. the problem is how they are used. We could indeed, instead of using compiled code + a library, write a bash function invoking a cli tool comprehensively w/ tests, ci, fixtures, proper error handling, whatnot. In practice this is not a lot of fun to do in bash, so it's not done.

@mkulke
Copy link

mkulke commented Jan 8, 2024

note: there is containerd config dump which dumps the content of the current configuration, including the resolved imports.

I wrote a snippet that would illustrate how a dedicated tool for changing the containerd config could look like #313

if you look at the number of panics() and expects() it becomes obvious how many code paths there are for such a seemingly simple operation

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

No branches or pull requests

3 participants