Skip to content
Merged
2 changes: 1 addition & 1 deletion .github/workflows/pre-commit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Temporary SQLite/LZMA - Install missing libraries
run: sudo apt install -y libsqlite3-dev libbz2-dev
- name: Install asdf & tools
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-docker-pulls.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Checkout voxel51/aloha-github-workflows
uses: actions/checkout@v7
with:
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/test-docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Install asdf & tools
uses: ./.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache
- name: Run unit tests
Expand Down Expand Up @@ -66,7 +66,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Temporary SQLite/LZMA - Install missing libraries
run: sudo apt install -y libsqlite3-dev libbz2-dev
- name: Install asdf & tools
Expand Down Expand Up @@ -123,7 +123,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Install asdf & tools
uses: ./.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache
- name: Authenticate to Google Cloud
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-helm.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Temporary SQLite/LZMA - Install missing libraries
run: sudo apt install -y libsqlite3-dev libbz2-dev
- name: Install asdf & tools
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test-integration-helm.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
sed -i \
's%voxel51/aloha-github-workflows/.github/actions/generate-version-file@main%./.github/actions/aloha-github-workflows/.github/actions/generate-version-file%' \
.github/actions/aloha-github-workflows/.github/actions/asdf-install-and-cache/action.yml
- uses: actions/setup-python@v6.2.0
- uses: actions/setup-python@v6.3.0
- name: Temporary SQLite/LZMA - Install missing libraries
run: sudo apt install -y libsqlite3-dev libbz2-dev
- name: Install asdf & tools
Expand Down
6 changes: 6 additions & 0 deletions helm/fiftyone-teams-app/templates/_telemetry.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ Inputs (dict):
podName — value for POD_NAME env var (defaults to fieldRef metadata.name)
executor — bool, when true emit EXECUTOR_SIDECAR=true and TELEMETRY_SOCKET env
targetContainer — when set, emit TARGET_CONTAINER env var (used by job sidecars)
sidecarEnv — optional map of extra env vars to append (e.g. NVIDIA_* so a
sidecar on a GPU node can read NVML/GPU metrics)
*/}}
{{- define "telemetry.sidecar-env" -}}
{{- $secretName := .ctx.Values.secret.name -}}
Expand Down Expand Up @@ -127,6 +129,10 @@ Inputs (dict):
secretKeyRef:
name: {{ $secretName }}
key: fiftyoneDatabaseName
{{- range $name, $value := .sidecarEnv }}
- name: {{ $name }}
value: {{ $value | quote }}
{{- end }}
{{- end }}

{{/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@
}}
{{- $defaultDescription := printf "Long running operations delegated to %s" $name }}
{{- $updateStrategy := merge (dict) ($v.updateStrategy|default dict) ($baseTpl.updateStrategy) }}
{{- $mergedResources := merge (dict) ($v.resources | default dict) ($baseTpl.resources | default dict) }}
{{- /* When the executor requests a GPU, expose that same GPU to its telemetry
sidecar for read-only metrics. This sets env vars only — the sidecar never
requests its own nvidia.com/gpu, so it consumes no GPU quota and the
executor keeps the allocated GPU. Honored where the NVIDIA runtime accepts
NVIDIA_VISIBLE_DEVICES (most setups; GKE blocks env-based exposure). */}}
{{- $gpuRequested := or (not (empty (dig "limits" "nvidia.com/gpu" "" $mergedResources))) (not (empty (dig "requests" "nvidia.com/gpu" "" $mergedResources))) }}
{{- $sidecarEnv := dict }}
{{- if $gpuRequested }}
{{- $sidecarEnv = dict "NVIDIA_VISIBLE_DEVICES" "all" "NVIDIA_DRIVER_CAPABILITIES" "compute,utility" }}
{{- end }}
{{- $mergedVolumeMounts := $v.volumeMounts | default $baseTpl.volumeMounts | default list }}
{{- $mergedVolumes := $v.volumes | default $baseTpl.volumes | default list }}
{{- if $.Values.telemetry.enabled }}
Expand Down Expand Up @@ -99,7 +110,7 @@ spec:
env:
{{- include "delegated-operator-deployments.env-vars-list" $envContext | indent 12 }}
resources:
{{- toYaml (merge (dict) ($v.resources | default dict) ($baseTpl.resources)) | nindent 12 }}
{{- toYaml $mergedResources | nindent 12 }}
{{- with $mergedVolumeMounts }}
volumeMounts:
{{- toYaml . | nindent 12 }}
Expand Down Expand Up @@ -132,7 +143,7 @@ spec:
periodSeconds: {{ ($v.startup).periodSeconds | default $baseTpl.startup.periodSeconds }}
timeoutSeconds: {{ ($v.startup).timeoutSeconds | default $baseTpl.startup.timeoutSeconds }}
{{- if $.Values.telemetry.enabled }}
{{- include "telemetry.sidecar" (dict "ctx" $ "serviceType" "delegated-operator" "targetName" "fiftyone delegated" "executor" true) | nindent 8 }}
{{- include "telemetry.sidecar" (dict "ctx" $ "serviceType" "delegated-operator" "targetName" "fiftyone delegated" "executor" true "sidecarEnv" $sidecarEnv) | nindent 8 }}
{{- end }}
{{- with (merge (dict) ($v.nodeSelector | default dict) ($baseTpl.nodeSelector)) }}
nodeSelector:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ data:
{{- $mergedVolumeMounts := $jobConfig.volumeMounts | default $baseTpl.volumeMounts | default list }}
{{- $mergedVolumes := $jobConfig.volumes | default $baseTpl.volumes | default list }}

