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

Pass TOML configuration options for runtimes CRI is not aware of #7764

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

mxpv
Copy link
Member

@mxpv mxpv commented Dec 6, 2022

In current implementation CRI offers options field in containerd's configuration file to configure runtimes. It's quite nice as it allows to have one configuration file for both containerd and the underlying runtimes. However this only works for options CRI is aware of.

For all other runtime implementations there is no way to pass configuration (except to specify a config path, which runtime will use to read config file from disk).

This PR adds another option. It extends generic runtime options and adds a bytes blob. containerd can now pass entire TOML section specified for a runtime in that blob, so runtime implementations can unmarshal its configuration directly from that blob instead.

@mxpv mxpv added impact/changelog area/cri Container Runtime Interface (CRI) labels Dec 6, 2022
@mxpv mxpv added this to New in Code Review via automation Dec 6, 2022
@mxpv mxpv moved this from New to Ready For Review in Code Review Dec 6, 2022
@mxpv
Copy link
Member Author

mxpv commented Dec 6, 2022

/test pull-containerd-sandboxed-node-e2e

@mxpv
Copy link
Member Author

mxpv commented Dec 7, 2022

/test pull-containerd-sandboxed-node-e2e

@dcantah dcantah self-requested a review December 7, 2022 03:36
pkg/runtimeoptions/v1/api.proto Outdated Show resolved Hide resolved
runtime/v2/shim/util.go Outdated Show resolved Hide resolved
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@mxpv mxpv changed the title Extend configuration options for runtimes CRI is not aware of Pass TOML configuration options for runtimes CRI is not aware of Dec 7, 2022
@mxpv mxpv requested a review from kzys December 8, 2022 15:50
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM nice improvement

@mxpv mxpv merged commit e1abaeb into containerd:main Dec 8, 2022
Code Review automation moved this from Ready For Review to Done Dec 8, 2022
@mxpv mxpv deleted the config branch December 8, 2022 20:59
dcantah added a commit to dcantah/containerd that referenced this pull request Feb 11, 2023
In containerd#7764 it was made
so that generic runtime options in the containerd toml config file
would get passed to shims regardless of if containerd knew of the
type beforehand and could supply the struct. However, this was only
added for the sandbox server fork here and not the regular ol' CRI
server. This change just mirrors the parts that need to be plopped in
pkg/cri/server

Signed-off-by: Danny Canter <danny@dcantah.dev>
@@ -11,4 +11,7 @@ message Options {
// ConfigPath specifies the filesystem location of the config file
// used by the runtime.
string config_path = 2;
// Blob specifies an in-memory TOML blob passed from containerd's configuration section
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just seeing this change.
I'm wondering why this would be TOML?

This struct already has a type url supplied, why wouldn't the body be the marshaled raw data?

This seems extremely CRI specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by raw data?
We configure runtimes in containerd in TOML format in the first place.
This change is the way to passthru a piece of that "runtime" section directly to the runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the marshaled version, such as proto or json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so per discussion..need an issue to align the type_url to logic specifying that said type defines the bytes in config_body if present...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #8246

jsturtevant pushed a commit to jsturtevant/containerd that referenced this pull request Sep 21, 2023
In containerd#7764 it was made
so that generic runtime options in the containerd toml config file
would get passed to shims regardless of if containerd knew of the
type beforehand and could supply the struct. However, this was only
added for the sandbox server fork here and not the regular ol' CRI
server. This change just mirrors the parts that need to be plopped in
pkg/cri/server

Signed-off-by: Danny Canter <danny@dcantah.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cri Container Runtime Interface (CRI) impact/changelog
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants