feat(vector): add support for raising file descriptor limits#3311
Open
vparfonov wants to merge 1 commit into
Open
feat(vector): add support for raising file descriptor limits#3311vparfonov wants to merge 1 commit into
vparfonov wants to merge 1 commit into
Conversation
Contributor
Author
|
/hold |
📝 WalkthroughWalkthroughAdds support for the ChangesRaise-fd-limit annotation support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov 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 |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/features/raise_fd_limit.adoc (1)
1-38: 💤 Low valueConsider clarifying the default behavior when the annotation is absent.
The documentation is comprehensive and well-written. One minor suggestion: line 9 states "The Cluster Logging Operator does not enable this feature by default (
false)" which could be slightly clearer. Consider rephrasing to explicitly state that when the annotation is absent, the behavior is equivalent to setting it to"false".For example: "The Cluster Logging Operator does not enable this feature by default. When the annotation is absent, the behavior is equivalent to
false, matching Vector's default behavior."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/raise_fd_limit.adoc` around lines 1 - 38, Clarify the default behavior when the annotation is absent in the documentation. In the paragraph discussing the default behavior of the Cluster Logging Operator (currently stating "The Cluster Logging Operator does not enable this feature by default (`false`), matching Vector's default behavior"), rephrase it to explicitly state that when the observability.openshift.io/raise-fd-limit annotation is absent, the behavior is equivalent to `false`. The revised text should clearly communicate that omitting the annotation results in the same behavior as setting it to `"false"`, making it unambiguous what happens in the absence of the annotation configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/misc/raise_fd_limit_test.go`:
- Around line 26-32: The test in the It block claiming to verify "with
VECTOR_RAISE_FD_LIMIT env var" does not actually set the
observability.openshift.io/raise-fd-limit annotation or verify the environment
variable is present in the collector. Enhance this test to first set the
annotation to "true" on the forwarder object before calling framework.Deploy(),
then after verifying Vector starts successfully, additionally verify that the
VECTOR_RAISE_FD_LIMIT environment variable is actually set to "true" in the
collector container by examining the environment or pod spec. Consider creating
additional test cases for the annotation set to "false", omitted entirely, and
with an invalid value, verifying the corresponding environment variable values
and any status conditions as appropriate.
---
Nitpick comments:
In `@docs/features/raise_fd_limit.adoc`:
- Around line 1-38: Clarify the default behavior when the annotation is absent
in the documentation. In the paragraph discussing the default behavior of the
Cluster Logging Operator (currently stating "The Cluster Logging Operator does
not enable this feature by default (`false`), matching Vector's default
behavior"), rephrase it to explicitly state that when the
observability.openshift.io/raise-fd-limit annotation is absent, the behavior is
equivalent to `false`. The revised text should clearly communicate that omitting
the annotation results in the same behavior as setting it to `"false"`, making
it unambiguous what happens in the absence of the annotation configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47d81bf4-7967-4ea7-9515-979bd502d52b
📒 Files selected for processing (9)
api/observability/v1/conditions.godocs/features/raise_fd_limit.adocinternal/collector/collector.gointernal/collector/collector_test.gointernal/constants/annotations.gointernal/validations/observability/validate.gointernal/validations/observability/validate_annotations.gointernal/validations/observability/validate_annotations_test.gotest/functional/misc/raise_fd_limit_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/functional/misc/raise_fd_limit_test.go (1)
28-37:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBoth functional tests bypass the annotation mechanism and do not validate the feature. Both test cases manually inject the
VECTOR_RAISE_FD_LIMITenvironment variable viaDeployWithVisitor, completely bypassing the annotation-to-environment-variable conversion that this PR implements. These tests only verify that manually-set env vars can be read back, not that theobservability.openshift.io/raise-fd-limitannotation is processed byRaiseFdLimit()and injected into the collector container.
test/functional/misc/raise_fd_limit_test.go#L28-L37: ReplaceDeployWithVisitorwith settingframework.Forwarder.Annotations[constants.AnnotationVectorRaiseFdLimit] = "true"before callingframework.Deploy().test/functional/misc/raise_fd_limit_test.go#L39-L48: ReplaceDeployWithVisitorwith settingframework.Forwarder.Annotations[constants.AnnotationVectorRaiseFdLimit] = "false"before callingframework.Deploy().Additionally, add test cases for: (1) omitted annotation (verify default
"false"), and (2) invalid annotation value (verify default"false"and status condition if applicable).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/misc/raise_fd_limit_test.go` around lines 28 - 37, The tests in test/functional/misc/raise_fd_limit_test.go are bypassing the annotation mechanism by manually setting the VECTOR_RAISE_FD_LIMIT environment variable via DeployWithVisitor instead of validating that the annotation processing works correctly. At lines 28-37 (anchor), replace the DeployWithVisitor call with setting framework.Forwarder.Annotations[constants.AnnotationVectorRaiseFdLimit] = "true" before calling framework.Deploy(). At lines 39-48 (sibling), make the same change but set the annotation to "false". Then remove the manual environment variable injection and the RunCommand verification, since the tests should now verify that the annotation is properly processed by the RaiseFdLimit() function. Finally, add two additional test cases: one verifying the default behavior when the annotation is omitted, and another verifying the behavior when an invalid annotation value is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/functional/misc/raise_fd_limit_test.go`:
- Around line 28-37: The tests in test/functional/misc/raise_fd_limit_test.go
are bypassing the annotation mechanism by manually setting the
VECTOR_RAISE_FD_LIMIT environment variable via DeployWithVisitor instead of
validating that the annotation processing works correctly. At lines 28-37
(anchor), replace the DeployWithVisitor call with setting
framework.Forwarder.Annotations[constants.AnnotationVectorRaiseFdLimit] = "true"
before calling framework.Deploy(). At lines 39-48 (sibling), make the same
change but set the annotation to "false". Then remove the manual environment
variable injection and the RunCommand verification, since the tests should now
verify that the annotation is properly processed by the RaiseFdLimit() function.
Finally, add two additional test cases: one verifying the default behavior when
the annotation is omitted, and another verifying the behavior when an invalid
annotation value is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9bb147d9-a0ad-43d8-94d3-3435e69fc064
📒 Files selected for processing (9)
api/observability/v1/conditions.godocs/features/raise_fd_limit.adocinternal/collector/collector.gointernal/collector/collector_test.gointernal/constants/annotations.gointernal/validations/observability/validate.gointernal/validations/observability/validate_annotations.gointernal/validations/observability/validate_annotations_test.gotest/functional/misc/raise_fd_limit_test.go
✅ Files skipped from review due to trivial changes (1)
- api/observability/v1/conditions.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/validations/observability/validate_annotations_test.go
- internal/validations/observability/validate.go
- internal/validations/observability/validate_annotations.go
- internal/constants/annotations.go
- internal/collector/collector.go
Contributor
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-ci
Bot
added
the
needs-rebase
Add annotation-driven support for Vector's VECTOR_RAISE_FD_LIMIT env var, enabling the collector to raise file descriptor soft limits at startup. This prevents "Too many open files" errors when monitoring large numbers of log files concurrently. Key changes: - New annotation: observability.openshift.io/raise-fd-limit (default: false) - Sets VECTOR_RAISE_FD_LIMIT env var on the collector container - Vector reads the env var natively, no entrypoint script changes needed - Input validation rejects invalid values (only "true"/"false" allowed) - Status condition reports validation errors to the user Default matches Vector's behavior (false = don't raise fd limits). Users must explicitly set the annotation to "true" to opt in. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/spelling/patterns.txt (1)
55-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the hyphenated-name ignore pattern.
\b[a-z]+(?:-[a-z]+)+\bwill also match common prose like “end-to-end” and “follow-up”, which weakens spell-check coverage far beyond Kubernetes resource names. A tighter allow-list or a more specific resource-name regex would be safer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/spelling/patterns.txt around lines 55 - 62, Narrow the hyphenated-name ignore rule in the spelling patterns so it only matches Kubernetes-style resource names, not general prose. Update the regex entry in the patterns file near the existing resource-name rule to be more specific (or replace it with a tighter allow-list) while keeping the other ignore patterns like the mixed-identifier and pod hash rules intact. Use the existing comment context around the Kubernetes resource-name pattern to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/actions/spelling/patterns.txt:
- Around line 55-62: Narrow the hyphenated-name ignore rule in the spelling
patterns so it only matches Kubernetes-style resource names, not general prose.
Update the regex entry in the patterns file near the existing resource-name rule
to be more specific (or replace it with a tighter allow-list) while keeping the
other ignore patterns like the mixed-identifier and pod hash rules intact. Use
the existing comment context around the Kubernetes resource-name pattern to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a35f63f-3153-40eb-b24e-11d737942dad
📒 Files selected for processing (10)
.github/actions/spelling/patterns.txtapi/observability/v1/conditions.godocs/features/raise_fd_limit.adocinternal/collector/collector.gointernal/collector/collector_test.gointernal/constants/annotations.gointernal/validations/observability/validate.gointernal/validations/observability/validate_annotations.gointernal/validations/observability/validate_annotations_test.gotest/functional/misc/raise_fd_limit_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/features/raise_fd_limit.adoc
- api/observability/v1/conditions.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/collector/collector_test.go
- test/functional/misc/raise_fd_limit_test.go
- internal/validations/observability/validate_annotations_test.go
- internal/collector/collector.go
- internal/validations/observability/validate_annotations.go
- internal/validations/observability/validate.go
Contributor
Author
|
/retest |
Contributor
|
@vparfonov: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds annotation-driven support for Vector's
VECTOR_RAISE_FD_LIMITenv var, allowing users to opt in to raising file descriptor soft limits at startup. This prevents "Too many open files" errors when Vector monitors large numbers of log files concurrently.Details
observability.openshift.io/raise-fd-limit"true"raise fd soft limit to match hard limit"false"(default) no change, matches Vector's default behaviorVECTOR_RAISE_FD_LIMITenv var on the collector container; Vector reads it nativelyUsage
/cc
/assign @jcantril
Links
Summary by CodeRabbit
observability.openshift.io/raise-fd-limitannotation forClusterLogForwarderto control whether Vector raises the process file descriptor soft limit at startup (default:false; valid values:true/false).falsefor invalid values and surfacing a status condition when misconfigured.