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

Limits returns 64-bit max for VSS on all platforms #379

Open
SuperQ opened this issue Apr 19, 2021 · 9 comments
Open

Limits returns 64-bit max for VSS on all platforms #379

SuperQ opened this issue Apr 19, 2021 · 9 comments

Comments

@SuperQ
Copy link
Member

SuperQ commented Apr 19, 2021

In #340 we fixed the Limits value for "unlimited" to be max uint64. But this value would be incorrect for 32-bit systems, and could depend on kernel/user VM split configuration.

Now I'm also wondering if it's actually valid that 64-bits is the right value for VSS max for Intel 64bit, given there is still probably some kernel reserved address space in 64-bit mode.

@SuperQ
Copy link
Member Author

SuperQ commented Apr 22, 2021

I've been digging into this a bit, and there's a bunch of issues with the current values for process_virtual_memory_max_bytes.

When we detect "unlimited" we output uint64 max. This is very wrong for 32-bit systems, and partly wrong even for 64-bit systems.

For example, it depends on kernel compile flags for 32-bit systems (CONFIG_VMSPLIT_3G etc). On 64-bit, it's architecture dependent, and kernel version dependent.

I'm considering reverting the patch, or changing it to return nil in the case of "unlimited".

I'm also wondering what to do in the unlimited case in client_golang. When ulimit is unlimited I have found no way to determine what the value should be.

CC: @beorn7 @discordianfish

@beorn7
Copy link
Member

beorn7 commented Apr 22, 2021

How about +Inf in that case? It looks we have no way of determining the true value. We cannot use the string "unlimited" as the value of the metric, but +Inf seems to match it quite well.

@SuperQ
Copy link
Member Author

SuperQ commented Apr 23, 2021

Yea, +Inf is also an option. The down side is it's also not exactly true, since the real value is there.

Perhaps we should not expose the metric in client_golang if the value is +Inf.

@SuperQ
Copy link
Member Author

SuperQ commented Apr 23, 2021

The function currently returns a uint64, so there's no +Inf option.

@beorn7
Copy link
Member

beorn7 commented Apr 26, 2021

How many fields in the ProcLimits could have that "unlimited" string in their source field? If that's common, perhaps change the value from uint64 to a struct {uint64, bool} where the bool says if it is set to unlimited?

We could document in the help string of the proc metrics that a Prometheus floating point value of +Inf means that the limit is set to unlimited.

@beorn7
Copy link
Member

beorn7 commented Apr 26, 2021

Disclaimer: I might totally misunderstand the context here. (But just not exposing a metric sounds weird.)

@SuperQ
Copy link
Member Author

SuperQ commented Apr 27, 2021

The problem is that "unlimited" doesn't actually mean the value in bytes is "+Inf". There are hard limits imposed by the platform and kernel, and configuration combinations. This is mostly not an issue on 64-bit platforms, but can be very limiting on 32-bit.

So, exposing the value as +Inf is not true.

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2021

I got that. That's why I said it has to be documented.

However, if I understand you correctly, it's highly non-trivial to find out what the true value is if what we get is unlimited.

Not exposing the metric means the metric doesn't exist. It does not communicate "The value is reported as unlimited by the kernel. That doesn't mean it is actually unlimited. It means there is a limit which we cannot easily determine."

However, we could document that a value of +Inf means the following: "The value is reported as unlimited by the kernel. That doesn't mean it is actually unlimited. It means there is a limit which we cannot easily determine."

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2021

Of course, if there is a way to find out the true value, then let's just use it.

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

No branches or pull requests

2 participants