go-units
go-units copied to clipboard
docker
Should verify that the max value is not too big
When parsing the ulimit, we should check if the ulimit max value is greater then the processes max value. If yes then we should return an error.
Signed-off-by: Daniel J Walsh [email protected]
There's been some discussion around validation in the "go" code versus delegating to the kernel itself; does Linux return an error in this case, or is it silently ignored?
Sure, the reason I wanted to do this hear was to avoid duplicating the data. Perhaps if we add a different function.
@thaJeztah Revamped this work, (Going through some old PR's) Now just added a verify function that will check against kernel. The problem is these values are just handed to different OCIs, and they can give you some useless information back to the user.
This should make it easier to diagnose the error, with a better error message.
@thaJeztah seems this issue was addressed -- the test case now gets the max value from the kernel.
Ah, forgot about this one.
I'm still a bit on the fence if querying what the kernel supports, and validating against that is within scope of this module and/or if it should be more of a concern of the code using this module (so far the module was used to only parse values, and to validate a correct format, but no other assumptions) 🤔.
What are your thoughts on that, @kolyshkin ?
I'm on the verge about it. True, this package is only about parsing, it does not talk to the kernel in any way etc.
OTOH this package knows about ulimit, which is a UNIX concept, and adding Verify method is super easy here, and is not that easy in the other package (since the implementation uses a map which is internal to this package).
Overall I think this should go in, with the only change that Verify should not be specific to Linux -- looks like any unix (including *bsd and darwin) should support it. Perhaps even !windows (need to take a look at whatever x/sys/unix implements).
Another thing is, this raises the bar to how this should be tested -- at least we now need to compile this for various GOOS/GOARCH combinations.
Yes it's a bit of a tough one; I can definitely see the convenience of having the Validate(). Mostly concerned if it would be widening the scope too much (in addition to the dependency graph).