os: add constrainedmem() #27805

evanlucas · 2019-05-21T22:55:15Z

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This exposes libuv's uv_get_constrained_memory(). It is useful when running inside a container.

nodejs-github-bot · 2019-05-21T22:55:17Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3590

bnoordhuis

I expect uv_get_constrained_memory() will still go through some revisions. It'd be good to mark os.constrainedmem() as experimental for now.

bnoordhuis · 2019-05-21T23:10:50Z

src/node_os.cc

@@ -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)


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.

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); }

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.

ah yea that makes sense. Any recommendations on how best to handle this? Should we expose as a BigInt here?

cjihrig · 2019-05-22T03:36:43Z

doc/api/os.md

+
+* Returns: {integer}
+
+The `os.constrainedmem()` method returns the an integer that represents the


Maybe reword "the an integer that represents" as "an integer representing"

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.

Sure, happy to add. Thanks for the suggestions!

sindresorhus · 2019-05-22T05:11:24Z

doc/api/os.md

@@ -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()


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.

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.

ChALkeR · 2019-05-22T23:35:08Z

doc/api/os.md

+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.


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.

Good call. I'll add. Thanks!

ChALkeR

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:

Related: #27508.

Upd: I filed libuv/libuv#2315

ChALkeR · 2019-06-12T09:04:49Z

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.

ChALkeR · 2020-01-13T02:27:05Z

Status update: Fedora 31 was released in October, has Cgroups v2 enabled by default.
And they removed Docker because of the lack of Cgroups v2 support.

jasnell · 2020-06-25T14:18:23Z

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

ChALkeR · 2020-07-27T04:55:10Z

Ecosystem status update: Mandriva is also migrating to unified hierarchy: OpenMandrivaAssociation/systemd@86cb4a2
Upd: seems to be temporary reverted because of Docker.

ChALkeR · 2022-09-15T12:30:18Z

This is now fixed in libuv, so perhaps we can reopen after an libuv upgrade?

os: add constrainedmem()

5adefe3

This exposes libuv's uv_get_constrained_memory(). It is useful when running inside a container.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem. labels May 21, 2019

bnoordhuis reviewed May 21, 2019

View reviewed changes

cjihrig reviewed May 22, 2019

View reviewed changes

sindresorhus reviewed May 22, 2019

View reviewed changes

ChALkeR mentioned this pull request May 22, 2019

src: use cgroups to get memory limits #27508

Closed

3 tasks

ChALkeR reviewed May 22, 2019

View reviewed changes

ChALkeR suggested changes May 28, 2019

View reviewed changes

ChALkeR mentioned this pull request May 28, 2019

uv_get_constrained_memory does not seem to work? (checked under cgroup2) libuv/libuv#2315

Closed

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label May 28, 2019

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR mentioned this pull request Jan 12, 2020

linux: support hierarchies and cgroup2 for mem constraint lookup libuv/libuv#2323

Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

jasnell closed this Jun 25, 2020

mjameswh mentioned this pull request Jul 29, 2022

fix(worker) use total_available_size to calculate maxCachedWorkflows temporalio/sdk-typescript#765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: add constrainedmem() #27805

os: add constrainedmem() #27805

evanlucas commented May 21, 2019

nodejs-github-bot commented May 21, 2019

bnoordhuis left a comment

bnoordhuis May 21, 2019

evanlucas May 21, 2019

bnoordhuis May 22, 2019

evanlucas May 28, 2019

cjihrig May 22, 2019

sam-github May 22, 2019

evanlucas May 28, 2019

sindresorhus May 22, 2019

ChALkeR May 22, 2019 •

edited

Loading

ChALkeR May 22, 2019 •

edited

Loading

evanlucas May 28, 2019

ChALkeR left a comment •

edited

Loading

ChALkeR commented Jun 12, 2019 •

edited

Loading

ChALkeR commented Jan 13, 2020 •

edited

Loading

jasnell commented Jun 25, 2020

ChALkeR commented Jul 27, 2020 •

edited

Loading

ChALkeR commented Sep 15, 2022


		* Returns: {integer}

		The `os.constrainedmem()` method returns the an integer that represents the

os: add constrainedmem() #27805

os: add constrainedmem() #27805

Conversation

evanlucas commented May 21, 2019

Checklist

nodejs-github-bot commented May 21, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR May 22, 2019 • edited Loading

Choose a reason for hiding this comment

ChALkeR May 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR left a comment • edited Loading

Choose a reason for hiding this comment

ChALkeR commented Jun 12, 2019 • edited Loading

ChALkeR commented Jan 13, 2020 • edited Loading

jasnell commented Jun 25, 2020

ChALkeR commented Jul 27, 2020 • edited Loading

ChALkeR commented Sep 15, 2022

ChALkeR May 22, 2019 •

edited

Loading

ChALkeR May 22, 2019 •

edited

Loading

ChALkeR left a comment •

edited

Loading

ChALkeR commented Jun 12, 2019 •

edited

Loading

ChALkeR commented Jan 13, 2020 •

edited

Loading

ChALkeR commented Jul 27, 2020 •

edited

Loading