-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
ci: revert #4020 #4446
ci: revert #4020 #4446
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please refer to commits (rather than PRs) in commit messages? It is way easier to follow this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this PR also silently reverts part of commit 3083bd4. Why?
fca2de8
to
d756525
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Need to fix EL9 CI first (see #4455). |
When I re-run the failure CI, it is suddenly green now! |
8132024
to
1560b9a
Compare
1560b9a
to
4706091
Compare
@lifubang please ^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs commit IDs not PRs in commit message
This reverts commit 65a1074. We needed [1] because when we removed the bindfd logic in [2] we had not yet moved the binary cloning logic to Go and thus it was necessary to increase the memory limit in CI because the clone was happening after joining the cgroup. However, [3] finally moved that code to Go and thus the cloning is now done outside of the container's cgroup and thus is no longer accounted as part of the container's memory usage at any point. Now we can properly support running a simple container with lower memory usage as we did before. [1]: commit 65a1074 ("increase memory.max in cgroups.bats") [2]: commit b999376 ("nsenter: cloned_binary: remove bindfd logic entirely") [3]: commit 0e9a335 ("nsexec: migrate memfd /proc/self/exe logic to Go code") Signed-off-by: lfbzhm <[email protected]> [cyphar: fixed commit messages] Signed-off-by: Aleksa Sarai <[email protected]>
we need (#4020) because of (#3931), at that time, we removed the bindfd logic, and the
memfd logic will use more memory than before, but we have not yet moved binary clone from
runc init to runc parent process, so we need to increase memory limit in CI.
As we have moved the runc binary clone logic from runc init to runc parent process in
(#3987), so the memory usage of binary clone will not be included in container's memory
cgroup accounting. Now we can support run a simple container with lower memory usage the
same as before.
Signed-off-by: lifubang [email protected]