Repository navigation
ocispec: raise actor RLIMIT_NOFILE from 1024 to 1048576 - #2242
Benjamin Elder (BenTheElder) merged 1 commit into
Conversation
| const hostname = "actor" | ||
|
|
||
| // noFileLimit is the RLIMIT_NOFILE soft and hard limit for actor containers. | ||
| // It matches what containerd and runsc give a process on a GKE node. |
There was a problem hiding this comment.
Let's not leave suggestions in the code that we're just doing whatever GKE does and not considering other clusters ... and let's make sure that we have considered ecosystem. What does crun do? Does the OCI spec specify this?
There was a problem hiding this comment.
Removed, thanks!
Did some investigation with AI:
- The OCI spec leaves process.rlimits optional and defines no default. Its sample config, runc spec, and crun spec all template 1024/1024, which is where Substrate's original pin came from.
- At runtime runc and crun apply only what the spec lists and otherwise inherit their own limit.
- Suggestions: keep pinning a value rather than inherit, because runsc refuses a restore whose rlimits differ from the checkpoint, as a result, snapshots restore only on nodes with the same host limit.
ebfd3f9 to
21dc4bf
Compare
| const hostname = "actor" | ||
|
|
||
| // noFileLimit is the RLIMIT_NOFILE soft and hard limit for actor containers. | ||
| const noFileLimit = 1048576 |
There was a problem hiding this comment.
why move this out to a top level const? just document it inline?
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
generally OK with this, I don't think we should copy out the literal constant to a top level constant though, it's just harder to read and not being used anywhere else
Benjamin Elder (BenTheElder)
added
kind/feature
61b0c01 to
6e30794
Compare
|
|
||
| // Build returns a runtime-neutral OCI spec for an actor container. | ||
| func Build(o Options) *specs.Spec { | ||
| // noFileLimit is the RLIMIT_NOFILE soft and hard limit: the kernel's |
There was a problem hiding this comment.
... being more explicit: before this change, these were literal inline constants. is there a good reason to not continue that and just update the value?
we don't need to explain what RLIMIT_NOFILE is if we just set them directly in the Rlimit spec.
This PR could be +2/-2 and easier to read later, the context below you don't know if noFileLimit is computed or constant.
| } | ||
| } | ||
|
|
||
| func TestBuild_NoFileLimit(t *testing.T) { |
There was a problem hiding this comment.
Testing a constant seems overkill?
| Hard: noFileLimit, | ||
| Soft: noFileLimit, |
There was a problem hiding this comment.
| Hard: noFileLimit, | |
| Soft: noFileLimit, | |
| // NOTE: We raised NOFILE because <reason> | |
| Hard: 1048576, | |
| Soft: 1048576, |
6e30794 to
6d90d29
Compare
|
PR got removed from merge queue due to |
The OCI spec pinned RLIMIT_NOFILE for every actor container at 1024 soft and 1024 hard, with which a proxy-heavy actor can run out of sockets. On the other hand, pods have no rlimit field and inherit the runtime's limit, which on GKE is 1048576.
6d90d29 to
4f53100
Compare
Pods have no rlimit field and inherit the runtime's limit, which is 1048576 on GKE for plain pods, containerd, atelet and the runsc sandbox. With no
RLIMIT_NOFILEin the spec, the actor inherits the runsc sandbox process's host limit (1073741816 on kind, 1048576 on GKE).Test plan
kind cluster built from this branch with
hack/create-kind-cluster.shandhack/install-ate-kind.sh --deploy-ate-system --deploy-demo-counter. An actor from the counter image with--validate-existing-file-path=/proc/self/limitsreturns its own limits through atenet-router:🤖 This PR was developed with AI assistance. I have reviewed and tested all changes.