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

Add execCPUAffinity to the runtime process spec #174

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

keisku
Copy link
Contributor

@keisku keisku commented Jun 28, 2024

Solves #168

Used #136 as a reference to add new field to the spec.

@keisku keisku force-pushed the issue168 branch 2 times, most recently from a5b7a17 to 9412a02 Compare June 28, 2024 04:36
Comment on lines +567 to +588
/// ExecCPUAffinity specifies CPU affinity used to execute the process.
/// This setting is not applicable to the container's init process.
pub struct ExecCPUAffinity {
#[serde(default, skip_serializing_if = "Option::is_none")]
/// cpu_affinity_initial is a list of CPUs a runtime parent process to be run on
/// initially, before the transition to container's cgroup.
/// This is a a comma-separated list, with dashes to represent ranges.
/// For example, `0-3,7` represents CPUs 0,1,2,3, and 7.
cpu_affinity_initial: Option<String>,

#[serde(default, skip_serializing_if = "Option::is_none")]
/// cpu_affinity_final is a list of CPUs the process will be run on after the transition
/// to container's cgroup. The format is the same as for `initial`. If omitted or empty,
/// the container's default CPU affinity, as defined by cpu.cpus property, is used.
cpu_affinity_final: Option<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally fields in execCPUAffinity are initial and final.

https://github.com/opencontainers/runtime-spec/blob/701738418b9555d5213337a0991fd0ffd6c37808/config.md?plain=1#L343-L353

Since final is a reserved keyword in Rust, I added cpu_affinity_ as prefix.

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@saschagrunert
Copy link
Member

PTAL @utam0k

@saschagrunert
Copy link
Member

@keisku do you mind a rebase?

@keisku keisku force-pushed the issue168 branch 2 times, most recently from 58232ff to 6e72c7d Compare July 1, 2024 14:13
@keisku
Copy link
Contributor Author

keisku commented Jul 1, 2024

After rebasing it, I think we should add validation to ExecCPUAffinity and unit tests. Let me add it!

@saschagrunert
Copy link
Member

@utam0k PTAL

@utam0k
Copy link
Member

utam0k commented Jul 2, 2024

Sorry for the late

@utam0k utam0k merged commit a3d8805 into containers:main Jul 2, 2024
6 checks passed
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.

3 participants