[dbode] Call syscall.Setrlimit to set num files open hard limit with setcap for DB docker image #1666
Conversation
src/dbnode/server/limits.go
Outdated
| @@ -78,3 +83,75 @@ func validateProcessLimits() error { | |||
|
|
|||
| return multiErr.FinalError() | |||
| } | |||
|
|
|||
| func raiseRlimitToNROpen() error { | |||
There was a problem hiding this comment.
Thought: this maybe should be in a _linux.go file.
src/dbnode/server/limits.go
Outdated
| @@ -78,3 +83,75 @@ func validateProcessLimits() error { | |||
|
|
|||
| return multiErr.FinalError() | |||
| } | |||
|
|
|||
| func raiseRlimitToNROpen() error { | |||
| cmd := exec.Command("sysctl", "-a") | |||
There was a problem hiding this comment.
Should just be able to check /proc/sys: https://www.kernel.org/doc/Documentation/sysctl/fs.txt
In kube:
/ # hostname
m3db-cluster-rep0-0
/ # cat /proc/sys/fs/nr_open
3000000
There was a problem hiding this comment.
Ah cool, yup I'll change it to this instead.
|
I actually didn't need IPC_LOCK thankfully. |
Codecov Report
@@ Coverage Diff @@
## master #1666 +/- ##
========================================
- Coverage 72% 71.9% -0.2%
========================================
Files 969 969
Lines 81358 81096 -262
========================================
- Hits 58602 58312 -290
- Misses 18913 18963 +50
+ Partials 3843 3821 -22
Continue to review full report at Codecov.
|
robskillington
changed the title
|
Looks good so far, just doing some sanity checking with this and the operator PR before stamping |
There was a problem hiding this comment.
Feels kind of dirty but I understand the pragmatism. Do you think this is worth throwing behind an environment variable?
I'll leave it up to you, but I can imagine a "ATTEMPT_INCREASE_PROCESS_LIMITS" environment variable that you can set to true in the Dockerfile. The primary reason I say that is it seems fair to be doing this in the Dockerfiles we create, but I don't like the idea of the binary doing it indiscriminately regardless of where it is running.
| @@ -26,5 +26,9 @@ COPY --from=builder /go/src/github.com/m3db/m3/bin/m3dbnode /bin/ | |||
| COPY --from=builder /go/src/github.com/m3db/m3/src/dbnode/config/m3dbnode-local-etcd.yml /etc/m3dbnode/m3dbnode.yml | |||
| COPY --from=builder /go/src/github.com/m3db/m3/scripts/m3dbnode_bootstrapped.sh /bin/ | |||
|
|
|||
| # Use setcap and run as specific user | |||
| RUN apk add libcap && \ | |||
There was a problem hiding this comment.
Had not seen this before. Did some reading. Kind of bizarre that the capabilities get set on the file/binary level
There was a problem hiding this comment.
What does the +ep do? Can you just add a comment to this line generally also
There was a problem hiding this comment.
Not opposed to adding a comment, the +e is "effective" and +p is for "permitted".
src/x/os/limits.go
Outdated
| // RaiseProcessNoFileToNROpenResult captures the result of trying to | ||
| // raise the process num files open limit to the nr_open system value. | ||
| type RaiseProcessNoFileToNROpenResult struct { | ||
| RaiseRequired bool |
There was a problem hiding this comment.
Maybe RaisePerformed
There was a problem hiding this comment.
Sure thing.
| RaiseRequired bool | ||
| NROpenValue uint64 | ||
| NoFileMaxValue uint64 | ||
| NoFileCurrValue uint64 |
There was a problem hiding this comment.
Maybe a comment here saying that this will be the curr value before the raise was performed (if it was performed)
There was a problem hiding this comment.
That's not true though, it will be the curr value (after the raise) or if no raise then it will be the curr value (unadjusted).
|
|
||
| // RaiseProcessNoFileToNROpen attempts to raise the process num files | ||
| // open limit to the nr_open system value. | ||
| func RaiseProcessNoFileToNROpen() (RaiseProcessNoFileToNROpenResult, error) { |
There was a problem hiding this comment.
If we're gonna start doing this it might be nice to have a generic AttemptToSetProcessLimits which calls this function. That way we don't have to change server.go each time and we have an established pattern
There was a problem hiding this comment.
This is that kind of method however, it is meant to be abstract and returns enough information (without taking a logger itself). That way callers can either log it as a warning or a hard error, depending on their situation.
|
Sure thing, I'll make it opt in and used only by the docker image. |
… into r/raise-soft-limit-to-hard-limit
What this PR does / why we need it:
This raises the rlimit to whatever nr_open is set to and also sets correct permissions for the docker image to set rlimit.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: