fix executor sidecar#574
Closed
kaixi-wang wants to merge 93 commits into
Closed
Conversation
…v2.19.0-rc.5 api:v2.19.0rc6 app:v2.19.0rc6 cas:v2.19.0-rc.5
Add an opt-in escape hatch for operators running managed Redis (ElastiCache, MemoryStore, etc.) or an existing shared Redis. When `telemetry.redis.external.url` is set, the chart skips the bundled Redis `Deployment`/`Service`/`PersistentVolumeClaim` and wires both consumer workloads and auto-injected sidecars at the external URL. Threads through a new `telemetry.redis.url` helper that returns either the external URL or the in-cluster `redis://<release>-telemetry-redis` URL, replacing the inline `printf` previously used in two places. Tests in tests/unit/helm/telemetry-redis_test.go cover: bundled Redis NOT rendered when external.url set; api deployment env var points at the external URL on every container (workload + sidecar); the default release-scoped URL still wires correctly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the single `telemetry.redis.storage` knob with a `telemetry.redis.persistence` block: - `persistence.enabled` (default `true`) — when `false`, skip the PVC and run Redis on an `emptyDir`. State is lost on pod restart; intended for dev/minikube clusters without a dynamic PV provisioner. - `persistence.size` — replaces `storage`. Same default (`1Gi`). - `persistence.storageClass` — passed through to the PVC. Leave unset to use the cluster's default `StorageClass`. Tests added: PVC has the expected `storageClassName` when set; `persistence.enabled=false` skips the PVC and the Deployment switches the `redis-data` volume to `emptyDir`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The delegated-operator workload containers and DO Job templates were still building `FIFTYONE_TELEMETRY_REDIS_URL` inline via `printf "redis://%s:6379" (include "telemetry.redis.name" .ctx)`, bypassing the `telemetry.redis.url` helper added in the external-Redis commit. As a result, setting `telemetry.redis.external.url` correctly pointed api/app/cas/plugins/teams-app at the external URL but left the DO workloads pointing at the in-cluster Service. Both sites now go through `telemetry.redis.url .ctx`. Regression tests added to tests/unit/helm/telemetry-redis_test.go cover both the DO deployment workload container and the DO Job ConfigMap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…v2.19.0-rc.6 api:v2.19.0rc7 app:v2.19.0rc7 cas:v2.19.0-rc.6
- pin sidecar image default to v0.1.62 (matches helm `telemetry.sidecar.image`); update unit-test expectations in lockstep - add `TELEMETRY_REDIS_MAXMEMORY_POLICY` env knob (was hardcoded to `allkeys-lru`; now mirrors helm `telemetry.redis.maxmemoryPolicy`) - add `deploy.resources` cpu/memory limits + reservations to telemetry Redis and every sidecar service to match the helm defaults (Redis: 0.25/512M lim, 0.10/256M res; sidecars: 0.10/512M) - rewrite env.template telemetry section: drop stale `compose.telemetry.yaml overlay` reference (bundled by default since the sidecar work landed), document the new policy knob and the v0.1.62 pin, and clarify when to override - expand `docs/configuring-telemetry.md` with the new env var and a resource-limits table cross-referencing the helm defaults Applies symmetrically to internal-auth and legacy-auth, including the plugins / dedicated-plugins / delegated-operators / delegated-operators GPU overlay variants.
…s.yaml The docker README documents that `compose.plugins.yaml` and `compose.dedicated-plugins.yaml` are used *instead of* `compose.yaml` (not layered on top of it), which previously forced each of the three base files to redefine `telemetry-redis`, `fiftyone-app-telemetry`, and `teams-api-telemetry` verbatim. Bumping image tags or resource limits meant editing the same block three times — and missing one would silently desync the deployments. Move the telemetry services into `common-services.yaml` with `-common` suffixes (mirroring the existing pattern for `fiftyone-app-common`, `teams-api-common`, etc.), then have each compose file pull them in via `extends:`. Each child redeclares `depends_on` because compose's `extends` intentionally does not propagate it. The rendered `docker compose config` output is byte-identical to the pre-refactor version for every combo across both auth flavors; this is a pure structural cleanup with no runtime behavior change. `compose.delegated-operators.gpu.yaml` keeps its standalone sidecar definition: it's only used in one file, and the GPU device reservation diverges enough from the regular DO sidecar to make `extends` more trouble than it saves.
The default RollingUpdate strategy spins up a new pod before the old one terminates, which fails with a Multi-Attach error on the RWO PVC. Recreate serializes pod replacement so the volume can detach first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The project-level `volumes:` block in compose.delegated-operators.yaml now declares per-slot executor-socket volumes for `teams-do-2` and `teams-do-3`, so they appear in `project.Volumes` regardless of which `do-N` profile is active. Add them to the expected volume set in the internal-auth and legacy-auth `TestVolumes/delegatedOperations` cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Alan Smith <209585+mo-getter@users.noreply.github.com>
The integration tests asserted on the literal string `Container <project>-<svc>-1 Started` (two spaces) emitted by older Docker Compose CLI versions. Compose v5.x emits a single space, so the `s.Contains` check fails for every service even though `up` succeeded — surfacing as a parent FAIL on every TestDockerComposeUp case while leaf log assertions still PASS. Switch to `s.Regexp` with `\s+` so the assertion matches both formats. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chore: fix do integration test
…v2.19.0-rc.13 api:v2.19.0rc14 app:v2.19.0rc14 cas:v2.19.0-rc.13
…1000 Same-UID fast path through ptrace_may_access() lets the sidecar tail /proc/<pid>/fd/1 without DAC_READ_SEARCH, matching the common sidecar definitions already on this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…v2.19.0-rc.14 api:v2.19.0rc15 app:v2.19.0rc15 cas:v2.19.0-rc.14
…v2.19.0-rc.15 api:v2.19.0rc16 app:v2.19.0rc16 cas:v2.19.0-rc.15
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 PR introduces comprehensive telemetry sidecar support for FiftyOne Enterprise v2.19.0+, spanning Docker Compose and Kubernetes Helm deployments. Changes include telemetry documentation, Helm template implementation, Docker environment configuration, and extensive test coverage ensuring telemetry is injected and configured correctly across all deployment scenarios. ChangesTelemetry Feature Implementation and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (3)
tests/unit/helm/telemetry-sidecar_test.go (2)
126-130: ⚡ Quick winMake telemetry-off assertion strict for
ShareProcessNamespace.Line 126 currently allows an explicit
falsevalue, but the test description says the field should be unset when telemetry is off. Assertnildirectly so regressions are caught precisely.Proposed diff
- if deployment.Spec.Template.Spec.ShareProcessNamespace != nil { - s.False(*deployment.Spec.Template.Spec.ShareProcessNamespace, - "shareProcessNamespace should not be true on %s when telemetry is disabled", - tc.template) - } + s.Nil(deployment.Spec.Template.Spec.ShareProcessNamespace, + "shareProcessNamespace should be unset on %s when telemetry is disabled", + tc.template)🤖 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 126 - 130, The test currently uses s.False to check deployment.Spec.Template.Spec.ShareProcessNamespace allowing an explicit false; change the assertion to require the pointer be nil to enforce the field is unset when telemetry is disabled. Locate the assertion referencing deployment.Spec.Template.Spec.ShareProcessNamespace in telemetry-sidecar_test.go and replace the s.False check with s.Nil on deployment.Spec.Template.Spec.ShareProcessNamespace (keeping the same failure message/context) so the test fails if the field is explicitly set to true or false instead of being omitted.
183-194: ⚡ Quick winAssert exact
Capabilities.Addto avoid silent privilege creep.Current checks only verify
SYS_PTRACEpresence/absence, so extra added capabilities could slip through unnoticed.Proposed diff
- 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") + s.Equal([]corev1.Capability{"SYS_PTRACE"}, sc.Capabilities.Add, + "executor sidecar should only add SYS_PTRACE") } else { - s.False(hasPtrace, "service-mode sidecar must not add SYS_PTRACE") + 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/absence of "SYS_PTRACE" by iterating over sc.Capabilities.Add (using hasPtrace) which allows extra capabilities to be added unnoticed; update the assertion to compare sc.Capabilities.Add exactly against the expected slice for each case (when tc.executor true assert sc.Capabilities.Add equals the exact expected list including "SYS_PTRACE", when tc.executor false assert it equals the exact expected list without "SYS_PTRACE"), using the test helper/assert method already in use so the test fails if any unexpected capability is present.tests/unit/helm/delegated-operator-job-configmap_test.go (1)
698-726: ⚡ Quick winAdd a telemetry-disabled regression case for socket injection.
This table only validates telemetry-enabled paths. Please add a
telemetry.enabled=falsecase expecting zerotelemetry-socketvolume/mount so unconditional injection regressions are caught.Proposed test-case addition
testCases := []struct { name string values map[string]string expectVol int // expected occurrences of telemetry-socket volume expectMnt int // expected occurrences of telemetry-socket mount }{ + { + name: "doesNotInjectWhenTelemetryDisabled", + values: map[string]string{ + "telemetry.enabled": "false", + "delegatedOperatorJobTemplates.jobs.cpuDefault.unused": "nil", + }, + expectVol: 0, + expectMnt: 0, + }, { name: "autoInjectsWhenUserHasNoTelemetrySocket", values: map[string]string{ "telemetry.enabled": "true", "delegatedOperatorJobTemplates.jobs.cpuDefault.unused": "nil",🤖 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 698 - 726, Add a new test case in the testCases slice to cover telemetry-disabled behavior: when "telemetry.enabled" is set to "false" (alongside existing keys like "delegatedOperatorJobTemplates.jobs.cpuDefault.unused"), assert expectVol: 0 and expectMnt: 0 so the test verifies no automatic injection of the "telemetry-socket" volume/mount; locate the testCases variable in delegated-operator-job-configmap_test.go and add the new case (referencing the existing field names expectVol, expectMnt, and keys under delegatedOperatorJobTemplates) to prevent unconditional injection regressions.
🤖 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
"COMPOSE_PROFILES=do-1,gpu" is invalid because profiles start at do-2; update
the inline example in the text that currently reads "worker profile (e.g.
`COMPOSE_PROFILES=do-1,gpu`)" to use a valid profile example such as
"`COMPOSE_PROFILES=gpu`" (or show alternatives "`COMPOSE_PROFILES=do-2,gpu`" /
"`COMPOSE_PROFILES=do-3,gpu`" when describing adding extra CPU worker slots) so
readers aren't guided to a non-existent profile.
In `@docker/docs/upgrading.md`:
- Around line 143-144: Fix the malformed Markdown emphasis by balancing the bold
markers around the service name and capability: ensure `teams-do` and the
capability are consistently emphasized (for example change the line to
"**`teams-do`** requires the `SYS_PTRACE` capability." or "**teams-do** requires
the **`SYS_PTRACE`** capability.") — update the text that mentions teams-do and
SYS_PTRACE so the asterisks/backticks are properly paired.
In `@helm/fiftyone-teams-app/templates/NOTES.txt`:
- Around line 7-14: The NOTES.txt warning currently only shows when
telemetry.redis.persistence.existingClaim is empty, but if
telemetry.redis.persistence.enabled is false the Redis volume is still ephemeral
even when existingClaim is set; update the Helm template condition to trigger
the warning whenever .Values.telemetry.enabled is true,
.Values.telemetry.redis.external.url is empty, and
.Values.telemetry.redis.persistence.enabled is false (i.e. remove the extra
check against .Values.telemetry.redis.persistence.existingClaim), so replace the
conditional using .Values.telemetry.redis.persistence.enabled/ existingClaim
with one that only checks (not .Values.telemetry.redis.persistence.enabled)
alongside the other predicates and keep the same warning text in the NOTES
block.
In `@tests/unit/helm/telemetry-redis_test.go`:
- Around line 139-147: The test currently only verifies
FIFTYONE_TELEMETRY_REDIS_URL when it is found; update the loop over
deployment.Spec.Template.Spec.Containers and container.Env to assert the env var
exists per container before comparing its value: for each container, set a found
flag when ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL", after scanning
container.Env call s.True(found, "container %s must have
FIFTYONE_TELEMETRY_REDIS_URL", container.Name) and then assert ev.Value ==
expectedURL (or compare the stored value) to validate the value; reference the
container.Env, ev.Name, and FIFTYONE_TELEMETRY_REDIS_URL symbols to locate where
to add the presence check.
---
Nitpick comments:
In `@tests/unit/helm/delegated-operator-job-configmap_test.go`:
- Around line 698-726: Add a new test case in the testCases slice to cover
telemetry-disabled behavior: when "telemetry.enabled" is set to "false"
(alongside existing keys like
"delegatedOperatorJobTemplates.jobs.cpuDefault.unused"), assert expectVol: 0 and
expectMnt: 0 so the test verifies no automatic injection of the
"telemetry-socket" volume/mount; locate the testCases variable in
delegated-operator-job-configmap_test.go and add the new case (referencing the
existing field names expectVol, expectMnt, and keys under
delegatedOperatorJobTemplates) to prevent unconditional injection regressions.
In `@tests/unit/helm/telemetry-sidecar_test.go`:
- Around line 126-130: The test currently uses s.False to check
deployment.Spec.Template.Spec.ShareProcessNamespace allowing an explicit false;
change the assertion to require the pointer be nil to enforce the field is unset
when telemetry is disabled. Locate the assertion referencing
deployment.Spec.Template.Spec.ShareProcessNamespace in telemetry-sidecar_test.go
and replace the s.False check with s.Nil on
deployment.Spec.Template.Spec.ShareProcessNamespace (keeping the same failure
message/context) so the test fails if the field is explicitly set to true or
false instead of being omitted.
- Around line 183-194: The test currently only checks presence/absence of
"SYS_PTRACE" by iterating over sc.Capabilities.Add (using hasPtrace) which
allows extra capabilities to be added unnoticed; update the assertion to compare
sc.Capabilities.Add exactly against the expected slice for each case (when
tc.executor true assert sc.Capabilities.Add equals the exact expected list
including "SYS_PTRACE", when tc.executor false assert it equals the exact
expected list without "SYS_PTRACE"), using the test helper/assert method already
in use so the test fails if any unexpected capability is present.
🪄 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: 109c1fc3-63fb-4735-baa0-9aecf1d621a2
⛔ 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.
Replace invalid do-1 profile example.
COMPOSE_PROFILES=do-1,gpu conflicts with the slot model documented elsewhere (slot 1 is always on; profiles start at do-2). This example can lead users to set a non-existent profile. Use gpu alone (or do-2,gpu / do-3,gpu when adding extra CPU worker slots).
Suggested doc fix
- worker profile (e.g. `COMPOSE_PROFILES=do-1,gpu`); see
+ worker profile (e.g. `COMPOSE_PROFILES=gpu`; or `COMPOSE_PROFILES=do-2,gpu`
+ to add a second CPU worker); 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`; or `COMPOSE_PROFILES=do-2,gpu` | |
| to add a second CPU worker); 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 "COMPOSE_PROFILES=do-1,gpu" is invalid because profiles
start at do-2; update the inline example in the text that currently reads
"worker profile (e.g. `COMPOSE_PROFILES=do-1,gpu`)" to use a valid profile
example such as "`COMPOSE_PROFILES=gpu`" (or show alternatives
"`COMPOSE_PROFILES=do-2,gpu`" / "`COMPOSE_PROFILES=do-3,gpu`" when describing
adding extra CPU worker slots) so readers aren't guided to a non-existent
profile.
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 Markdown emphasis in capability requirement.
The emphasis markers are unbalanced around SYS_PTRACE, which may render incorrectly in docs.
Suggested doc fix
-1. **`teams-do` requires the **`SYS_PTRACE`** capability to allow the
+1. **`teams-do` requires the `SYS_PTRACE` capability** to allow the📝 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
| 1. **`teams-do` requires the **`SYS_PTRACE`** capability to allow the | |
| telemetry agent to observe the target process. | |
| 1. **`teams-do` requires the `SYS_PTRACE` capability** to allow the | |
| telemetry agent to observe the target process. |
🤖 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, Fix the malformed Markdown
emphasis by balancing the bold markers around the service name and capability:
ensure `teams-do` and the capability are consistently emphasized (for example
change the line to "**`teams-do`** requires the `SYS_PTRACE` capability." or
"**teams-do** requires the **`SYS_PTRACE`** capability.") — update the text that
mentions teams-do and SYS_PTRACE so the asterisks/backticks are properly paired.
Comment on lines
+7
to
+14
| {{- if and .Values.telemetry.enabled (not .Values.telemetry.redis.external.url) (not .Values.telemetry.redis.persistence.enabled) (not .Values.telemetry.redis.persistence.existingClaim) }} | ||
| [INFO] Telemetry Redis is running on an emptyDir volume (no PersistentVolumeClaim). | ||
| Long-term telemetry archives in MongoDB are unaffected, but the in-window | ||
| dashboard backscroll (~10 minutes of un-archived data) is lost if the | ||
| Redis pod restarts. To make telemetry survive Redis restarts, set | ||
| `telemetry.redis.persistence.enabled: true` (needs a working StorageClass | ||
| or `telemetry.redis.persistence.existingClaim` pointing at a pre-created PVC). | ||
| {{ end }} |
Contributor
There was a problem hiding this comment.
Warn on effective emptyDir even when existingClaim is set.
At Line 7, the condition hides this warning when telemetry.redis.persistence.existingClaim is non-empty. But with telemetry.redis.persistence.enabled=false, existingClaim is ignored and Redis is still ephemeral, so operators can miss the data-loss warning.
Suggested 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) }}
[INFO] Telemetry Redis is running on an emptyDir volume (no PersistentVolumeClaim).
Long-term telemetry archives in MongoDB are unaffected, but the in-window
dashboard backscroll (~10 minutes of un-archived data) is lost if the
Redis pod restarts. To make telemetry survive Redis restarts, set
`telemetry.redis.persistence.enabled: true` (needs a working StorageClass
- or `telemetry.redis.persistence.existingClaim` pointing at a pre-created PVC).
+ or `telemetry.redis.persistence.existingClaim` pointing at a pre-created PVC).
{{ end }}🧰 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` around lines 7 - 14, The
NOTES.txt warning currently only shows when
telemetry.redis.persistence.existingClaim is empty, but if
telemetry.redis.persistence.enabled is false the Redis volume is still ephemeral
even when existingClaim is set; update the Helm template condition to trigger
the warning whenever .Values.telemetry.enabled is true,
.Values.telemetry.redis.external.url is empty, and
.Values.telemetry.redis.persistence.enabled is false (i.e. remove the extra
check against .Values.telemetry.redis.persistence.existingClaim), so replace the
conditional using .Values.telemetry.redis.persistence.enabled/ existingClaim
with one that only checks (not .Values.telemetry.redis.persistence.enabled)
alongside the other predicates and keep the same warning text in the NOTES
block.
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 each container before validating value.
At Line 139, this test only checks equality when FIFTYONE_TELEMETRY_REDIS_URL is found. If the env var is missing, the test still passes silently.
Suggested fix
for _, container := range deployment.Spec.Template.Spec.Containers {
+ found := false
for _, ev := range container.Env {
if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
+ found = true
s.Equal(expectedURL, ev.Value,
"FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
container.Name)
+ break
}
}
+ s.Require().True(found,
+ "FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", 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 verifies FIFTYONE_TELEMETRY_REDIS_URL when it is found; update
the loop over deployment.Spec.Template.Spec.Containers and container.Env to
assert the env var exists per container before comparing its value: for each
container, set a found flag when ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL",
after scanning container.Env call s.True(found, "container %s must have
FIFTYONE_TELEMETRY_REDIS_URL", container.Name) and then assert ev.Value ==
expectedURL (or compare the stored value) to validate the value; reference the
container.Env, ev.Name, and FIFTYONE_TELEMETRY_REDIS_URL symbols to locate where
to add the presence check.
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
4 participants
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