Skip to content

feat(ml): ml-batch-job chart + quanvnn application — audit-driven fixes folded#38

Merged
JackMaarek merged 23 commits into
mainfrom
feature/ml-batch-job-chart
Apr 26, 2026
Merged

feat(ml): ml-batch-job chart + quanvnn application — audit-driven fixes folded#38
JackMaarek merged 23 commits into
mainfrom
feature/ml-batch-job-chart

Conversation

@JackMaarek

@JackMaarek JackMaarek commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Folds the two audit-driven fixes from AUDIT_QUANVNN_MVP.md (2026-04-26) into
the existing feature branch, ready to merge to main.

Audit findings addressed

  • BLOCKER-1 (HIGH, §2.4): chart args ↔ mount paths ↔ image contract misalignment.
    Resolved via Fix A — Helm-only, no QuanvNN code change.
  • MEDIUM (§2.5): quanvnn.yaml pinned project: platform while
    argocd/projects/ml.yaml was dead code. Repointed to project: ml and
    added ignoreDifferences for batch/Job /status and Pod controller-uid.

Changes

  • kubernetes/helm/values/quanvnn-dev.yaml — paths, args, gpu.enabled
  • argocd/applications/ml/quanvnn.yaml — project, ignoreDifferences

Validation

  • helm lint passes.
  • helm template rendered output shows the corrected paths
    (/data/dubai_data/Dubai256, /data/quanv_ckpt, /app/output/fusion).
  • kubectl apply --dry-run=client passes on the ArgoCD Application.

Out of scope (Sprint 2)

  • NVIDIA device plugin, PriorityClasses, S3+IRSA, ExternalSecret template,
    Kyverno allowlist, AWS SM seed.

Identity chain (audit §6)

ServiceAccount: quanvnn-sa in ml. OIDC subject:
system:serviceaccount:ml:quanvnn-sa. Do NOT rename nameOverride,
releaseName, or serviceAccount.name — silently breaks IRSA in Sprint 2.

Follow-up commits (review-driven, 2026-04-26)

Doc-grounded review (PR_38_REVIEW.md, Context7 cross-checks) flagged two
items requiring action; PodMonitor namespace selector was already correct
(prometheus-values.yaml uses match-all {}, CASE B — no commit).

  • 5c559dc docs(ml-batch-job): correct istio-injection state for ml namespace
    Stale README claimed namespace had injection enabled with per-pod opt-out.
    Actual: kubernetes/namespaces/ml-namespace.yaml has no istio-injection
    label (= disabled). README aligned; obsolete sidecar.istio.io/inject=false
    block removed.
  • 04697ad fix(argocd/ml): cover prefixed controller-uid label in ignoreDifferences
    Per Kubernetes labels-annotations-taints reference (Context7 / k8s docs):
    K8s 1.27+ Job controller sets BOTH controller-uid AND
    batch.kubernetes.io/controller-uid simultaneously on owned Pods. K8s 1.33
    still sets both (verified via researchMode query). Added the prefixed
    jsonPointer /metadata/labels/batch.kubernetes.io~1controller-uid (RFC 6901
    encoding) alongside the legacy entry to prevent ArgoCD drift on the new key.

…trap sequence

- Create kubernetes/namespaces/istio-ingress.yaml with istio-injection enabled
- Fix gateway.yaml: wave 1→2, namespace istio-system→istio-ingress,
  remove sidecar.istio.io/inject: false, add HA config (replicas, PDB, HPA)
- Fix istiod.yaml: wave 0→-1 to ensure control plane healthy before gateway
- Fix platform.yaml: add istio-ingress destination, wildcard clusterResourceWhitelist

Resolves: gateway ErrImagePull on image:auto due to missing injection
Resolves: sync-wave ordering violation (gateway before istiod healthy)
Resolves: AppProject blocking gateway sync to istio-ingress namespace
Batch Jobs are ephemeral — no Service exists or is needed.
The headless Service was an unnecessary indirection solely for
ServiceMonitor discovery. PodMonitor targets pods directly by
label selector, single file, same Prometheus Operator stack.

Removes: service.yaml, service-monitor.yaml
Adds: pod-monitor.yaml
Updates: ml AppProject whitelist (PodMonitor replaces ServiceMonitor+Service)
…, gpu flag, volumeMounts

- kubernetes/namespaces/ml-namespace.yaml: remove istio-injection: enabled label;
  namespace-level injection must be absent for batch Jobs — use pod-level opt-in instead
- argocd/applications/ml/quanvnn.yaml: replace hardcoded env name with __ENV__ placeholder
  in name, environment label, and values file path; add sync-wave: "9" annotation;
  change project from ml to platform
