Fix: raise soft open-file limit at serve() startup#83
Fix: raise soft open-file limit at serve() startup#83Ankitsinghsisodya wants to merge 3 commits intoknative-extensions:mainfrom
Conversation
Python does not automatically raise the process soft open-file limit to the hard limit on startup, unlike the Go and Java runtimes. Under container environments that set a low soft limit (e.g. 1024), functions fail under load with "too many open files" errors. Add _raise_nofile_limit() in a new src/func_python/_ulimit.py module and call it at the top of serve() in both http.py and cloudevent.py. The helper caps the target at 65536 to avoid passing RLIM_INFINITY directly to setrlimit (which the kernel rejects), and silently skips on non-Unix platforms where the resource module is unavailable. Add six unit tests in tests/test_ulimit.py covering normal raise, no-op when soft equals hard, RLIM_INFINITY capping, ImportError, OSError, and ValueError cases. Relates to knative/func#3513
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @Ankitsinghsisodya! It looks like this is your first PR to knative-extensions/func-python 🎉 |
There was a problem hiding this comment.
Pull request overview
Adds a startup helper to raise the process open-file soft limit (RLIMIT_NOFILE) so Python functions don’t fail under load on platforms with low default ulimits.
Changes:
- Introduce
src/func_python/_ulimit.pywith_raise_nofile_limit()helper. - Invoke
_raise_nofile_limit()at the top ofserve()in both HTTP and CloudEvent runtimes. - Add unit tests for
_raise_nofile_limit()behavior via mocks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/func_python/_ulimit.py |
New helper to adjust RLIMIT_NOFILE and log success/failure. |
src/func_python/http.py |
Calls the ulimit-raising helper at serve() startup. |
src/func_python/cloudevent.py |
Calls the ulimit-raising helper at serve() startup. |
tests/test_ulimit.py |
New tests validating ulimit adjustment and logging behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import logging | ||
| import sys | ||
| import unittest | ||
| from unittest.mock import MagicMock, patch, call |
There was a problem hiding this comment.
Unused import: call is imported from unittest.mock but never used in this test module. Removing it will avoid lint noise.
| from unittest.mock import MagicMock, patch, call | |
| from unittest.mock import MagicMock, patch |
| # Should complete without raising anything | ||
| ulimit._raise_nofile_limit() | ||
|
|
||
| def test_os_error_logs_warning(self, caplog=None): |
There was a problem hiding this comment.
test_os_error_logs_warning includes an extra caplog=None parameter (a pytest-style fixture) but the test is written using unittest.TestCase and assertLogs. Consider removing the unused parameter to keep the test signature conventional and avoid confusion.
| def test_os_error_logs_warning(self, caplog=None): | |
| def test_os_error_logs_warning(self): |
| if hard == resource.RLIM_INFINITY: | ||
| target = _MAX_NOFILE | ||
| else: | ||
| target = min(hard, _MAX_NOFILE) | ||
| resource.setrlimit(resource.RLIMIT_NOFILE, (target, hard)) |
There was a problem hiding this comment.
When hard != RLIM_INFINITY, the code currently caps target to _MAX_NOFILE via min(hard, _MAX_NOFILE). This diverges from the PR description (“raise soft to the hard limit” and only cap when hard is infinite) and can leave the soft limit below the actual hard limit on systems where hard is high (e.g. 1M). Consider setting target = hard for finite hard limits and only applying _MAX_NOFILE when hard == RLIM_INFINITY (or updating the description if the always-cap behavior is intentional).
| else: | ||
| target = min(hard, _MAX_NOFILE) | ||
| resource.setrlimit(resource.RLIMIT_NOFILE, (target, hard)) |
There was a problem hiding this comment.
The code comment says RLIM_INFINITY cannot be passed directly to setrlimit, but the call still passes hard (which is RLIM_INFINITY in that branch) as the hard component: setrlimit(..., (target, hard)). If RLIMIT_NOFILE rejects RLIM_INFINITY, this will always fail and the soft limit won’t be raised. Consider also capping the hard argument (e.g. use (target, target) when hard == RLIM_INFINITY) so the call can succeed.
| else: | |
| target = min(hard, _MAX_NOFILE) | |
| resource.setrlimit(resource.RLIMIT_NOFILE, (target, hard)) | |
| new_hard = target | |
| else: | |
| target = min(hard, _MAX_NOFILE) | |
| new_hard = hard | |
| resource.setrlimit(resource.RLIMIT_NOFILE, (target, new_hard)) |
| ulimit._raise_nofile_limit() | ||
|
|
||
| mock_resource.setrlimit.assert_called_once_with( | ||
| mock_resource.RLIMIT_NOFILE, (ulimit._MAX_NOFILE, RLIM_INFINITY) |
There was a problem hiding this comment.
This test currently asserts setrlimit is called with (ulimit._MAX_NOFILE, RLIM_INFINITY). If _raise_nofile_limit() is updated to avoid passing RLIM_INFINITY into setrlimit (so the operation can succeed), this expectation should be adjusted accordingly (e.g. hard arg capped to the same finite value).
| mock_resource.RLIMIT_NOFILE, (ulimit._MAX_NOFILE, RLIM_INFINITY) | |
| mock_resource.RLIMIT_NOFILE, | |
| (ulimit._MAX_NOFILE, ulimit._MAX_NOFILE), |
- Remove min(hard, _MAX_NOFILE) cap for finite hard limits; raise soft to the actual hard limit as the Go/Java runtimes do. The _MAX_NOFILE cap now applies only to the RLIM_INFINITY branch where it is needed. - Fix misleading comment: it is the soft limit, not the hard limit, that cannot be RLIM_INFINITY without CAP_SYS_RESOURCE. - Replace importlib.reload()-based test helpers with plain pytest functions that patch sys.modules only at call time, eliminating shared module-state mutation between tests. - Remove dead caplog=None default parameter from test_os_error_logs_warning; caplog is now injected as a proper pytest fixture. - Add test_raises_soft_limit_to_hard_above_65536 to guard against the truncation regression. - Add wire-up tests for http.serve() and cloudevent.serve() confirming _raise_nofile_limit() is called on every serve() invocation.
… versioning - Move `import resource` to module level as `_resource` so tests can patch `func_python._ulimit._resource` directly with unittest.mock.patch, replacing fragile sys.modules manipulation that lacked guaranteed isolation. - Replace `logging.info` with `_logger.debug` (named logger via getLogger(__name__)): routine startup detail should not appear at INFO in production logs; named logger lets users silence func_python logs independently. - Use `resource.RLIM_INFINITY` from the real module in tests instead of a hardcoded magic integer that could silently diverge on non-Linux platforms. - Bump version to 0.8.2 and add CHANGELOG entry for the ulimit feature.
Problem
Python functions fail under load on container platforms that set a low
soft open-file limit (e.g. 1024). The Go and Java runtimes raise the
soft limit to the hard limit automatically at startup; Python does not.
Relates to knative/func#3513
Solution
Add
_raise_nofile_limit()in a newsrc/func_python/_ulimit.pymodule and call it at the very top of
serve()in bothhttp.pyandcloudevent.py. Placing the fix in the library means every deployedfunction benefits from a single version bump, rather than requiring a
rebuild of every container image.
The implementation:
RLIMIT_NOFILEto the hardlimit exactly, matching Go and Java runtime behaviour.
RLIM_INFINITYhard limits: caps the soft limit at_MAX_NOFILE(65536) because the kernel rejects setting the soft limit to
RLIM_INFINITYwithoutCAP_SYS_RESOURCE.resourceis unavailable(
_resource = Noneat module level).DEBUGmessage on success and aWARNINGon failure via anamed logger (
logging.getLogger(__name__)), but never raises — afailure to adjust the limit is not a fatal error.
Changes
src/func_python/_ulimit.py— new shared helpersrc/func_python/http.py— import and call_raise_nofile_limit()at top ofserve()src/func_python/cloudevent.py— sametests/test_ulimit.py— nine unit tests usingpatch("func_python._ulimit._resource")for guaranteed mock isolationpyproject.toml— version bumped to 0.8.2CHANGELOG.md— entry added under 0.8.2Test plan
test_raises_soft_limit_to_hard— soft < hard (finite ≤ 65536),setrlimitcalled with full hard valuetest_raises_soft_limit_to_hard_above_65536— hard = 131072,setrlimitcalled with 131072 (not capped)test_no_change_when_soft_equals_hard—setrlimitnot calledtest_rlim_infinity_capped_at_max— hard isRLIM_INFINITY, target is_MAX_NOFILEtest_import_error_is_silently_skipped—_resource = None, no exceptiontest_os_error_logs_warning—setrlimitraisesOSError, warning logged, no exceptiontest_value_error_logs_warning—setrlimitraisesValueError, warning logged, no exceptiontest_http_serve_calls_raise_nofile_limit— wire-up verified forhttp.serve()test_cloudevent_serve_calls_raise_nofile_limit— wire-up verified forcloudevent.serve()All nine tests pass (
poetry run pytest tests/test_ulimit.py -v).