Release/v2.19.0#577
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…v2.19.0-rc.0 api:v2.19.0rc1 app:v2.19.0rc1 cas:v2.19.0-rc.0
chore: bump release 2.19.0
…v2.19.0-rc.1 api:v2.19.0rc2 app:v2.19.0rc2 cas:v2.19.0-rc.1
…v2.19.0-rc.2 api:v2.19.0rc3 app:v2.19.0rc3 cas:v2.19.0-rc.2
…v2.19.0-rc.3 api:v2.19.0rc4 app:v2.19.0rc4 cas:v2.19.0-rc.3
…v2.19.0-rc.4 api:v2.19.0rc5 app:v2.19.0rc5 cas:v2.19.0-rc.4
chore: fix helm.fiftyone.ai table
Enables the new observability features released in FiftyOne Enterprise 2.19.0, including enhanced process metrics, real-time logs, and more, viewable directly within the UI.
Changes:
* Helm
* Adds a sidecar container to the following pods:
* fiftyone-app
* teams-api
* plugins
* do deployments
* do jobs (as a native sidecar)
* Adds a role and rolebinding for telemetry
* Adds get pods/log permission to api role
* Adds an optional redis deployment and service, with a further optional chart-managed pvc, or a further optional existing PVC by name
* Compose
* Drops support for `FIFTYONE_DELEGATED_OPERATOR_WORKER_REPLICAS` config option, replacing it with `COMPOSE_PROFILES="do-3"`
* Adds sidecar containers to same services as Helm
* Adds a tmpfs volume for the telemetry socket
* Scripts: Adds support for the new telemetry-sidecar image
---------
Signed-off-by: Alan Smith <209585+mo-getter@users.noreply.github.com>
Signed-off-by: Kacey <kacey@voxel51.com>
Co-authored-by: kacey <kacey@voxel51.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com>
…v2.19.0-rc.18 api:v2.19.0rc19 app:v2.19.0rc19 cas:v2.19.0-rc.18
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:
WalkthroughVersion 2.19.0 release adds bundled telemetry sidecars with Redis backend across Docker Compose and Helm deployments. Updates include new telemetry environment variables, pod security context defaults (UID/GID 1000), Compose profile-based delegated-operator worker scaling, comprehensive documentation, and extensive test coverage for telemetry injection and configuration. ChangesFiftyOne Enterprise v2.19.0 Telemetry & Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 9
🧹 Nitpick comments (2)
tests/unit/helm/delegated-operator-job-configmap_test.go (1)
714-725: ⚡ Quick winAssert mount path preservation in user-declared telemetry socket case.
The dedup test only checks count. It should also verify the existing custom mount path (
/custom/path) is preserved, not replaced during injection.Suggested assertion addition
{ name: "doesNotDuplicateWhenUserDeclaresTelemetrySocket", values: map[string]string{ "telemetry.enabled": "true", "delegatedOperatorJobTemplates.jobs.cpuDefault.unused": "nil", "delegatedOperatorJobTemplates.jobs.cpuDefault.volumes[0].name": socketName, "delegatedOperatorJobTemplates.jobs.cpuDefault.volumes[0].emptyDir.medium": "Memory", "delegatedOperatorJobTemplates.jobs.cpuDefault.volumeMounts[0].name": socketName, "delegatedOperatorJobTemplates.jobs.cpuDefault.volumeMounts[0].mountPath": "/custom/path", }, expectVol: 1, expectMnt: 1, }, @@ s.Equal( testCase.expectMnt, countMountsByName(job.Spec.Template.Spec.Containers[0].VolumeMounts, socketName), "telemetry-socket volumeMount count mismatch", ) + if testCase.name == "doesNotDuplicateWhenUserDeclaresTelemetrySocket" { + found := false + for _, m := range job.Spec.Template.Spec.Containers[0].VolumeMounts { + if m.Name == socketName { + s.Equal("/custom/path", m.MountPath, "existing telemetry mountPath should be preserved") + found = true + } + } + s.True(found, "expected telemetry-socket mount to exist") + } }) } }Also applies to: 748-752
🤖 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 714 - 725, In the "doesNotDuplicateWhenUserDeclaresTelemetrySocket" test, add an assertion that the user-provided volumeMount path "/custom/path" is preserved (i.e., the injected telemetry mount did not override it) by inspecting the rendered job's volumeMounts for the entry named socketName and asserting mountPath == "/custom/path" in addition to the existing expectVol/expectMnt count checks; make the same mount-path-preservation assertion for the other similar test case referenced in the comment (the neighboring dedup test around the other case).tests/unit/helm/plugins-deployment_test.go (1)
2525-2528: ⚡ Quick winAdd nil guards before dereferencing PodSecurityContext fields.
These direct dereferences can panic and make regressions harder to diagnose. Guard the pointer fields first.
Proposed test-hardening patch
func(podSecurityContext *corev1.PodSecurityContext) { + s.Require().NotNil(podSecurityContext, "podSecurityContext should be set") + s.Require().NotNil(podSecurityContext.FSGroup, "fsGroup should be set") + s.Require().NotNil(podSecurityContext.RunAsGroup, "runAsGroup should be set") + s.Require().NotNil(podSecurityContext.RunAsNonRoot, "runAsNonRoot should be set") + s.Require().NotNil(podSecurityContext.RunAsUser, "runAsUser should be set") s.Equal(int64(1000), *podSecurityContext.FSGroup, "fsGroup should be 1000 (image UID)") s.Equal(int64(1000), *podSecurityContext.RunAsGroup, "runAsGroup should be 1000") s.True(*podSecurityContext.RunAsNonRoot, "runAsNonRoot should be true") s.Equal(int64(1000), *podSecurityContext.RunAsUser, "runAsUser should be 1000")🤖 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/plugins-deployment_test.go` around lines 2525 - 2528, The assertions currently dereference podSecurityContext fields directly (FSGroup, RunAsGroup, RunAsNonRoot, RunAsUser) which can panic; add nil guards by first asserting podSecurityContext is not nil (e.g., s.NotNil(podSecurityContext)) and then assert each pointer field is not nil (s.NotNil(podSecurityContext.FSGroup), etc.) before dereferencing, and then perform the existing equality/true checks to keep test semantics intact.
🤖 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-delegated-operators.md`:
- Line 43: The sentence "If you used on the <2.19 default of 3 workers, set
`COMPOSE_PROFILES=do-3`." contains a typo and awkward phrasing; replace it with
the suggested wording "If you were using the pre-2.19 default of 3 workers, set
`COMPOSE_PROFILES=do-3`." Update that exact sentence in the documentation to
improve clarity.
In `@docker/docs/configuring-gpu-workloads.md`:
- Around line 57-58: Update the example that sets COMPOSE_PROFILES so it follows
the delegated-operator profile semantics: replace the incorrect
`COMPOSE_PROFILES=do-1,gpu` with `COMPOSE_PROFILES=gpu` (or
`COMPOSE_PROFILES=do-2,gpu` / `COMPOSE_PROFILES=do-3,gpu` when showing how to
scale extra workers). Ensure any mention of slot 1 being optional is removed and
that examples reference `do-2`/`do-3` for additional worker slots and `gpu` for
the GPU profile.
In `@docker/docs/configuring-telemetry.md`:
- Around line 143-148: Add the missing compose.override.yaml snippet to the "To
run without telemetry" opt-out section by including a services block that sets
deploy.replicas: 0 for each telemetry service (refer to service names
telemetry-redis, fiftyone-app-telemetry, teams-api-telemetry,
teams-plugins-telemetry, teams-do-telemetry, teams-do-2-telemetry,
teams-do-3-telemetry, teams-do-gpu-telemetry) so users can scale telemetry to
zero, and fix the typo "Cpmpose" to "Compose" where it appears.
In `@docker/internal-auth/env.template`:
- Around line 97-107: Add the missing example `COMPOSE_PROFILES` line referenced
by the comment: insert a commented and an uncommented example showing how to
enable delegated-operator slots (e.g., `COMPOSE_PROFILES="do-2"` or
`COMPOSE_PROFILES="do-3"`) near the existing explanatory block so users can
copy/paste to enable `do-2`/`do-3`; reference the environment variable name
COMPOSE_PROFILES and ensure the examples mirror the comment text by showing both
single and multi-slot profiles (e.g., `do-2` and `do-3`) and note that it must
not exceed the license max.
In `@helm/docs/upgrading.md`:
- Around line 173-181: Update the paragraph describing the delegated-operator
sidecar to remove the incorrect claim that the configuration is "compatible with
Pod Security Admission `restricted` out of the box"; instead explicitly state
that the delegated-operator sidecar requires the `SYS_PTRACE` capability (which
is prohibited by PSA `restricted`) and therefore clusters must either allow
`SYS_PTRACE` for that workload/namespace via a policy exception or disable
telemetry by setting `telemetry.enabled: false`. Mention `delegated-operator`
and `telemetry.enabled` by name so readers know which sidecar and flag are
affected, and remove the existing wording implying automatic `restricted`
compatibility.
In `@helm/fiftyone-teams-app/templates/_telemetry.tpl`:
- Around line 146-152: The telemetry sidecar securityContext drops all
capabilities but conditionally adds SYS_PTRACE without ensuring a root UID,
which will fail if cluster defaults enforce non-root; update the telemetry
sidecar block in _telemetry.tpl (securityContext) so that when adding
["SYS_PTRACE"] (the conditional around .executor) you also set runAsUser: 0 and
runAsNonRoot: false (i.e., make these keys conditional alongside the add:
["SYS_PTRACE"] clause) so the sidecar runs as root only when ptrace is required.
In `@tests/unit/helm/delegated-operator-instance-deployment_test.go`:
- Around line 3655-3658: The test directly dereferences fields on
podSecurityContext (FSGroup, RunAsGroup, RunAsNonRoot, RunAsUser) which can
panic if nil; update the assertions in the test function to first assert
s.NotNil(podSecurityContext) and then s.NotNil for each pointer field
(podSecurityContext.FSGroup, podSecurityContext.RunAsGroup,
podSecurityContext.RunAsNonRoot, podSecurityContext.RunAsUser) before calling
s.Equal/ s.True on their dereferenced values so failures report clearly instead
of causing a panic.
In `@tests/unit/helm/telemetry-redis_test.go`:
- Around line 139-147: The test currently only asserts the value when
FIFTYONE_TELEMETRY_REDIS_URL is found, so a missing env var would not fail;
update the loop over deployment.Spec.Template.Spec.Containers in
telemetry-redis_test.go to assert presence of the env var for each container
(same pattern as TestExternalUrlWiresApiDeployment) by checking container.Env
contains an entry with Name == "FIFTYONE_TELEMETRY_REDIS_URL" (use s.True or
s.Require().True) and then assert that entry's Value equals expectedURL,
referencing the container.Name in the failure messages.
In `@utils/validate-docker-pulls.sh`:
- Around line 17-18: The script validate-docker-pulls.sh currently appends the
chart appVersion to voxel51 images and has no way to override that, causing
mismatches with RC tags; add a CLI flag (e.g., --expected-tag) and/or
environment variable (e.g., EXPECTED_IMAGE_TAG) parsing at the top of
validate-docker-pulls.sh and use that value when building the expected image
name instead of always using the chart APP_VERSION; update the logic that
constructs expected image strings (references to where appVersion/APP_VERSION is
used) to prefer EXPECTED_IMAGE_TAG if set (or the CLI flag) and fall back to the
existing behavior otherwise, and include usage/help text for the new option.
---
Nitpick comments:
In `@tests/unit/helm/delegated-operator-job-configmap_test.go`:
- Around line 714-725: In the "doesNotDuplicateWhenUserDeclaresTelemetrySocket"
test, add an assertion that the user-provided volumeMount path "/custom/path" is
preserved (i.e., the injected telemetry mount did not override it) by inspecting
the rendered job's volumeMounts for the entry named socketName and asserting
mountPath == "/custom/path" in addition to the existing expectVol/expectMnt
count checks; make the same mount-path-preservation assertion for the other
similar test case referenced in the comment (the neighboring dedup test around
the other case).
In `@tests/unit/helm/plugins-deployment_test.go`:
- Around line 2525-2528: The assertions currently dereference podSecurityContext
fields directly (FSGroup, RunAsGroup, RunAsNonRoot, RunAsUser) which can panic;
add nil guards by first asserting podSecurityContext is not nil (e.g.,
s.NotNil(podSecurityContext)) and then assert each pointer field is not nil
(s.NotNil(podSecurityContext.FSGroup), etc.) before dereferencing, and then
perform the existing equality/true checks to keep test semantics intact.
🪄 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: 68922226-e655-451e-8eb0-ace3fb9029b6
⛔ Files ignored due to path filters (35)
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/Chart.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!**/*.yamlhelm/values.yamlis excluded by!**/*.yamlskaffold.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 (40)
Makefiledocker/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/custom-plugins.mddocs/orchestrators/configuring-databricks-orchestrator.mddocs/orchestrators/configuring-kubernetes-orchestrator.mdhelm/docs/upgrading.mdhelm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/README.md.gotmplhelm/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/.envtests/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/common_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
💤 Files with no reviewable changes (1)
- tests/fixtures/docker/.env
| > profiles (`do-2`, `do-3`). Set `COMPOSE_PROFILES=do-N` to add slots | ||
| > up to N (cap of 3). This replaces the deprecated | ||
| > `FIFTYONE_DELEGATED_OPERATOR_WORKER_REPLICAS` environment variable. | ||
| > If you used on the <2.19 default of 3 workers, set `COMPOSE_PROFILES=do-3`. |
Contributor
There was a problem hiding this comment.
Fix typo in upgrade guidance sentence.
Line 43 reads awkwardly (If you used on the <2.19 default...). Suggested wording: If you were using the pre-2.19 default of 3 workers, set COMPOSE_PROFILES=do-3.
🤖 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-delegated-operators.md` at line 43, The sentence "If
you used on the <2.19 default of 3 workers, set `COMPOSE_PROFILES=do-3`."
contains a typo and awkward phrasing; replace it with the suggested wording "If
you were using the pre-2.19 default of 3 workers, set `COMPOSE_PROFILES=do-3`."
Update that exact sentence in the documentation to improve clarity.
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.
Fix profile example to match delegated-operator profile semantics.
Line 57 uses COMPOSE_PROFILES=do-1,gpu, but this conflicts with the delegated-operators doc update in this PR where slot 1 is always on and optional profiles are do-2/do-3. Use COMPOSE_PROFILES=gpu (or do-2,gpu / do-3,gpu when scaling extra workers).
🤖 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, Update the
example that sets COMPOSE_PROFILES so it follows the delegated-operator profile
semantics: replace the incorrect `COMPOSE_PROFILES=do-1,gpu` with
`COMPOSE_PROFILES=gpu` (or `COMPOSE_PROFILES=do-2,gpu` /
`COMPOSE_PROFILES=do-3,gpu` when showing how to scale extra workers). Ensure any
mention of slot 1 being optional is removed and that examples reference
`do-2`/`do-3` for additional worker slots and `gpu` for the GPU profile.
Comment on lines
+143
to
+148
| To run without telemetry | ||
|
|
||
| 1. Add a `compose.override.yaml` that scales the | ||
| telemetry services to zero replicas: | ||
|
|
||
| ### Scaling teams-do with telemetry |
Contributor
There was a problem hiding this comment.
Complete the “Opting out” instructions and fix the Compose typo
The section introduces an opt-out flow but stops before showing the actual compose.override.yaml example, then jumps to a different heading. Also Line 237 has “Cpmpose”. Please include the missing override snippet and correct the typo.
Suggested patch
@@
To run without telemetry
1. Add a `compose.override.yaml` that scales the
telemetry services to zero replicas:
+
+```yaml
+services:
+ telemetry-redis:
+ deploy:
+ replicas: 0
+ fiftyone-app-telemetry:
+ deploy:
+ replicas: 0
+ teams-api-telemetry:
+ deploy:
+ replicas: 0
+ teams-plugins-telemetry:
+ deploy:
+ replicas: 0
+ teams-do-telemetry:
+ deploy:
+ replicas: 0
+ teams-do-2-telemetry:
+ deploy:
+ replicas: 0
+ teams-do-3-telemetry:
+ deploy:
+ replicas: 0
+ teams-do-gpu-telemetry:
+ deploy:
+ replicas: 0
+```
@@
-in the Cpmpose files.
+in the Compose files.Also applies to: 237-237
🤖 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-telemetry.md` around lines 143 - 148, Add the missing
compose.override.yaml snippet to the "To run without telemetry" opt-out section
by including a services block that sets deploy.replicas: 0 for each telemetry
service (refer to service names telemetry-redis, fiftyone-app-telemetry,
teams-api-telemetry, teams-plugins-telemetry, teams-do-telemetry,
teams-do-2-telemetry, teams-do-3-telemetry, teams-do-gpu-telemetry) so users can
scale telemetry to zero, and fix the typo "Cpmpose" to "Compose" where it
appears.
Comment on lines
+97
to
+107
| # Delegated-operator worker count is selected via Compose profiles when | ||
| # the `compose.delegated-operators.yaml` overlay is in use. Slot 1 | ||
| # (`teams-do`) runs by default; uncomment the line below to add slots | ||
| # 2 and/or 3, each paired with its own telemetry sidecar (static cap | ||
| # of 3). `do-N` includes every lower slot, so `do-3` runs three workers. | ||
| # | ||
| # See docker/docs/configuring-telemetry.md | ||
| # and docker/docs/configuring-delegated-operators.md for details. The | ||
| # value must not exceed the max concurrent delegated operators set in | ||
| # your license file. | ||
|
|
Contributor
There was a problem hiding this comment.
Add the missing COMPOSE_PROFILES example line referenced by the comment.
The text says “uncomment the line below,” but no line is present. Add a concrete example so users can actually enable do-2/do-3.
Suggested doc fix
# Delegated-operator worker count is selected via Compose profiles when
# the `compose.delegated-operators.yaml` overlay is in use. Slot 1
# (`teams-do`) runs by default; uncomment the line below to add slots
# 2 and/or 3, each paired with its own telemetry sidecar (static cap
# of 3). `do-N` includes every lower slot, so `do-3` runs three workers.
+#
+# COMPOSE_PROFILES=do-2 # enables teams-do-2 (plus default teams-do)
+# COMPOSE_PROFILES=do-3 # enables teams-do-2 and teams-do-3 (plus default teams-do)
#
# See docker/docs/configuring-telemetry.md
# and docker/docs/configuring-delegated-operators.md for details. The
# value must not exceed the max concurrent delegated operators set in
# your license file.🤖 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/internal-auth/env.template` around lines 97 - 107, Add the missing
example `COMPOSE_PROFILES` line referenced by the comment: insert a commented
and an uncommented example showing how to enable delegated-operator slots (e.g.,
`COMPOSE_PROFILES="do-2"` or `COMPOSE_PROFILES="do-3"`) near the existing
explanatory block so users can copy/paste to enable `do-2`/`do-3`; reference the
environment variable name COMPOSE_PROFILES and ensure the examples mirror the
comment text by showing both single and multi-slot profiles (e.g., `do-2` and
`do-3`) and note that it must not exceed the license max.
Comment on lines
+173
to
+181
| user) with `cap_drop: ALL`. The delegated-operator sidecar | ||
| additionally has `SYS_PTRACE` added (required for `py-spy` stack | ||
| sampling); the other three sidecars run with no capabilities. This | ||
| posture is compatible with | ||
| [Pod Security Admission][psa] `restricted` out of the box. | ||
| Clusters enforcing additional admission policies (OPA/Gatekeeper, | ||
| Kyverno) that explicitly block `SYS_PTRACE` on the | ||
| delegated-operator namespace can either allow that single capability | ||
| or disable telemetry with `telemetry.enabled: false`. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes Pod Security Standards "restricted", which Linux capabilities are allowed in container securityContext.capabilities.add? Is SYS_PTRACE allowed?
💡 Result:
In the Kubernetes "restricted" Pod Security Standard, the configuration for container capabilities is highly restrictive [1][2]. Under this policy, containers must drop all capabilities, and the only capability that is explicitly permitted to be added back is NET_BIND_SERVICE [1][2]. SYS_PTRACE is not allowed under the restricted Pod Security Standard [1][2]. Any attempt to add SYS_PTRACE or any other capability (other than NET_BIND_SERVICE) via spec.containers[*].securityContext.capabilities.add will violate the restricted policy and result in the Pod being rejected by the Pod Security Admission controller if it is set to enforce mode [1][3].
Citations:
- 1: https://kubernetes.io/docs/concepts/security/pod-security-standards/
- 2: https://www.armosec.io/glossary/pod-security-standards/
- 3: https://www.apptio.com/topics/kubernetes/best-practices/pod-security-admission/
🌐 Web query:
Kubernetes Pod Security Standards restricted seccomp capabilities add allowed NET_BIND_SERVICE SYS_PTRACE container securityContext.capabilities.add
💡 Result:
Under the Kubernetes Restricted Pod Security Standard, adding capabilities beyond those explicitly permitted is prohibited [1][2]. Specifically, the Restricted policy mandates that containers must drop all capabilities and are only permitted to add back the NET_BIND_SERVICE capability [1][3]. Consequently, adding NET_BIND_SERVICE via container.securityContext.capabilities.add is compliant with the Restricted standard [1][4], while adding SYS_PTRACE is not allowed and will cause a violation [1][2]. Regarding seccomp, the Restricted policy requires that the seccomp profile must not be set to Unconfined [1][2]. It must be explicitly set to either RuntimeDefault or Localhost [1][2]. If a container's seccomp profile is not explicitly defined, it may inherit the pod-level profile, but the final configuration must conform to these restricted values [1][5]. In summary: 1. NET_BIND_SERVICE: Allowed to be added [1][2]. 2. SYS_PTRACE: Prohibited from being added [1][2]. 3. Seccomp: Must be RuntimeDefault or Localhost; Unconfined is prohibited [1][5][2].
Citations:
- 1: https://kubernetes.io/docs/concepts/security/pod-security-standards/
- 2: https://www.armosec.io/glossary/pod-security-standards/
- 3: https://kubernetes.website.cncfstack.com/docs/concepts/security/pod-security-standards/
- 4: https://www.dynatrace.com/news/blog/kubernetes-security-best-practices-security-context/
- 5: https://kubernetes.io/docs/reference/node/seccomp/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '150,210p' helm/docs/upgrading.md | nl -ba | sed -n '1,120p'Repository: voxel51/fiftyone-teams-app-deploy
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="helm/docs/upgrading.md"
if [ ! -f "$f" ]; then
echo "Missing file: $f"
exit 1
fi
# Print lines 160-195 with line numbers using awk (nl not available)
awk 'NR>=160 && NR<=195 {printf "%d:%s\n", NR, $0}' "$f"Repository: voxel51/fiftyone-teams-app-deploy
Length of output: 1751
Fix PSA “restricted” compatibility claim: SYS_PTRACE is not allowed
helm/docs/upgrading.mdsays the delegated-operator sidecar addsSYS_PTRACE, but also claims the posture is compatible with PSArestricted“out of the box”; Kubernetes “restricted” only permits addingNET_BIND_SERVICEand prohibitsSYS_PTRACE, so enforcing restricted will reject the pod.- Reword to state compatibility requires either a policy exception allowing
SYS_PTRACEfor that workload/namespace or disabling telemetry (telemetry.enabled: false).
🤖 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/docs/upgrading.md` around lines 173 - 181, Update the paragraph
describing the delegated-operator sidecar to remove the incorrect claim that the
configuration is "compatible with Pod Security Admission `restricted` out of the
box"; instead explicitly state that the delegated-operator sidecar requires the
`SYS_PTRACE` capability (which is prohibited by PSA `restricted`) and therefore
clusters must either allow `SYS_PTRACE` for that workload/namespace via a policy
exception or disable telemetry by setting `telemetry.enabled: false`. Mention
`delegated-operator` and `telemetry.enabled` by name so readers know which
sidecar and flag are affected, and remove the existing wording implying
automatic `restricted` compatibility.
Comment on lines
+146
to
+152
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: ["ALL"] | ||
| {{- if .executor }} | ||
| add: ["SYS_PTRACE"] | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
# Verify whether pod-level non-root defaults coexist with sidecar templates lacking runAsUser override.
# Expected: find pod non-root defaults and confirm telemetry sidecar securityContext has no runAsUser/runAsNonRoot lines.
set -euo pipefail
echo "== Files setting pod-level non-root defaults =="
rg -n -C2 'runAsNonRoot|runAsUser|podSecurityContext|securityContext' helm/fiftyone-teams-app/templates
echo
echo "== telemetry.sidecar and telemetry.native-sidecar securityContext blocks =="
rg -n -C4 'define "telemetry\.sidecar"|define "telemetry\.native-sidecar"|securityContext:|runAsNonRoot|runAsUser|SYS_PTRACE' helm/fiftyone-teams-app/templates/_telemetry.tplRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 2091
Fix telemetry sidecar securityContext to handle non-root defaults when adding SYS_PTRACE
helm/fiftyone-teams-app/templates/_telemetry.tpl’s telemetry sidecar securityContext drops all capabilities and conditionally adds SYS_PTRACE, but it lacks explicit runAsNonRoot: false / runAsUser: 0. If the chart/pod security defaults enforce non-root, the sidecar may fail to start or access required /proc/ptrace capabilities.
Suggested fix
securityContext:
allowPrivilegeEscalation: false
+ runAsNonRoot: false
+ runAsUser: 0
capabilities:
drop: ["ALL"]
{{- if .executor }}
add: ["SYS_PTRACE"]
{{- end }}
@@
securityContext:
allowPrivilegeEscalation: false
+ runAsNonRoot: false
+ runAsUser: 0
capabilities:
drop: ["ALL"]
{{- if .executor }}
add: ["SYS_PTRACE"]
{{- end }}📝 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
| securityContext: | |
| allowPrivilegeEscalation: false | |
| capabilities: | |
| drop: ["ALL"] | |
| {{- if .executor }} | |
| add: ["SYS_PTRACE"] | |
| {{- end }} | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: false | |
| runAsUser: 0 | |
| capabilities: | |
| drop: ["ALL"] | |
| {{- if .executor }} | |
| add: ["SYS_PTRACE"] | |
| {{- end }} |
🤖 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/_telemetry.tpl` around lines 146 - 152, The
telemetry sidecar securityContext drops all capabilities but conditionally adds
SYS_PTRACE without ensuring a root UID, which will fail if cluster defaults
enforce non-root; update the telemetry sidecar block in _telemetry.tpl
(securityContext) so that when adding ["SYS_PTRACE"] (the conditional around
.executor) you also set runAsUser: 0 and runAsNonRoot: false (i.e., make these
keys conditional alongside the add: ["SYS_PTRACE"] clause) so the sidecar runs
as root only when ptrace is required.
Comment on lines
+3655
to
+3658
| s.Equal(int64(1000), *podSecurityContext.FSGroup, "fsGroup should be 1000 (image UID)") | ||
| s.Equal(int64(1000), *podSecurityContext.RunAsGroup, "runAsGroup should be 1000") | ||
| s.True(*podSecurityContext.RunAsNonRoot, "runAsNonRoot should be true") | ||
| s.Equal(int64(1000), *podSecurityContext.RunAsUser, "runAsUser should be 1000") |
Contributor
There was a problem hiding this comment.
Guard pointer dereferences in PodSecurityContext assertions.
At Line 3655 and Line 3675 blocks, direct pointer dereferences can panic before assertion reporting if the field is unexpectedly nil. Add explicit NotNil checks first for clearer failures.
Suggested patch
func(podSecurityContext *corev1.PodSecurityContext) {
+ s.Require().NotNil(podSecurityContext.FSGroup, "fsGroup should be set")
+ s.Require().NotNil(podSecurityContext.RunAsGroup, "runAsGroup should be set")
+ s.Require().NotNil(podSecurityContext.RunAsNonRoot, "runAsNonRoot should be set")
+ s.Require().NotNil(podSecurityContext.RunAsUser, "runAsUser should be set")
s.Equal(int64(1000), *podSecurityContext.FSGroup, "fsGroup should be 1000 (image UID)")
s.Equal(int64(1000), *podSecurityContext.RunAsGroup, "runAsGroup should be 1000")
s.True(*podSecurityContext.RunAsNonRoot, "runAsNonRoot should be true")
s.Equal(int64(1000), *podSecurityContext.RunAsUser, "runAsUser should be 1000")
...
}Also applies to: 3675-3678
🤖 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
3655 - 3658, The test directly dereferences fields on podSecurityContext
(FSGroup, RunAsGroup, RunAsNonRoot, RunAsUser) which can panic if nil; update
the assertions in the test function to first assert s.NotNil(podSecurityContext)
and then s.NotNil for each pointer field (podSecurityContext.FSGroup,
podSecurityContext.RunAsGroup, podSecurityContext.RunAsNonRoot,
podSecurityContext.RunAsUser) before calling s.Equal/ s.True on their
dereferenced values so failures report clearly instead of causing a panic.
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 path
On Line 139, this loop validates value only when FIFTYONE_TELEMETRY_REDIS_URL exists, so the test can pass even if the env var is missing entirely on a container. Add an explicit presence assertion per container (same pattern used in TestExternalUrlWiresApiDeployment).
Suggested patch
for _, container := range deployment.Spec.Template.Spec.Containers {
- for _, ev := range container.Env {
+ 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.True(found, "FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", 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 { | |
| 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.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 asserts the value when FIFTYONE_TELEMETRY_REDIS_URL is found, so
a missing env var would not fail; update the loop over
deployment.Spec.Template.Spec.Containers in telemetry-redis_test.go to assert
presence of the env var for each container (same pattern as
TestExternalUrlWiresApiDeployment) by checking container.Env contains an entry
with Name == "FIFTYONE_TELEMETRY_REDIS_URL" (use s.True or s.Require().True) and
then assert that entry's Value equals expectedURL, referencing the
container.Name in the failure messages.
Comment on lines
+17
to
+18
| # Images with an explicit `:tag` are taken as-is; voxel51/ images without | ||
| # a tag get the chart appVersion appended automatically. |
Contributor
There was a problem hiding this comment.
Make expected image tag overridable for prerelease validation
The current “append chart appVersion” behavior is causing this job to validate voxel51/*:v2.19.0, but this PR’s fixtures are still on RC image tags (v2.19.0rc19 / v2.19.0-rc.18). That mismatch is what the failing pipeline output shows. Please add an override path (CLI flag/env var) so CI can validate against RC tags before GA images exist.
Suggested patch
@@
-expected_tag=$(yq '.appVersion' "${GIT_ROOT}/helm/fiftyone-teams-app/Chart.yaml")
+expected_tag="${EXPECTED_IMAGE_TAG:-$(yq '.appVersion' "${GIT_ROOT}/helm/fiftyone-teams-app/Chart.yaml")}"@@ print_usage() {
- echo "-f, --values VALUES_YAML values.yaml to use in templating. Defaults to ${VALUES_YAML}"
+ echo "-f, --values VALUES_YAML values.yaml to use in templating. Defaults to ${VALUES_YAML}"
+ echo "-t, --expected-tag TAG override appended tag for untagged voxel51/* images"
@@ parse_arguments() {
-f | --values)
@@
shift 2
;;
+ -t | --expected-tag)
+ EXPECTED_IMAGE_TAG="${2-}"
+ if [[ -z "${EXPECTED_IMAGE_TAG}" ]]; then
+ log_error "--expected-tag requires a non-empty value"
+ exit 1
+ fi
+ shift 2
+ ;;🤖 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 `@utils/validate-docker-pulls.sh` around lines 17 - 18, The script
validate-docker-pulls.sh currently appends the chart appVersion to voxel51
images and has no way to override that, causing mismatches with RC tags; add a
CLI flag (e.g., --expected-tag) and/or environment variable (e.g.,
EXPECTED_IMAGE_TAG) parsing at the top of validate-docker-pulls.sh and use that
value when building the expected image name instead of always using the chart
APP_VERSION; update the logic that constructs expected image strings (references
to where appVersion/APP_VERSION is used) to prefer EXPECTED_IMAGE_TAG if set (or
the CLI flag) and fall back to the existing behavior otherwise, and include
usage/help text for the new option.
mo-getter
previously approved these changes
May 28, 2026
…v2.19.0 api:v2.19.0 app:v2.19.0 cas:v2.19.0
mo-getter
approved these changes
May 28, 2026
This was referenced May 29, 2026
Merged
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
release v2.19.0
Review Priority
Changes
Checklist
Testing
n/a