fix(telemetry): set kernel user to match workload user for logs#572
Closed
kaixi-wang wants to merge 3 commits into
Closed
fix(telemetry): set kernel user to match workload user for logs#572kaixi-wang wants to merge 3 commits into
kaixi-wang wants to merge 3 commits into
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds telemetry sidecar support to FiftyOne Enterprise v2.19+, enabling per-service metrics and logs via bundled Redis backend. Changes span Docker Compose and Kubernetes deployment documentation, Helm chart templates, comprehensive test coverage for new telemetry components, and updates to existing tests and fixtures to account for telemetry services and environment configuration. ChangesTelemetry Deployment and Documentation
Helm Chart Telemetry Implementation
Telemetry Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/helm/delegated-operator-instance-deployment_test.go (1)
5764-5769: ⚡ Quick winUse the shared YAML-doc splitter in this test.
strings.Split(output, "---")with positional indexing is brittle. Reusing the test helper for multi-doc YAML parsing will make this check less formatting-sensitive.🤖 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 `@tests/unit/helm/delegated-operator-instance-deployment_test.go` around lines 5764 - 5769, Replace the brittle strings.Split(output, "---") + docs[1] approach with the repository's shared YAML-doc splitter helper (e.g., SplitYAMLDocuments or testhelper.SplitYAMLDocs) to parse output into documents; call that helper with output, assert you got the expected number of docs, locate the deployment document from the returned slice, and pass that document to helm.UnmarshalK8SYaml(subT, doc, &deployment) instead of using positional indexing on the raw split.tests/unit/helm/delegated-operator-job-configmap_test.go (1)
748-751: ⚡ Quick winAvoid relying on container index for mount assertions.
Asserting against
Containers[0]is order-sensitive. Resolve the target workload container by name first, then assert telemetry mount count on that container.🤖 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 `@tests/unit/helm/delegated-operator-job-configmap_test.go` around lines 748 - 751, The test currently indexes Containers[0] which is fragile; instead locate the target container by matching its Name (e.g., find the container where c.Name == <expected container name> used in the test) and then pass that container's VolumeMounts into countMountsByName along with socketName; replace any use of job.Spec.Template.Spec.Containers[0] with a lookup (fail the test if the named container is not found) and then assert that countMountsByName(foundContainer.VolumeMounts, socketName) equals testCase.expectMnt.
🤖 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 `@docker/docs/configuring-gpu-workloads.md`:
- Around line 57-58: The example COMPOSE_PROFILES value uses an invalid profile
`do-1,gpu`; update the example to a valid Compose profile for slot + GPU, e.g.
use `COMPOSE_PROFILES=gpu` or `COMPOSE_PROFILES=slot-2,gpu` instead of
`do-1,gpu` so the documentation reflects the new slot model; change the example
string in the paragraph that shows COMPOSE_PROFILES accordingly.
In `@docker/docs/upgrading.md`:
- Around line 143-144: The markdown line describing the capability requirement
has unbalanced bold markers around SYS_PTRACE which breaks rendering; update the
line in the documentation that mentions teams-do and SYS_PTRACE so the bold
markers are balanced (for example ensure both opening and closing ** wrap
SYS_PTRACE or wrap the entire phrase **`SYS_PTRACE`** consistently), preserving
the surrounding wording and inline code formatting for teams-do and the
capability name.
In `@helm/fiftyone-teams-app/templates/NOTES.txt`:
- Line 7: The INFO note's Helm conditional in templates/NOTES.txt incorrectly
requires both .Values.telemetry.redis.persistence.enabled to be false AND
.Values.telemetry.redis.persistence.existingClaim to be false to show the
warning; change the condition so the warning appears when persistence is not
effectively persistent by using an OR between those checks. Update the if
expression that currently reads (not
.Values.telemetry.redis.persistence.enabled) (not
.Values.telemetry.redis.persistence.existingClaim) to use (or (not
.Values.telemetry.redis.persistence.enabled) (not
.Values.telemetry.redis.persistence.existingClaim)) while keeping the other
checks (.Values.telemetry.enabled and not .Values.telemetry.redis.external.url)
the same so the note shows when Redis is ephemeral.
In `@tests/unit/helm/telemetry-redis_test.go`:
- Around line 139-147: The test currently only calls s.Equal(expectedURL,
ev.Value) when it finds an env var named "FIFTYONE_TELEMETRY_REDIS_URL", so a
missing env var can let the test pass; update the loop over
deployment.Spec.Template.Spec.Containers to first assert the env var exists for
each container (e.g., set a found flag or use s.Require().Truef/Assertf)
referencing "FIFTYONE_TELEMETRY_REDIS_URL" and container.Name, and only then
compare ev.Value to expectedURL with s.Equal(expectedURL, ev.Value) so missing
variables fail the test explicitly.
In `@tests/unit/helm/telemetry-sidecar_test.go`:
- Around line 183-194: The test currently only checks presence of "SYS_PTRACE"
via sc.Capabilities.Add and may miss accidental extra capabilities; update the
assertion to verify the exact capabilities list when tc.executor is true and
empty when false by comparing sc.Capabilities.Add to the expected slice/set.
Locate the loop and hasPtrace check around sc.Capabilities.Add and replace it
with an exact equality/assertion (e.g., compare as a sorted slice or convert to
a set) using tc.executor to decide the expected value so the test fails if any
unexpected capability is present.
---
Nitpick comments:
In `@tests/unit/helm/delegated-operator-instance-deployment_test.go`:
- Around line 5764-5769: Replace the brittle strings.Split(output, "---") +
docs[1] approach with the repository's shared YAML-doc splitter helper (e.g.,
SplitYAMLDocuments or testhelper.SplitYAMLDocs) to parse output into documents;
call that helper with output, assert you got the expected number of docs, locate
the deployment document from the returned slice, and pass that document to
helm.UnmarshalK8SYaml(subT, doc, &deployment) instead of using positional
indexing on the raw split.
In `@tests/unit/helm/delegated-operator-job-configmap_test.go`:
- Around line 748-751: The test currently indexes Containers[0] which is
fragile; instead locate the target container by matching its Name (e.g., find
the container where c.Name == <expected container name> used in the test) and
then pass that container's VolumeMounts into countMountsByName along with
socketName; replace any use of job.Spec.Template.Spec.Containers[0] with a
lookup (fail the test if the named container is not found) and then assert that
countMountsByName(foundContainer.VolumeMounts, socketName) equals
testCase.expectMnt.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4a13e15-af1b-4fb1-88ad-bc3da0c57e02
⛔ Files ignored due to path filters (32)
docker/common-services.yamlis excluded by!**/*.yamldocker/internal-auth/compose.dedicated-plugins.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.yamlis excluded by!**/*.yamldocker/internal-auth/compose.plugins.yamlis excluded by!**/*.yamldocker/internal-auth/compose.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.dedicated-plugins.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.plugins.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/api-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/api-role.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/app-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/delegated-operator-instance-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/delegated-operator-job-configmap.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/plugins-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-redis-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-redis-pvc.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-redis-service.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-role.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-rolebinding.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/values.schema.jsonis excluded by!**/*.jsonhelm/fiftyone-teams-app/values.yamlis excluded by!**/*.yamltests/fixtures/docker/compose.override.mongodb.yamlis excluded by!**/*.yamltests/fixtures/docker/compose.override.mongodb_do.yamlis excluded by!**/*.yamltests/fixtures/docker/compose.override.mongodb_plugins.yamlis excluded by!**/*.yamltests/fixtures/helm/integration_values.yamlis excluded by!**/*.yamltests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-cpu-default-override-template-values.yamlis excluded by!**/*.yamltests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-cpu-default.yamlis excluded by!**/*.yamltests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-override-example-cascading-template.yamlis excluded by!**/*.yamltests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-override-example-template.yamlis excluded by!**/*.yaml
📒 Files selected for processing (34)
docker/README.mddocker/docs/configuring-delegated-operators.mddocker/docs/configuring-gpu-workloads.mddocker/docs/configuring-telemetry.mddocker/docs/upgrading.mddocker/internal-auth/env.templatedocker/legacy-auth/env.templatedocs/orchestrators/configuring-kubernetes-orchestrator.mdhelm/docs/upgrading.mdhelm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/templates/NOTES.txthelm/fiftyone-teams-app/templates/_do_targets.tplhelm/fiftyone-teams-app/templates/_helpers.tplhelm/fiftyone-teams-app/templates/_telemetry.tpltests/fixtures/docker/integration_internal_auth.envtests/fixtures/docker/integration_legacy_auth.envtests/integration/compose/docker-compose-internal-auth_test.gotests/integration/compose/docker-compose-legacy-auth_test.gotests/unit/compose/docker-compose-internal-auth_test.gotests/unit/compose/docker-compose-legacy-auth_test.gotests/unit/helm/api-deployment_test.gotests/unit/helm/api-role_test.gotests/unit/helm/app-deployment_test.gotests/unit/helm/common_test.gotests/unit/helm/delegated-operator-instance-deployment_test.gotests/unit/helm/delegated-operator-job-configmap_test.gotests/unit/helm/plugins-deployment_test.gotests/unit/helm/teams-app-deployment_test.gotests/unit/helm/telemetry-redis_test.gotests/unit/helm/telemetry-rolebinding_test.gotests/unit/helm/telemetry-sidecar_test.gotests/unit/helm/yaml_helpers.goutils/bump-fixtures-helm.shutils/validate-docker-pulls.sh
Comment on lines
+57
to
+58
| worker profile (e.g. `COMPOSE_PROFILES=do-1,gpu`); see | ||
| [Configuring Telemetry](./configuring-telemetry.md#scaling-teams-do-with-telemetry) |
Contributor
There was a problem hiding this comment.
Use a valid Compose profile example for slot 1 + GPU.
do-1 is not part of the new slot model (slot 1 is always on). This example can mislead users into setting a non-existent profile.
Suggested edit
- worker profile (e.g. `COMPOSE_PROFILES=do-1,gpu`); see
+ worker profile (e.g. `COMPOSE_PROFILES=gpu`); see📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| worker profile (e.g. `COMPOSE_PROFILES=do-1,gpu`); see | |
| [Configuring Telemetry](./configuring-telemetry.md#scaling-teams-do-with-telemetry) | |
| worker profile (e.g. `COMPOSE_PROFILES=gpu`); see | |
| [Configuring Telemetry](./configuring-telemetry.md#scaling-teams-do-with-telemetry) |
🤖 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 `@docker/docs/configuring-gpu-workloads.md` around lines 57 - 58, The example
COMPOSE_PROFILES value uses an invalid profile `do-1,gpu`; update the example to
a valid Compose profile for slot + GPU, e.g. use `COMPOSE_PROFILES=gpu` or
`COMPOSE_PROFILES=slot-2,gpu` instead of `do-1,gpu` so the documentation
reflects the new slot model; change the example string in the paragraph that
shows COMPOSE_PROFILES accordingly.
Comment on lines
+143
to
+144
| 1. **`teams-do` requires the **`SYS_PTRACE`** capability to allow the | ||
| telemetry agent to observe the target process. |
Contributor
There was a problem hiding this comment.
Fix malformed emphasis formatting in capability requirement line.
The bold markers are unbalanced around SYS_PTRACE, which breaks markdown rendering.
Suggested edit
-1. **`teams-do` requires the **`SYS_PTRACE`** capability to allow the
+1. **`teams-do` requires the `SYS_PTRACE` capability** to allow the🤖 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 `@docker/docs/upgrading.md` around lines 143 - 144, The markdown line
describing the capability requirement has unbalanced bold markers around
SYS_PTRACE which breaks rendering; update the line in the documentation that
mentions teams-do and SYS_PTRACE so the bold markers are balanced (for example
ensure both opening and closing ** wrap SYS_PTRACE or wrap the entire phrase
**`SYS_PTRACE`** consistently), preserving the surrounding wording and inline
code formatting for teams-do and the capability name.
| https://helm.fiftyone.ai for details. | ||
| {{ end }} | ||
|
|
||
| {{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) (not .Values.telemetry.redis.persistence.existingClaim) }} |
Contributor
There was a problem hiding this comment.
Fix the warning condition for effective emptyDir mode.
Line 7 currently hides this INFO note when telemetry.redis.persistence.existingClaim is set, even if telemetry.redis.persistence.enabled is false. In that case Redis is still ephemeral, so the warning should still appear.
Proposed fix
-{{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) (not .Values.telemetry.redis.persistence.existingClaim) }}
+{{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) (not .Values.telemetry.redis.persistence.existingClaim) }} | |
| {{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) }} |
🧰 Tools
🪛 LanguageTool
[style] ~7-~7: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ues.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) (n...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~7-~7: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...emetry.redis.persistence.enabled) (not .Values.telemetry.redis.persistence.existingCla...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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 `@helm/fiftyone-teams-app/templates/NOTES.txt` at line 7, The INFO note's Helm
conditional in templates/NOTES.txt incorrectly requires both
.Values.telemetry.redis.persistence.enabled to be false AND
.Values.telemetry.redis.persistence.existingClaim to be false to show the
warning; change the condition so the warning appears when persistence is not
effectively persistent by using an OR between those checks. Update the if
expression that currently reads (not
.Values.telemetry.redis.persistence.enabled) (not
.Values.telemetry.redis.persistence.existingClaim) to use (or (not
.Values.telemetry.redis.persistence.enabled) (not
.Values.telemetry.redis.persistence.existingClaim)) while keeping the other
checks (.Values.telemetry.enabled and not .Values.telemetry.redis.external.url)
the same so the note shows when Redis is ephemeral.
Comment on lines
+139
to
+147
| for _, container := range deployment.Spec.Template.Spec.Containers { | ||
| for _, ev := range container.Env { | ||
| if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" { | ||
| s.Equal(expectedURL, ev.Value, | ||
| "FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL", | ||
| container.Name) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Assert env var presence in bundled URL test before checking value.
At Line 139, this test only validates the value if FIFTYONE_TELEMETRY_REDIS_URL exists, so it can pass even when the env var is missing on a container.
Suggested fix
expectedURL := fmt.Sprintf("redis://%s-telemetry-redis.%s.svc.cluster.local:6379",
s.releaseName, "fiftyone-teams")
for _, container := range deployment.Spec.Template.Spec.Containers {
- for _, ev := range container.Env {
- if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
- s.Equal(expectedURL, ev.Value,
- "FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
- container.Name)
- }
- }
+ var found *corev1.EnvVar
+ for i, ev := range container.Env {
+ if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
+ found = &container.Env[i]
+ break
+ }
+ }
+ s.Require().NotNil(found,
+ "FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", container.Name)
+ s.Equal(expectedURL, found.Value,
+ "FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
+ container.Name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, container := range deployment.Spec.Template.Spec.Containers { | |
| for _, ev := range container.Env { | |
| if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" { | |
| s.Equal(expectedURL, ev.Value, | |
| "FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL", | |
| container.Name) | |
| } | |
| } | |
| } | |
| for _, container := range deployment.Spec.Template.Spec.Containers { | |
| var found *corev1.EnvVar | |
| for i, ev := range container.Env { | |
| if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" { | |
| found = &container.Env[i] | |
| break | |
| } | |
| } | |
| s.Require().NotNil(found, | |
| "FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", container.Name) | |
| s.Equal(expectedURL, found.Value, | |
| "FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL", | |
| container.Name) | |
| } |
🤖 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 `@tests/unit/helm/telemetry-redis_test.go` around lines 139 - 147, The test
currently only calls s.Equal(expectedURL, ev.Value) when it finds an env var
named "FIFTYONE_TELEMETRY_REDIS_URL", so a missing env var can let the test
pass; update the loop over deployment.Spec.Template.Spec.Containers to first
assert the env var exists for each container (e.g., set a found flag or use
s.Require().Truef/Assertf) referencing "FIFTYONE_TELEMETRY_REDIS_URL" and
container.Name, and only then compare ev.Value to expectedURL with
s.Equal(expectedURL, ev.Value) so missing variables fail the test explicitly.
Comment on lines
+183
to
+194
| var hasPtrace bool | ||
| for _, capability := range sc.Capabilities.Add { | ||
| if capability == "SYS_PTRACE" { | ||
| hasPtrace = true | ||
| break | ||
| } | ||
| } | ||
| if tc.executor { | ||
| s.True(hasPtrace, "executor sidecar must add SYS_PTRACE for py-spy crash archives") | ||
| } else { | ||
| s.False(hasPtrace, "service-mode sidecar must not add SYS_PTRACE") | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten capability assertions to prevent unnoticed extra capabilities.
At Line 190, the test only checks whether SYS_PTRACE exists. It won’t fail if other capabilities are accidentally added.
Suggested fix
- var hasPtrace bool
- for _, capability := range sc.Capabilities.Add {
- if capability == "SYS_PTRACE" {
- hasPtrace = true
- break
- }
- }
- if tc.executor {
- s.True(hasPtrace, "executor sidecar must add SYS_PTRACE for py-spy crash archives")
- } else {
- s.False(hasPtrace, "service-mode sidecar must not add SYS_PTRACE")
- }
+ if tc.executor {
+ s.ElementsMatch(
+ []corev1.Capability{"SYS_PTRACE"},
+ sc.Capabilities.Add,
+ "executor sidecar should only add SYS_PTRACE")
+ } else {
+ s.Empty(sc.Capabilities.Add, "service-mode sidecar must not add capabilities")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var hasPtrace bool | |
| for _, capability := range sc.Capabilities.Add { | |
| if capability == "SYS_PTRACE" { | |
| hasPtrace = true | |
| break | |
| } | |
| } | |
| if tc.executor { | |
| s.True(hasPtrace, "executor sidecar must add SYS_PTRACE for py-spy crash archives") | |
| } else { | |
| s.False(hasPtrace, "service-mode sidecar must not add SYS_PTRACE") | |
| } | |
| if tc.executor { | |
| s.ElementsMatch( | |
| []corev1.Capability{"SYS_PTRACE"}, | |
| sc.Capabilities.Add, | |
| "executor sidecar should only add SYS_PTRACE") | |
| } else { | |
| s.Empty(sc.Capabilities.Add, "service-mode sidecar must not add capabilities") | |
| } |
🤖 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 `@tests/unit/helm/telemetry-sidecar_test.go` around lines 183 - 194, The test
currently only checks presence of "SYS_PTRACE" via sc.Capabilities.Add and may
miss accidental extra capabilities; update the assertion to verify the exact
capabilities list when tc.executor is true and empty when false by comparing
sc.Capabilities.Add to the expected slice/set. Locate the loop and hasPtrace
check around sc.Capabilities.Add and replace it with an exact equality/assertion
(e.g., compare as a sorted slice or convert to a set) using tc.executor to
decide the expected value so the test fails if any unexpected capability is
present.
kaixi-wang
changed the base branch from
release/v2.19.0
to
alan/telemetry-including-redis
kaixi-wang
commented
May 27, 2026
| // Fast" startup line instead — works regardless of worker count or log | ||
| // level (INFO is the default). | ||
| log: "Goin' Fast", | ||
| log: "Goin' Fast", |
Member
Author
There was a problem hiding this comment.
"Starting worker" only fires in multi-worker mode. Goin' Fast prints in both
Member
Author
|
closing in favor of #566 |
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.
Rationale
Review Priority
Changes
Checklist
Testing