- kubernetes/helm/ml-batch-job/Chart.yaml: fix comment ServiceMonitor → PodMonitor
- kubernetes/helm/ml-batch-job/values.yaml: fix namespace comment (chart does not create
  namespace — platform prerequisite); rename metrics.serviceMonitor → metrics.podMonitor;
  add workload.kind field; add mountPath/readOnly fields to persistentVolumes schema;
  compress to 199 lines (was 255, exceeds 200-line limit)
- kubernetes/helm/ml-batch-job/templates/pod-monitor.yaml: update guard and scrapeInterval
  reference from serviceMonitor to podMonitor key
- kubernetes/helm/ml-batch-job/templates/job-cpu.yaml: volumeMounts now use .mountPath
  (default /<name>) and .readOnly (default false) per-volume fields
- kubernetes/helm/ml-batch-job/templates/job-gpu.yaml: same volumeMount fix as job-cpu
- kubernetes/helm/values/quanvnn-dev.yaml: gpu.enabled false (dev runs CPU-only);
  add workload.kind: Job; add mountPath/readOnly per PVC (dubai_data ro, quanv_ckpt ro,
  fusion_qnn/results rw); rename serviceMonitor → podMonitor
…base

- argocd/platform/external-secrets/external-secrets.yaml: restore __ENV__,
  __TARGET_REVISION__, __AWS_REGION__, __ESO_IRSA_ROLE_ARN__ placeholders
  that leaked as hardcoded dev values during rebase
- argocd/projects/applications.yaml: restore __REPO_URL__ placeholder
  (was overwritten with hardcoded GitHub URL during rebase conflict resolution)
Required by quanvnn Application moving from project:ml to project:platform.
The monitoring namespace was already present in the destination list.
… volumeMounts, AppProject

- kubernetes/namespaces/ml-namespace.yaml: remove istio-injection: enabled; rewrite
  comment to reflect pod-level opt-in for batch Jobs
- argocd/applications/ml/quanvnn.yaml: replace hardcoded env name with __ENV__ in name,
  label, and values file path; add sync-wave: "9"; change project: ml → project: platform
- kubernetes/helm/ml-batch-job/Chart.yaml: fix comment ServiceMonitor → PodMonitor
- kubernetes/helm/ml-batch-job/values.yaml: fix namespace comment (chart does not create
  namespace); rename metrics.serviceMonitor → metrics.podMonitor; add workload.kind field;
  add mountPath and readOnly fields to persistentVolumes schema; compress to 199 lines
- kubernetes/helm/ml-batch-job/templates/pod-monitor.yaml: update guard and scrapeInterval
  reference from serviceMonitor to podMonitor key
- kubernetes/helm/ml-batch-job/templates/job-cpu.yaml: volumeMounts use .mountPath
  (default /<name>) and .readOnly (default false) per volume
- kubernetes/helm/ml-batch-job/templates/job-gpu.yaml: same volumeMount fix
- kubernetes/helm/values/quanvnn-dev.yaml: gpu.enabled: false (dev CPU-only); add
  workload.kind: Job; mountPath/readOnly per PVC — data:/data/dubai_data (ro),
  checkpoints:/data/quanv_ckpt (ro), results:/app/fusion_qnn/results (rw);
  rename serviceMonitor → podMonitor
- argocd/projects/platform.yaml: add ml namespace to destinations
- argocd/platform/external-secrets/external-secrets.yaml: restore __ENV__,
  __TARGET_REVISION__, __AWS_REGION__, __ESO_IRSA_ROLE_ARN__ placeholders
- argocd/projects/applications.yaml: restore __REPO_URL__ placeholder

@JackMaarek JackMaarek left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review — ml-batch-job chart + QuanvNN Application

Reviewed against Helm best practices (Context7 / helm.sh), Prometheus Operator PodMonitor patterns, and platform conventions.

Overall shape is solid — the chart is reusable, values are well-structured, the ArgoCD Application uses multi-source correctly, and the conformity audit fixes are all applied. Three issues must be resolved before merge; four are non-blocking suggestions.


🔴 BLOCKING — must fix before merge

1. Prometheus has no RBAC to scrape the ml namespace

pod-monitor.yaml creates a PodMonitor in the ml namespace, but kube-prometheus-stack only provisions RBAC (Role + RoleBinding) for Prometheus in the namespaces it controls by default (monitoring, kube-system, default). Without a Role granting get/list/watch on pods and a matching RoleBinding for the Prometheus ServiceAccount in the ml namespace, the PodMonitor will be silently ignored at runtime — no scrape targets, no errors, nothing in Prometheus UI.