{{- /* When the executor requests a GPU, expose that same GPU to its telemetry
sidecar for read-only metrics. This sets env vars only — the sidecar never
requests its own nvidia.com/gpu, so it consumes no GPU quota and the
executor keeps the allocated GPU. Honored where the NVIDIA runtime accepts
NVIDIA_VISIBLE_DEVICES (most setups; GKE blocks env-based exposure). */}}
{{- $gpuRequested := or (not (empty (dig "limits" "nvidia.com/gpu" "" $mergedResources))) (not (empty (dig "requests" "nvidia.com/gpu" "" $mergedResources))) }}
{{- $sidecarEnv := dict }}
{{- if $gpuRequested }}
{{- $sidecarEnv = dict "NVIDIA_VISIBLE_DEVICES" "all" "NVIDIA_DRIVER_CAPABILITIES" "compute,utility" }}
{{- end }}

{{- if $.Values.telemetry.enabled }}
{{- $hasSocketVolume := false }}
{{- range $vol := $mergedVolumes }}
Expand Down Expand Up @@ -146,7 +157,7 @@ data:
{{- end }}
{{- if $.Values.telemetry.enabled }}
initContainers:
{{- include "telemetry.native-sidecar" (dict "ctx" $ "serviceType" "delegated-operator" "targetName" "fiftyone delegated" "executor" true "targetContainer" "executor") | nindent 12 }}
{{- include "telemetry.native-sidecar" (dict "ctx" $ "serviceType" "delegated-operator" "targetName" "fiftyone delegated" "executor" true "targetContainer" "executor" "sidecarEnv" $sidecarEnv) | nindent 12 }}
{{- end }}
{{- with $mergedNodeSelector }}
nodeSelector:
Expand Down
130 changes: 120 additions & 10 deletions tests/unit/helm/delegated-operator-instance-deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5711,16 +5711,6 @@ func (s *deploymentDelegatedOperatorInstanceTemplateTest) TestTelemetrySocketInj
}
return n
}
// findContainer returns the named container (the main DO container,
// not the telemetry-sidecar) from a pod spec.
findContainer := func(containers []corev1.Container, name string) *corev1.Container {
for i, c := range containers {
if c.Name == name {
return &containers[i]
}
}
return nil
}

testCases := []struct {
name string
Expand Down Expand Up @@ -5783,3 +5773,123 @@ func (s *deploymentDelegatedOperatorInstanceTemplateTest) TestTelemetrySocketInj
})
}
}

// TestTelemetryDisabledOmitsSidecar verifies that when telemetry is disabled
// the rendered DO deployment carries neither the telemetry-sidecar container nor
// the telemetry-socket volume/mount. The disabled shape is also covered
// indirectly by TestContainerCount (container count is 1, not 2) and the
// disableTelemetry-based volume/mount tests; this is the focused negative
// assertion living next to the positive cases above.
func (s *deploymentDelegatedOperatorInstanceTemplateTest) TestTelemetryDisabledOmitsSidecar() {
const socketName = "telemetry-socket"

options := &helm.Options{SetValues: map[string]string{
"telemetry.enabled": "false",
"delegatedOperatorDeployments.deployments.teamsDoCpuDefault.enabled": "true",
}}
output := helm.RenderTemplate(s.T(), options, s.chartPath, s.releaseName, s.templates)

docs := strings.Split(output, "---")
s.Require().GreaterOrEqual(len(docs), 2, "expected at least one rendered deployment")

var deployment appsv1.Deployment
helm.UnmarshalK8SYaml(s.T(), docs[1], &deployment)

s.Nil(findContainer(deployment.Spec.Template.Spec.Containers, "telemetry-sidecar"),
"telemetry-sidecar container should be absent when telemetry is disabled")
s.Nil(findVolume(deployment.Spec.Template.Spec.Volumes, socketName),
"telemetry-socket volume should be absent when telemetry is disabled")

main := findContainer(deployment.Spec.Template.Spec.Containers, "teams-do-cpu-default")
s.Require().NotNil(main, "main DO container not found")
s.Nil(findVolumeMount(main.VolumeMounts, socketName),
"telemetry-socket volumeMount should be absent when telemetry is disabled")
}

