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

config: simplify final CPU affinity rule #1261

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jul 17, 2024

Description of execCPUAffinity.final field (added in #1253) said that if it's not set or empty, the final affinity is the one of container's cgroup. This was done because we thought the kernel changes process' CPU affinity to one of cgroup. It's not the case (see here).

To keep runtimes simple, it makes sense to not do anything if the final affinity is not explicitly set. This change does just that.

Description of execCPUAffinity.final said that if it's not set or empty,
the final affinity is the one of container's cgroup. This was done
because we thought the kernel changes process' CPU affinity to one of
cgroup. It's not the case.

To keep the runtime simple, it makes sense to not do anything if the
final affinity is not explicitly set. This change does just that.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

@kad PTAL

@kad
Copy link
Contributor

kad commented Jul 17, 2024

I'm fine with new wording.

However, in overall, I'd prefer to have that field to be not only for exec, but for all stages of container lifecycle, as we have ideas on how it might be useful in some corner case scenarios even for pid 1 of container... But that's another story :)

@kolyshkin
Copy link
Contributor Author

However, in overall, I'd prefer to have that field to be not only for exec, but for all stages of container lifecycle, as we have ideas on how it might be useful in some corner case scenarios even for pid 1 of container...

We can rename it to "CPUAffinity" and apply to both exec and init. Feel free to open a PR.

kolyshkin added a commit to kolyshkin/runc that referenced this pull request Jul 17, 2024
As per
 - opencontainers/runtime-spec#1253
 - opencontainers/runtime-spec#1261

Add some tests (alas it's impossible to test initial CPU affinity
without adding debug logging).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@tianon tianon merged commit 8f3fbc8 into opencontainers:main Aug 7, 2024
3 checks passed
kolyshkin added a commit to kolyshkin/runc that referenced this pull request Aug 16, 2024
As per
 - opencontainers/runtime-spec#1253
 - opencontainers/runtime-spec#1261

Add some tests (alas it's impossible to test initial CPU affinity
without adding debug logging).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link

@Jpjames1 Jpjames1 left a comment

Choose a reason for hiding this comment

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

Commented

Copy link

@Jpjames1 Jpjames1 left a comment

Choose a reason for hiding this comment

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

¿

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.

6 participants