Fix: add a Role + RoleBinding in kubernetes/manifests/monitoring/ (or as chart templates guarded by metrics.enabled):

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: prometheus-ml-scraper
  namespace: ml
rules:
  - apiGroups: [""]
    resources: ["pods"]
    verbs: ["get", "list", "watch"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: prometheus-ml-scraper
  namespace: ml
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: prometheus-ml-scraper
subjects:
  - kind: ServiceAccount
    name: kube-prometheus-stack-prometheus
    namespace: monitoring

Also verify that prometheus.prometheusSpec.podMonitorNamespaceSelector in prometheus-values.yaml is set to match-all (matchLabels: {}) or explicitly includes the ml namespace — otherwise the Prometheus Operator itself won't watch for PodMonitors outside its own namespace.


2. Empty image.cpu.repository renders silently as :cpu-latest

job-cpu.yaml:

image: "{{ .Values.image.cpu.repository }}:{{ .Values.image.cpu.tag }}"

If image.cpu.repository is not overridden (chart default is ""), this renders as :cpu-latest — syntactically accepted by the Kubernetes API but fails with InvalidImageName at scheduling time. The error is hard to trace back to a missing value.

Use Helm's required function to catch this at helm template / helm install time:

image: "{{ required "image.cpu.repository must be set" .Values.image.cpu.repository }}:{{ .Values.image.cpu.tag }}"

Same fix for image.gpu.repository inside the {{- if .Values.gpu.enabled }} block in job-gpu.yaml.


3. README.md namespace table is stale after Istio injection fix

The table in README.md reads:

| `ml` | ML batch workloads | enabled (Jobs opt-out via annotation) | restricted |

But ml-namespace.yaml no longer carries istio-injection: enabled — it was removed during the conformity audit. The table should show disabled to reflect the actual state.


🟡 SUGGESTIONS — non-blocking, recommended for follow-up

4. Missing values.schema.json

Helm recommends a values.schema.json (JSON Schema draft-07) alongside values.yaml to enforce types, enums, and required fields at helm install / helm lint time — before any template rendering runs. This is the correct layer to constrain workload.kind to ["Job"], pullPolicy to the three valid strings, and namespace.name to a non-empty string.

Minimal schema for the critical fields:

{
  "$schema": "https://json-schema.org/draft-07/schema#",
  "type": "object",
  "required": ["namespace", "image"],
  "properties": {
    "workload": {
      "type": "object",
      "properties": {
        "kind": { "type": "string", "enum": ["Job"] }
      }
    },
    "namespace": {
      "type": "object",
      "required": ["name"],
      "properties": { "name": { "type": "string", "minLength": 1 } }
    },
    "image": {
      "type": "object",
      "properties": {
        "cpu": {
          "type": "object",
          "properties": {
            "pullPolicy": { "type": "string", "enum": ["Always", "IfNotPresent", "Never"] }
          }
        },
        "gpu": {
          "type": "object",
          "properties": {
            "pullPolicy": { "type": "string", "enum": ["Always", "IfNotPresent", "Never"] }
          }
        }
      }
    }
  }
}

5. No ttlSecondsAfterFinished on Job specs

Without a TTL, completed and failed Jobs accumulate indefinitely. The quanvnn Application intentionally retains them for log inspection — valid, but a long TTL makes the retention policy explicit rather than implicit:

spec:
  ttlSecondsAfterFinished: 604800  # 7 days — retain for log inspection, then auto-clean
  backoffLimit: {{ .Values.cpu.backoffLimit }}

Expose this as a configurable value in values.yaml (default 604800, set to 0 to disable retention).


6. workload.kind has no template guard

values.yaml documents workload.kind: "Job" with a note that only Job is currently implemented. But no template enforces this — a user setting workload.kind: Deployment silently gets a Job. Add a fail guard at the top of job-cpu.yaml:

{{- if ne .Values.workload.kind "Job" }}
{{- fail (printf "workload.kind '%s' is not supported. Only 'Job' is currently implemented." .Values.workload.kind) }}
{{- end }}

7. completions and parallelism not explicit on Job specs

Kubernetes defaults both to 1 when omitted, but making them explicit documents intent and avoids surprises if the defaults ever change in a future API version:

spec:
  completions: 1
  parallelism: 1
  backoffLimit: {{ .Values.cpu.backoffLimit }}

✅ What's correct

  • Multi-source ArgoCD Application with $values alias — correct pattern for separating chart from per-env values ref
  • sync-wave: "9" — correctly placed after monitoring stack (waves 4–8)
  • CreateNamespace=false in syncOptions — namespace is platform-managed, not chart-managed
  • prune: true + selfHeal: true — right for a commit-triggered Job model
  • ignoreDifferences on /status for batch/Job — prevents ArgoCD from flagging Completed Jobs as OutOfSync
  • PodMonitor instead of ServiceMonitor — correct for ephemeral Jobs with no Service endpoint
  • Grafana dashboard ConfigMap in monitoring namespace with grafana_dashboard: "1" — correct sidecar discovery pattern
  • podSecurityContext.seccompProfile: RuntimeDefault — satisfies PSS restricted profile
  • readOnly: true on data and checkpoint PVCs in quanvnn-dev.yaml — correct least-privilege for read-only inputs
  • Named templates prefixed with ml-batch-job.* — correct Helm convention, prevents subchart name conflicts
  • __ENV__ / __TARGET_REVISION__ / __REPO_URL__ placeholders throughout — correct for platform-bot hydration model

…ents

- job-cpu.yaml: add required validation on image.cpu.repository — prevents
  silent :cpu-latest render when repository is unset; add workload.kind fail
  guard; add completions:1 parallelism:1; add ttlSecondsAfterFinished
- job-gpu.yaml: same fixes — required on image.gpu.repository, completions,
  parallelism, ttlSecondsAfterFinished
- values.yaml: add job.ttlSecondsAfterFinished (default 604800 — 7 days);
  keep at 200 lines
- values.schema.json: new — JSON Schema draft-07 for helm install/lint
  validation; enforces workload.kind enum, required fields, port range,
  pullPolicy enum
kube-prometheus-stack does not automatically create Role/RoleBinding outside
its own namespace. Without this, PodMonitors in the ml namespace are silently
ignored — no scrape targets appear in Prometheus.

Role grants get/list/watch on pods in ml namespace to the Prometheus
ServiceAccount (kube-prometheus-stack-prometheus) in the monitoring namespace.
podMonitorNamespaceSelector: {} in prometheus-values.yaml already enables
match-all namespace discovery for PodMonitors.
BLOCKER-1 from AUDIT_QUANVNN_MVP.md: chart args and mount paths did not
match the QuanvNN image defaults (Dockerfile + perspeqtive/output_config.py).

Apply Fix A (audit recommended):
- persistentVolumes.results.mountPath: /app/fusion_qnn/results -> /app/output/fusion
- Remove DATASET_DIR / QUANV_CHECKPOINT_DIR / OUTPUT_DIR env-var overrides from config:
- gpu.enabled: false -> true
- gpu.args: align values with /data/dubai_data/Dubai256, /data/quanv_ckpt,
  /app/output/fusion; add --skip-calibrate --skip-train --skip-evaluate (step 1 only)
- cpu.args: align values with same paths; keep --skip-extract (steps 2-N)

No QuanvNN code change. Helm-only fix.
AUDIT_QUANVNN_MVP.md §2.5 (MEDIUM): quanvnn.yaml pinned project: platform
while argocd/projects/ml.yaml exists with proper namespaceResourceWhitelist
scoping (SA, ConfigMap, PVC, Job, ExternalSecret, PodMonitor in ml namespace
+ Grafana dashboard ConfigMap in monitoring namespace).

- project: platform -> ml
- Add ignoreDifferences for batch/Job /status (Jobs reach Completed and
  ArgoCD must not flag drift) and Pod controller-uid label (kubelet auto-set).
@JackMaarek JackMaarek changed the title Feature/ml batch job chart feat(ml): ml-batch-job chart + quanvnn application — audit-driven fixes folded Apr 25, 2026
Audit conformity fixes set istio-injection: disabled on the ml namespace
(batch Jobs cannot complete with sidecar; PSS-restricted incompatible
with istio-init). The README section still showed 'enabled (Jobs opt-out
via annotation)' from an earlier draft.

Align the documentation with kubernetes/namespaces/ml-namespace.yaml and
remove the obsolete sidecar.istio.io/inject=false code block (the chart
no longer renders the per-pod opt-out since the namespace is disabled).
K8s 1.27 introduced batch.kubernetes.io/controller-uid as the canonical
label key (KEP-2473). The Job controller in K8s 1.33 sets BOTH the legacy
controller-uid and the new prefixed batch.kubernetes.io/controller-uid
labels on every Pod it owns (verified via Kubernetes labels-annotations-
taints reference).

Without this entry, ArgoCD continues to flag drift on the new label every
time a Job creates a Pod, even though the legacy label is already ignored.

RFC 6901 JSON Pointer encoding: '/' inside a token is encoded as '~1',
hence /metadata/labels/batch.kubernetes.io~1controller-uid.

Source: https://kubernetes.io/docs/reference/labels-annotations-taints/
@JackMaarek
JackMaarek merged commit 20f6624 into main Apr 26, 2026
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant