libct/cg/fs/freezer: fix freezing race#2774
Merged
cyphar merged 1 commit intoopencontainers:masterfrom Feb 1, 2021
Merged
Conversation
Contributor
Author
|
I have a number of tests / reproducers written but since this PR does not fix the issue 100% the test case will be flaky, so I am proposing to include this without a test. |
cyphar
reviewed
Feb 1, 2021
cyphar
reviewed
Feb 1, 2021
cyphar
previously approved these changes
Feb 1, 2021
Member
cyphar
left a comment
cyphar
left a comment
There was a problem hiding this comment.
LGTM, with some clarifying comments.
kolyshkin
force-pushed
the
freeze-race
branch
3 times, most recently
from
2ff76db to
2c34040
Compare
Contributor
Author
|
Rebased, updated the comments as per #2774 (comment) and #2774 (comment) |
mrunalp
reviewed
Feb 1, 2021
mrunalp
previously approved these changes
Feb 1, 2021
Before this commit, Set() used GetState() to check the freezer state and retry the operation if the actual state still differs from requested. This should help with the situation when a new process (such as one added by runc exec) is added to the container's cgroup while it's being freezed by the kernel, but it's not working as it should. The problem is, GetState() never returns FREEZING state, looping until the state is either FROZEN or THAWED, so Set() does not have a chance to repeate the freeze attempt. As a result, the container might end up stuck in a FREEZING state, with GetState() never returning (which in turn blocks some other operations). One way to fix this would be to have GetState returning FREEZING state instead of retrying ad infinitum. It would result in changing the public API, and no callers of GetState expects it to return this. To fix, let's not use GetState() from Set(). Instead, read the freezer.state file directly and act accordingly -- return success on FROZEN, retry on FREEZING, and error out on any other (unexpected) value. While at it, further improve the code: - limit the number of retries; - if retries are exceeded, thaw and return an error; - don't retry (or read the state back) on THAW. I played a lot with various reproducers for this bug, including - parallel runc execs and runc pause/resumes - parallel runc execs and runc --systemd-cgroup update (the latter performs freeze/unfreeze); - continuously running /bin/printf inside container in parallel with runc pause/resume; - running pthread bomb (from criu test suite) in parallel with runc pause/resume; and I was not able to make freeze work 100%, meaning sometimes runc pause fails, or runc --systemd-cgroup update produces a warning. With that said, it's still a big improvement over the previous state of affairs where container is stuck in FREEZING state, and GetState() (and all its users) are also stuck. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
freeze-race
branch
from
2c34040 to
76ae1f5
Compare
mrunalp
approved these changes
Feb 1, 2021
This was referenced Mar 1, 2021
This was referenced Apr 14, 2021
This was referenced May 6, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this PR, Set() used GetState() to check the freezer state
and retry the operation if the actual state still differs from requested.
This should help with the situation when a new process (such as one
added by runc exec) is added to the container's cgroup while it's being
freezed by the kernel, but it's not working as it should.
The problem is, GetState() never returns FREEZING state, looping until
the state is either FROZEN or THAWED, so Set() does not have a chance
to repeat the freeze attempt.
As a result, the container might end up stuck in a FREEZING state,
with GetState() never returning (which in turn blocks some other
operations).
One way to fix this would be to have GetState returning FREEZING state
instead of retrying ad infinitum. It would result in changing the public
API, and no callers of GetState expects it to return this.
To fix, let's not use GetState() from Set(). Instead, read the
freezer.state file directly and act accordingly -- return success
on FROZEN, retry on FREEZING, and error out on any other (unexpected)
value.
While at it, further improve the code:
I played a lot with various reproducers for this bug, including
(the latter performs freeze/unfreeze);
in parallel with runc pause/resume;
with runc pause/resume;
and I was not able to make freeze work 100%, meaning sometimes
runc pause fails, or runc --systemd-cgroup update produces a warning.
With that said, it's still a big improvement over the previous
state of affairs where container is stuck in FREEZING state,
and GetState() (and all its users) are also stuck.
For more info, please see #2753
This is a minimal fix that I think is ready and should be included into rc93.
Fixes: #2753