// TestTelemetrySidecarGpuEnv verifies that when a delegated-operator executor
// requests a GPU (via resources.limits or resources.requests), its
// telemetry-sidecar is given the NVIDIA_* env vars needed to read GPU metrics,
// without the sidecar requesting its own nvidia.com/gpu allocation. When no GPU
// is requested, the sidecar receives no NVIDIA_* env vars.
func (s *deploymentDelegatedOperatorInstanceTemplateTest) TestTelemetrySidecarGpuEnv() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, let's consider the tech debt of adding functions like this. Do we need to add tests for the test functions? Being declarative in tests is often preferred to these.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to a shared file

const gpuResource = "nvidia.com/gpu"

// gpuKey escapes the dot in nvidia.com/gpu so helm --set treats the whole
// string as a single map key rather than a nested path.
gpuKey := func(section string) string {
return "delegatedOperatorDeployments.deployments.teamsDoCpuDefault.resources." +
section + ".nvidia\\.com/gpu"
}

testCases := []struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a test case for when telemetry.enabled:false? Or is that covered somewhere else?

name string
values map[string]string
expectGpu bool
}{
{
name: "gpuInLimitsExposesEnvToSidecar",
values: map[string]string{
"telemetry.enabled": "true",
"delegatedOperatorDeployments.deployments.teamsDoCpuDefault.enabled": "true",
gpuKey("limits"): "1",
},
expectGpu: true,
},
{
name: "gpuInRequestsExposesEnvToSidecar",
values: map[string]string{
"telemetry.enabled": "true",
"delegatedOperatorDeployments.deployments.teamsDoCpuDefault.enabled": "true",
gpuKey("requests"): "1",
},
expectGpu: true,
},
{
name: "noGpuOmitsEnvFromSidecar",
values: map[string]string{
"telemetry.enabled": "true",
"delegatedOperatorDeployments.deployments.teamsDoCpuDefault.enabled": "true",
},
expectGpu: false,
},
}

for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
subT := s.T()
subT.Parallel()

options := &helm.Options{SetValues: tc.values}
output := helm.RenderTemplate(subT, options, s.chartPath, s.releaseName, s.templates)

docs := strings.Split(output, "---")
s.Require().GreaterOrEqual(len(docs), 2, "expected at least one rendered deployment")

var deployment appsv1.Deployment
helm.UnmarshalK8SYaml(subT, docs[1], &deployment)

sidecar := findContainer(deployment.Spec.Template.Spec.Containers, "telemetry-sidecar")
s.Require().NotNil(sidecar, "telemetry-sidecar container not found")

visibleDevices, hasVisibleDevices := envValue(sidecar.Env, "NVIDIA_VISIBLE_DEVICES")
driverCaps, hasDriverCaps := envValue(sidecar.Env, "NVIDIA_DRIVER_CAPABILITIES")

if tc.expectGpu {
s.True(hasVisibleDevices, "sidecar should have NVIDIA_VISIBLE_DEVICES")
s.Equal("all", visibleDevices, "NVIDIA_VISIBLE_DEVICES value mismatch")
s.True(hasDriverCaps, "sidecar should have NVIDIA_DRIVER_CAPABILITIES")
s.Equal("compute,utility", driverCaps, "NVIDIA_DRIVER_CAPABILITIES value mismatch")

// The sidecar reads the executor's GPU; it must not request its own.
_, limitsHasGpu := sidecar.Resources.Limits[corev1.ResourceName(gpuResource)]
_, requestsHasGpu := sidecar.Resources.Requests[corev1.ResourceName(gpuResource)]
s.False(limitsHasGpu, "sidecar must not request nvidia.com/gpu in limits")
s.False(requestsHasGpu, "sidecar must not request nvidia.com/gpu in requests")
} else {
s.False(hasVisibleDevices, "sidecar should not have NVIDIA_VISIBLE_DEVICES when no GPU requested")
s.False(hasDriverCaps, "sidecar should not have NVIDIA_DRIVER_CAPABILITIES when no GPU requested")
}
})
}
}
Loading
Loading