Skip to content

Remove exec PID files after use to prevent memory leaks#3595

Merged
openshift-merge-robot merged 1 commit into
containers:masterfrom
mheon:fix_exec_leak
Jul 18, 2019
Merged

Remove exec PID files after use to prevent memory leaks#3595
openshift-merge-robot merged 1 commit into
containers:masterfrom
mheon:fix_exec_leak

Conversation

@mheon
Copy link
Copy Markdown
Member

@mheon mheon commented Jul 18, 2019

We have another patch running to do the same for exit files, with a much more in-depth explanation of why it's necessary. Suffice to say that persistent files in tmpfs tied to container CGroups lead to significant memory allocations that last for the lifetime of the file.

I don't think I got all the error cases here, but I'm hesitant to put in a defer, due to the long-running nature of the exec function.

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S labels Jul 18, 2019
We have another patch running to do the same for exit files, with
a much more in-depth explanation of why it's necessary. Suffice
to say that persistent files in tmpfs tied to container CGroups
lead to significant memory allocations that last for the lifetime
of the file.

Based on a patch by Andrea Arcangeli (aarcange@redhat.com).

Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon
Copy link
Copy Markdown
Member Author

mheon commented Jul 18, 2019

Copy link
Copy Markdown
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@haircommander
Copy link
Copy Markdown
Collaborator

/lgtm
/hold
another day, another conmon exec rebase... :)

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jul 18, 2019
@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM assuming happy tests.

@mheon
Copy link
Copy Markdown
Member Author

mheon commented Jul 18, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2019
@openshift-merge-robot openshift-merge-robot merged commit 22e62e8 into containers:master Jul 18, 2019
@rh-atomic-bot rh-atomic-bot mentioned this pull request Jul 18, 2019
7 tasks
@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants