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

server/status: read current process's cgroup to determine available memory #43137

Merged

Conversation

ajwerner
Copy link
Contributor

Prior to this commit we would consult the system memory and the root cgroup
to determine the available memory. This is problematic because almost always
when systems use cgroups they use a hierarchy to create limits for specific
sets of processes. After this change we'll now determine the deepest child
hierarchy which contains this process and examine all of the relevant
limits to determine the memory limit.

Release note (bug fix): On linux machines we now respect the available memory
limit as dictated by the cgroup limits which apply to the cockroach process.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@derekwaynecarr
Copy link

to save future headaches, is it possible to support reading both cgroup v1 file or v2 file?

in Kubernetes, we are starting to work toward cgroup v2 enablement so adding support for memory.max in addition to what is checked here would help prevent future revisions.

@ajwerner
Copy link
Contributor Author

@bdarnell I didn't know about #32067 when I typed this. Do we prefer that patch? It adds a dependency on runc just to read the /proc/<pid>/cgroup file though that dependency makes for cleaner code. I think that patch still has a problem with regards to limits in a parent of the hierarchy being smaller than the limit in the child.

Relates to #31750.

@bdarnell
Copy link
Contributor

Do we prefer that patch? It adds a dependency on runc just to read the /proc//cgroup file though that dependency makes for cleaner code

My thinking is that if it's just the small amount of code in this PR, I'd rather parse the file ourselves and not have the dependency. But given the "TODO: support v2" comments, it seems useful to take on a dependency and get (hopefully) more complete and future-proof support for it. (runc does seem like a rather large dependency for this purpose though).

@ajwerner
Copy link
Contributor Author

But given the "TODO: support v2" comments, it seems useful to take on a dependency and get (hopefully) more complete and future-proof support for it.

I buy that. I went fishing around for a better library than the one in runc and didn't come up with too much. https://github.com/containerd/cgroups seems like it could be okay though on first glance the API seems pretty heavy weight. The logic required to support v2 is rather small. It's just detecting that there's a v2 hierarchy when parsing the /proc/<pid>/cgroup file which would be a row with a 0::$PATH line and then reading the memory.max file instead of the memory.limit_in_bytes file. The bigger win from adopting the dependency is that it parses the mount points to find out where the cgroups are mounted (also a TODO).

If I read your comment correctly, I should take a stab at adopting a dependency to achieve the goals and only if I have a bad time should we revisit this more manual approach.

@bdarnell
Copy link
Contributor

Yeah, I'd try with the dependency first.

@ajwerner ajwerner force-pushed the ajwerner/fix-cgroup-memory-reading branch from 4fca024 to 0b6c605 Compare January 7, 2020 20:49
@ajwerner
Copy link
Contributor Author

ajwerner commented Jan 7, 2020

Yeah, I'd try with the dependency first.

I ran in to trouble with the dependencies. I decided to just support v2. I wrote tests and tried it out in a variety of settings on VMs. It seems to work.

@vladdy would you be willing to give this a review?

@ajwerner
Copy link
Contributor Author

ajwerner commented Jan 7, 2020

I suspect the code needs more of a gate on the OS, perhaps by consulting runtime.GOOS at runtime or by using compilation directives. Otherwise it is RFAL.

@ajwerner ajwerner force-pushed the ajwerner/fix-cgroup-memory-reading branch 2 times, most recently from fd295e5 to 07fd8f3 Compare January 7, 2020 21:54
@ajwerner ajwerner requested a review from vladdy January 7, 2020 21:57
Prior to this commit we would consult the system memory and the root cgroup
to determine the available memory. This is problematic because almost always
when systems use cgroups they use a hierarchy to create limits for specific
sets of processes. After this change we'll now determine the deepest child
hierarchy which contains this process and examine all of the relevant
limits to determine the memory limit.

This commit adds a `util/cgroups` package which contains logic to examine
the cgroup hierarchy for the current process.

Release note (bug fix): On linux machines we now respect the available memory
limit as dictated by the cgroup limits which apply to the cockroach process.
@ajwerner ajwerner force-pushed the ajwerner/fix-cgroup-memory-reading branch from 07fd8f3 to a1f18d0 Compare January 7, 2020 22:34
Copy link
Contributor

@vladdy vladdy left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@ajwerner
Copy link
Contributor Author

ajwerner commented Jan 9, 2020

TFTR!

bors r=vladdy

craig bot pushed a commit that referenced this pull request Jan 9, 2020
43137: server/status: read current process's cgroup to determine available memory r=vladdy a=ajwerner

Prior to this commit we would consult the system memory and the root cgroup
to determine the available memory. This is problematic because almost always
when systems use cgroups they use a hierarchy to create limits for specific
sets of processes. After this change we'll now determine the deepest child
hierarchy which contains this process and examine all of the relevant
limits to determine the memory limit.

Release note (bug fix): On linux machines we now respect the available memory
limit as dictated by the cgroup limits which apply to the cockroach process.

Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jan 9, 2020

Build succeeded

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.

None yet

5 participants