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

Revert "Set temporary single CPU affinity before cgroup cpuset transition" #4283

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

kolyshkin
Copy link
Contributor

This reverts #3923, for reasons outlined in #3923 (comment). A short version below.


There's too much logic here figuring out which CPUs to use. Runc is a low level tool and is not supposed to be that "smart". What's worse, this logic is executed on every exec, making it slower. Some of the logic in (*setnsProcess).start is executed even if no annotation is set, thus making ALL execs slow.

Also, this should be a property of a process, rather than annotation.

The plan is to rework this.

This reverts commit afc23e3.

@kolyshkin kolyshkin added this to the 1.2.0 milestone May 22, 2024
@kolyshkin
Copy link
Contributor Author

Now, this needs to be reverted before the 1.2 release, as otherwise we'll have to support this.

@lifubang
Copy link
Member

I see the runtime- spec PR(opencontainers/runtime-spec#1253) has been approved, so it maybe merged recently? How about let the revert commit, bump runtime-spec commit and the new implementation commit completed in one PR.
It looks more make sense, but not very important.

@kolyshkin
Copy link
Contributor Author

How about let the revert commit, bump runtime-spec commit and the new implementation commit completed in one PR.

Makes sense (although upper-level runtime change is also needed for the whole thing to work again).

Marking this one a draft for the time being.

@kolyshkin kolyshkin marked this pull request as draft May 24, 2024 17:13
@lifubang
Copy link
Member

lifubang commented Jun 8, 2024

Because #3923 has not been in any release, and the related PR(opencontainers/runtime-spec#1253) has not been merged yet, so we need to merge this revert PR first if we want to draft 1.2.0-rc2.
@kolyshkin @AkihiroSuda WDYT

@AkihiroSuda AkihiroSuda mentioned this pull request Jun 8, 2024
21 tasks
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM

The runtime-spec PR this was implementing was closed (not merged), the new PR is still open. Reverting this seems the right thing to do here.

@kolyshkin do you agree on marking this ready for review and merging?

@kolyshkin kolyshkin marked this pull request as ready for review June 9, 2024 20:58
@kolyshkin
Copy link
Contributor Author

Yes, we need to merge it as we can't have this in a released version (and it blocks some other work). Once opencontainers/runtime-spec#1253 is merged I'll work on implementing it to replace this one.

There's too much logic here figuring out which CPUs to use. Runc is a
low level tool and is not supposed to be that "smart". What's worse,
this logic is executed on every exec, making it slower. Some of the
logic in (*setnsProcess).start is executed even if no annotation is set,
thus making ALL execs slow.

Also, this should be a property of a process, rather than annotation.

The plan is to rework this.

This reverts commit afc23e3.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@lifubang lifubang merged commit 349e5ab into opencontainers:main Jun 9, 2024
39 checks passed
@lifubang
Copy link
Member

lifubang commented Jun 9, 2024

We can prepare release 1.2.0-rc2 now.

@lifubang lifubang mentioned this pull request Jun 10, 2024
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.

4 participants