-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
os: add constrainedmem() #27805
os: add constrainedmem() #27805
Conversation
This exposes libuv's uv_get_constrained_memory(). It is useful when running inside a container.
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.
I expect uv_get_constrained_memory()
will still go through some revisions. It'd be good to mark os.constrainedmem()
as experimental for now.
@@ -151,6 +151,14 @@ static void GetTotalMemory(const FunctionCallbackInfo<Value>& args) { | |||
} | |||
|
|||
|
|||
static void GetConstrainedMemory(const FunctionCallbackInfo<Value>& args) { | |||
double amount = uv_get_constrained_memory(); | |||
if (amount < 0) |
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.
This condition is never true because uv_get_constrained_memory()
returns an unsigned integer. Assigning it to a double
isn't really correct either because its value may be well outside what can fit in 53 bits.
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.
Ah that makes sense. That being said, do we need to also update GetFreeMemory
and GetTotalMemory
? This is implemented almost identically to those.
static void GetFreeMemory(const FunctionCallbackInfo<Value>& args) {
double amount = uv_get_free_memory();
if (amount < 0)
return;
args.GetReturnValue().Set(amount);
}
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.
Ideally yes, but it's less acute for those functions because systems generally don't have > 2**53 bytes of memory.
However, it's common with cgroups to set the memory limit to 2**63 to indicate it's unconstrained and that's what uv_get_constrained_memory()
reports.
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.
ah yea that makes sense. Any recommendations on how best to handle this? Should we expose as a BigInt here?
|
||
* Returns: {integer} | ||
|
||
The `os.constrainedmem()` method returns the an integer that represents the |
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.
Maybe reword "the an integer that represents" as "an integer representing"
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.
I would suggest removing "an intger that represent the", make the text as parallel as possible to the wording of the freemem()
docs. Also, I know this has to do with returning the right max memory that is available to docker containers, because I read the issues, but I don't think anyone could possibly guess this from the hand wavy "constrained" description here. I suggest the docs be much more clear on what it is. Using docker by name isn't a bad thing, and also adding the exact /sys/... property that it is being read and returned on Linux isn't bad, either, it allows people to googl around and find out more information without reading the uv source.
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.
Sure, happy to add. Thanks for the suggestions!
@@ -49,6 +49,18 @@ Returns an object containing commonly used operating system specific constants | |||
for error codes, process signals, and so on. The specific constants currently | |||
defined are described in [OS Constants](#os_os_constants_1). | |||
|
|||
## os.constrainedmem() |
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.
I think it should be os.constrainedMemory()
. It's only a few characters longer, but more readable.
This is also more in line with the libuv method which is uv_get_constrained_memory
, making this a camel-cased version.
I'm aware it's like this for consistency with existing methods, but it's an awful naming convention and I think os.freemem()
and os.totalmem()
should be renamed too.
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.
While I share the sentiment, that is mostly orthogonal to this PR, I think?
Renaming freemem/totalmem and doc-deprecating the old variants should be proposed as a separate PR, imo, and this one should use the current convention if landed before that PR (and that PR should then also include renaming of this method) and the new convention if landed after that PR.
The `os.constrainedmem()` method returns the an integer that represents the | ||
amount of memory available to the process (in bytes) based on limits imposed by | ||
the operating system. If there is no such constraint, or the constraint is | ||
unknown, `0` is returned. |
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.
This should also mention that this value could be less than or greater than the total memory available, like uv docs mention it. Otherwise, it could promote incorrect usage of this method.
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.
Good call. I'll add. Thanks!
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.
Unfortunately, uv_get_constrained_memory()
does not seem to work, it returns 0 when there was a limit set via cgroups.
To clarify — this does not work under unified
cgroup hierarchy, i.e. when /sys/fs/cgroup
has been mounted with type=cgroup2
.
If this works under only legacy
or hybrid
hierarchies (I have not tested those) — I am against merging this until it works with unified
. Those two are deprecated and upstream is migrating off them, we shouldn't introduce brand new functionality that relies on deprecated API and does not work with the current one.
To test:
- Boot the system with
systemd.unified_cgroup_hierarchy=1
option - Run node (from user) with
systemd-run --user --scope --property=MemoryMax=100M ./node
- Observe the return value of
uv_get_constrained_memory()
, which is 0. It shouldn't be zero.
E.g.,Buffer.alloc(100e6).fill(0),0
fails.
Refs:
- https://github.com/systemd/systemd/blob/master/docs/CGROUP_DELEGATION.md#three-different-tree-setups-
- https://fedoraproject.org/wiki/Changes/CGroupsV2
Related: #27508.
Upd: I filed libuv/libuv#2315
This is a more visible change than #27508 doc-wise, so I would prefer to wait for libuv/libuv#2323 first before landing this. Once that will be landed and will get to this repo via deps/uv update, that would resolve my concern about cgroup2 support. |
Status update: Fedora 31 was released in October, has Cgroups v2 enabled by default. |
8ae28ff
to
2935f72
Compare
This unfortunately stalled out and hasn't been updated in a long time. Closing, but we can reopen if it is picked back up again |
Ecosystem status update: Mandriva is also migrating to |
This is now fixed in libuv, so perhaps we can reopen after an libuv upgrade? |
This exposes libuv's uv_get_constrained_memory() that was recently added.
It is useful when running inside a container.
Also, 👋. It's been a while.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes