Skip to content

feat(power-agent): per-node power-cap enforcement DaemonSet - #9682

Merged
nv-anants merged 28 commits into
mainfrom
pr1a/power-agent
Jun 25, 2026
Merged

feat(power-agent): per-node power-cap enforcement DaemonSet#9682
nv-anants merged 28 commits into
mainfrom
pr1a/power-agent

Conversation

@kaim-eng

@kaim-eng kaim-eng commented May 18, 2026

Copy link
Copy Markdown
Contributor

Part of the PR #9369 split plan.
This is PR 1 of 6 (PR 1a - Power Agent).

Predecessor: none (base = main)
Successor: #9683 (stacked on pr1a/power-agent; #9682 must land first)

Summary

Standalone Power Agent DaemonSet for per-node GPU power-cap enforcement, packaged as a Helm chart. The PR is reviewable in isolation from planner logic and has zero import dynamo.planner.* coupling.

Current branch shape: 26 files changed: net-new components/power_agent/ code and deploy/helm/charts/power-agent/ chart files, plus CI plumbing edits to 5 existing .github/ files.

  • components/power_agent/power_agent.py (566 LOC): NVML clamp helper, cgroup parser, multi-pod policy, SIGTERM restore, fail-safe reconcile (_list_pods_on_node -> Optional[list]), and UUID-gated orphan-cap restoration.
  • components/power_agent/Dockerfile: slim Python runtime; libnvidia-ml.so is injected at runtime by runtimeClassName: nvidia.
  • components/power_agent/tests/: 47 unit tests covering apply_cap, cgroup parsing, multi-pod policy, reconcile fail-safe, and shutdown.
  • deploy/helm/charts/power-agent/: Helm chart with DaemonSet, dev pod, ServiceAccount, and RBAC. RBAC renders namespaces through {{ .Release.Namespace }} and supports cluster-scoped or namespace-restricted mode.
  • .github/: power_agent path filter plus dedicated power-agent image build/push wiring in PR, post-merge, and release workflows.

Reviewer context:

  • Design context: docs/design-docs/powerplanner-design.md section 7 (lands later in the stack; readable from pr5/docs-devenv).
  • Planner contract: the only planner-facing coupling is the dynamo.nvidia.com/gpu-power-limit pod annotation.
  • Split-plan context: sections 2.1 and 2.1.2.

Validation

  • Required/current GitHub checks pass at the current tip; rust jobs and team-request are intentionally skipped by filters.
  • components/power_agent/tests/: 47 unit tests in-tree. The last full dev-pod run before the reconcile fail-safe additions was 43 passed on 2026-05-18; CI is green at the current tip.
  • Pre-commit on origin/main..pr1a/power-agent: 14 hooks pass, 4 skipped. pytest-marker-report was reconfirmed in a Linux pod with Missing sets: 0. The Windows host has the known pre-existing POSIX-only fcntl import issue in untouched test utilities; Linux CI is unaffected.
  • DCO: commits are signed off.

Merge Strategy

Rebase-and-merge, no squash, preserving the single PR commit in main history per split-plan section 4.3.

Stack Order

Merge #9682 first. #9683 and #9790 are both stacked on pr1a/power-agent and become actionable after #9682 lands.

@kaim-eng
kaim-eng requested review from a team as code owners May 18, 2026 15:53
@copy-pr-bot

copy-pr-bot Bot commented May 18, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This PR introduces a complete Kubernetes DaemonSet agent that enforces per-GPU NVML power caps on worker nodes. The daemon reconciles every 15 seconds, mapping running GPU processes to pod UIDs via cgroup parsing, reading power-limit annotations, resolving multi-pod conflicts, and applying caps via NVML with Prometheus metrics and graceful shutdown support.

Changes

Power Agent: GPU NVML Power Cap Enforcement DaemonSet

Layer / File(s) Summary
Module foundation and utilities
components/power_agent/power_agent.py (lines 1–213)
Module header, optional lazy imports for NVML/Kubernetes/Prometheus with fallbacks, logging, constants, cgroup parsing regex for extracting pod UIDs from /proc/{pid}/cgroup, atomic JSON persistence for managed GPU state, UUID normalization for pynvml version compatibility, and Prometheus metrics infrastructure with noop fallback when unavailable.
NVML cap application and multi-pod policy
components/power_agent/power_agent.py (lines 220–404), components/power_agent/tests/test_apply_cap.py, components/power_agent/tests/test_multi_pod_policy.py
NVML helper functions to clamp watts to device min/max constraints and apply power caps with logging and metric updates; multi-pod-per-GPU resolution policy that applies an agreed cap when all pods match, otherwise applies safe default on conflicts or missing/invalid annotations, incrementing error counters; unit tests validating clamping boundaries, NVML error tolerance, UUID handling (bytes vs str), and all policy scenarios (single pod, agreement, conflict, invalid annotations, mixed None/valid).
Shutdown and startup recovery
components/power_agent/power_agent.py (lines 280–337), components/power_agent/tests/test_shutdown.py
SIGTERM/SIGINT handler restores managed GPU power limits to NVML defaults and signals main loop exit; startup orphan recovery loads persisted GPU UUIDs, checks idle status, restores default caps for idle GPUs, and updates persisted state; unit tests validating per-GPU restoration, graceful shutdown with no managed GPUs, and NVML error handling.
PowerAgent orchestration
components/power_agent/power_agent.py (lines 411–577)
PowerAgent class initializes NVML/Kubernetes clients, loads orphan state, lists node pods, maps running PIDs to pod UIDs, builds UID-to-annotation mappings, resolves GPU cap via policy, applies caps via NVML, and persists managed state; blocking reconcile loop runs every 15 seconds; CLI main() entry point accepts --safe-default-watts, optional node/namespace scope, and Prometheus port, then starts daemon.
Cgroup parser validation
components/power_agent/tests/test_cgroup_parser.py
Unit tests for cgroup UID extraction across cgroup v1/v2 with systemd and cgroupfs layouts, QoS classes (Guaranteed/Burstable/BestEffort), and non-Kubernetes inputs; validates OSError handling and first-match-wins behavior when multiple cgroup lines exist.
Kubernetes deployment
deploy/power_agent/rbac.yaml, deploy/power_agent/daemonset.yaml, deploy/power_agent/dev-pod.yaml
RBAC ServiceAccount, ClusterRole with pod read-access, and ClusterRoleBinding; production DaemonSet runs on GPU-labeled nodes with hostPID, privileged container, nvidia runtime, environment variables for safe default (500 W) and node name, resource limits, and volume mounts for /proc (read-only) and persisted state at /var/lib/dynamo-power-agent; development Pod harness for manual testing and validation on live clusters.
Documentation and CI integration
components/power_agent/README.md, .github/filters.yaml
README documents Power Agent's 15-second reconciliation, cgroup UID extraction, annotation reading, NVML power cap enforcement, troubleshooting steps (annotations, logs), Prometheus metrics (applied limit, conflict/safe-default/failure counters), and shutdown/startup semantics; CI filter update triggers planner jobs on power_agent component and deployment changes.

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: introduction of a per-node power-cap enforcement DaemonSet for the Power Agent.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed PR description is comprehensive with clear overview, detailed summary of changes, specific file guidance, related issues reference, and explicit stack ordering context.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 5

🧹 Nitpick comments (1)
components/power_agent/power_agent.py (1)

537-539: ⚡ Quick win

Move argparse import to module scope.

Line 538 imports inside main(), which violates the repo’s Python import placement rule.

Proposed fix
+import argparse
 import json
 import logging
@@
 def main() -> None:
-    import argparse
-
     parser = argparse.ArgumentParser(description="Dynamo Power Agent DaemonSet")
As per coding guidelines: "Keep all imports at the top of each file (flag any import inside functions/classes)."
🤖 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 `@components/power_agent/power_agent.py` around lines 537 - 539, The argparse
import is inside the main() function; move the import to module scope by adding
"import argparse" at the top of the file and removing the in-function import in
main(), ensuring any type or usage remains correct (adjust other imports if
needed) so linting and the repo rule about top-level imports are satisfied.
🤖 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 `@components/power_agent/power_agent.py`:
- Around line 119-124: The function _load_previously_managed_gpus should
defensively handle malformed or unreadable state files: when reading
_MANAGED_STATE_PATH, catch OSError in addition to
FileNotFoundError/JSONDecodeError, load the JSON into a variable (e.g., data =
json.load(f)), verify isinstance(data, dict) before calling
data.get("managed_uuids", []), and if the root is not a dict (or managed_uuids
is not an iterable of strings) return an empty set; ensure any
TypeError/ValueError from unexpected types is handled and results in returning
set() rather than letting startup crash.
- Around line 294-297: Replace the broad except that swallows errors from the
pynvml.nvmlShutdown() call with a targeted handler: catch pynvml.NVMLError (or
the specific pynvml error class) as e, log the exception with the module/class
logger (e.g., logger.exception("NVML shutdown failed: %s", e) or
logging.exception(...)) and then re-raise the error to avoid silent failures;
modify the try/except around pynvml.nvmlShutdown() accordingly so only
NVML-specific errors are caught, logged, and propagated.
- Around line 494-501: The loop that builds pod_annotations appends one entry
per GPU process, causing pods with multiple processes to be counted multiple
times; change the logic in the procs loop (using _extract_pod_uid_from_cgroup
and uid_to_annotation) to deduplicate by pod UID before applying multi-pod
policy — e.g., track seen UIDs (or build a uid->annotation map) and only append
one (uid, annotation) pair per unique UID so each pod is counted once when
evaluating multi-pod warnings/metrics.

In `@deploy/power_agent/daemonset.yaml`:
- Around line 16-35: The DaemonSet metadata.namespace is hardcoded to "default",
which causes mismatches with the RBAC/service account namespace; update the
manifest to use the parameterized namespace variable (e.g.
${POWER_AGENT_NAMESPACE}) instead of "default" and ensure the ServiceAccount
referenced by spec.serviceAccountName ("dynamo-power-agent") is created/bound in
that same parameterized namespace so RBAC bindings resolve correctly; locate
metadata.name ("dynamo-power-agent"), metadata.namespace, and
spec.serviceAccountName in the template to make the change.
- Around line 55-58: Replace the mutable image tag
"nvcr.io/nvidia/dynamo/power-agent:latest" with an immutable reference (a
specific version tag or an image digest), e.g.
"nvcr.io/nvidia/dynamo/power-agent:vX.Y.Z" or
"nvcr.io/nvidia/dynamo/power-agent@sha256:<digest>", so deployments are
reproducible; keep or verify imagePullPolicy (e.g. IfNotPresent) is appropriate
for pinned images and update the inline comment to note the image is
intentionally pinned.

---

Nitpick comments:
In `@components/power_agent/power_agent.py`:
- Around line 537-539: The argparse import is inside the main() function; move
the import to module scope by adding "import argparse" at the top of the file
and removing the in-function import in main(), ensuring any type or usage
remains correct (adjust other imports if needed) so linting and the repo rule
about top-level imports are satisfied.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f9faddf8-30e0-4a7b-9055-b45244b2a979

📥 Commits

Reviewing files that changed from the base of the PR and between f20ff4e and 3cde449.

📒 Files selected for processing (11)
  • .github/filters.yaml
  • components/power_agent/README.md
  • components/power_agent/power_agent.py
  • components/power_agent/tests/__init__.py
  • components/power_agent/tests/test_apply_cap.py
  • components/power_agent/tests/test_cgroup_parser.py
  • components/power_agent/tests/test_multi_pod_policy.py
  • components/power_agent/tests/test_shutdown.py
  • deploy/power_agent/daemonset.yaml
  • deploy/power_agent/dev-pod.yaml
  • deploy/power_agent/rbac.yaml

Comment thread deploy/power-agent/power_agent.py
Comment thread components/power_agent/power_agent.py Outdated
Comment thread deploy/power-agent/power_agent.py
Comment thread deploy/power_agent/daemonset.yaml Outdated
Comment thread deploy/power_agent/daemonset.yaml Outdated
@kaim-eng
kaim-eng force-pushed the pr1a/power-agent branch from 3cde449 to 1338028 Compare May 18, 2026 16:03
Comment thread deploy/power_agent/rbac.yaml Outdated
Comment thread deploy/power-agent/power_agent.py
@kaim-eng
kaim-eng force-pushed the pr1a/power-agent branch from 1338028 to ec21081 Compare May 19, 2026 12:54
kaim-eng added a commit that referenced this pull request May 19, 2026
Folds deploy/power_agent/{daemonset,rbac,dev-pod}.yaml into a single
Helm chart at deploy/helm/charts/power-agent/, resolving the three
CodeRabbit findings on PR #9682:

  * hardcoded metadata.namespace=default -> {{ .Release.Namespace }}
  * mutable image :latest -> required image.tag with fail-fast validator
  * `${POWER_AGENT_NAMESPACE}` envsubst placeholder -> native Helm templating

The chart supports three deployment shapes selectable via values:
production DaemonSet (default, cluster-wide RBAC), namespace-restricted
production (Role+RoleBinding), and an in-cluster dev-iteration Pod
mounting power_agent.py from a ConfigMap. Three template-time validators
reject foot-guns at install time: empty image.tag, mutex violations
between daemonset.enabled and dev.enabled, and dev mode without a
pinned dev.nodeName. Dev mode also automatically forces namespace-scoped
RBAC (least privilege), leveraging power_agent.py's --namespace flag.

Design rationale, scope decisions, and review-feedback responses are
captured in docs/design-docs/power-agent-helm-chart-plan.md, committed
alongside the chart.

components/power_agent/README.md flips its install recipe to
``helm install``, and the planner CI filter (.github/filters.yaml) is
retargeted from deploy/power_agent/** to deploy/helm/charts/power-agent/**.
Two examples/deployments/powerplanner/*.yaml header references live on
PR #9687 and will be updated during that PR's cascade rebase per plan
section 5.3.

Validated locally:
  helm lint                                -> 0 errors
  helm template (3 positive exercises)     -> expected resources
  helm template (3 negative exercises)     -> expected fail-fast errors
  components/power_agent/tests/            -> 43/43 passed
  .github/scripts/test-filters.js          -> 20/20 passed
  pre-commit (cross-cutting hooks)         -> all applicable passed

Signed-off-by: Kai Ma <kaim@nvidia.com>
@github-actions github-actions Bot added the deployment::k8s Relates to dynamo deployment in kubernetes label May 19, 2026
kaim-eng added a commit that referenced this pull request May 19, 2026
Updates the three reference sites in examples/deployments/powerplanner/
that previously instructed users to ``kubectl apply -f deploy/power_agent/...``,
flipping them to the new Helm chart at deploy/helm/charts/power-agent/
that landed in PR #9682:

  * disagg-power-aware.yaml header recipe
  * README.md Prerequisites + verify section
  * MULTI_DGD.md file index

Also updates the verify-pods label selector from the legacy
``app=dynamo-power-agent`` to the chart-emitted
``app.kubernetes.io/name=power-agent``.

Design docs (powerplanner-design.md, power-agent-dcgm-actuator.md,
pr9369-split-plan.md, power-agent-helm-chart-plan.md) still reference
the old paths in their historical / architectural narrative sections,
which is intentional -- those describe the pre-chart state and the
transition rationale, not current deployment instructions.

Part of the PR #9369 cascade following PR #9682''s Helm chart landing.

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 19, 2026
Updates the three reference sites in examples/deployments/powerplanner/
that previously instructed users to ``kubectl apply -f deploy/power_agent/...``,
flipping them to the new Helm chart at deploy/helm/charts/power-agent/
that landed in PR #9682:

  * disagg-power-aware.yaml header recipe
  * README.md Prerequisites + verify section
  * MULTI_DGD.md file index

Also updates the verify-pods label selector from the legacy
``app=dynamo-power-agent`` to the chart-emitted
``app.kubernetes.io/name=power-agent``.

Design docs (powerplanner-design.md, power-agent-dcgm-actuator.md,
pr9369-split-plan.md, power-agent-helm-chart-plan.md) still reference
the old paths in their historical / architectural narrative sections,
which is intentional -- those describe the pre-chart state and the
transition rationale, not current deployment instructions.

Part of the PR #9369 cascade following PR #9682''s Helm chart landing.

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 20, 2026
Aligns docs/design-docs/power-agent-helm-chart-plan.md with the v1.1.0
chart that PR #9790 ships. The plan was authored for the NVML-only
v1.0.0 chart in PR #9682 and went stale once PR #9790 layered
agent.actuator + agent.dcgm.* + validateActuator + validateEnforce +
the helm-unittest suite on top.

Two changelog rows capture the refresh:
  v1.3 - first pass: 8 reviewer findings closed (3 blocking,
                     4 major, 2 medium, 1 low) on Status header,
                     values surface, dev ConfigMap recipe, image-tag
                     pinning, helm-unittest gating, internal-dev-doc
                     references, daemonset name, file/LOC accounting.
  v1.4 - second pass: 5 follow-up findings closed (2 major,
                     2 medium, 1 low) on Status self-contradiction,
                     stale section 4.2 dev-block comment, overstated
                     helm-unittest coverage prose, unrunnable
                     section 5.4 positive helm template overlays
                     (missing --set image.tag), and the lingering
                     filename reference in the v1.3 changelog.

No design reversal: every section 6 decision and the chart shape are
unchanged. Only the values surface (extended), helper set (extended
with two template-time validators), and validation-gate list
(helm-unittest now required) grew. Every claim in the refreshed
doc was verified against on-disk state: power_agent.py:706-804
for the 8-flag CLI surface, values.yaml for the dev-block recipe,
_helpers.tpl for the five-helper set, and the two
tests/validate_*_test.yaml files for the 24 enumerated unittest
cases.

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 20, 2026
…ical OCI form

Addresses three iterative PR9682 CodeRabbit review findings on the
chart's image-pinning surface. Bumps Chart.yaml 1.1.0 -> 1.2.0 (minor
per SemVer - image.digest is purely additive; existing tag-pinned
installs render byte-identically and need no change).

Round 1 (original PR9682 finding): image.tag=latest was rejected only
at the raw-manifest layer, not at the chart layer. helm template
--set image.tag=latest happily rendered nvcr.io/nvidia/dynamo/
power-agent:latest because validateImageTag only checked
"if not .Values.image.tag". Now case-insensitively rejects the literal
"latest" (LATEST, Latest, " latest " all fail) while still accepting
release tags that contain the substring (v1.0.0-latest-rc).

Round 2 (digest path rendered an invalid OCI reference): operators
were advised in values.yaml + README + tests to set image.tag=
sha256:abc... for digest pinning. This rendered repo:sha256:... -
INVALID per the OCI spec, which requires repo@sha256:... for digest
form (the second ":" in the rendered string parses as repo + tag
"sha256" with a stray suffix; kubelet manifest pulls fail with an
opaque error). Three changes close this:
  - values.yaml: add image.digest field with per-rule comment
    block explaining the OCI form, the 64-hex requirement, the
    mutex with image.tag, and the PR #9682 rationale.
  - _helpers.tpl: split validateImageTag into a full rule set
    (XOR(tag, digest), whitespace rejection on both, latest
    rejection on tag, sha256: prefix rejection on tag, exact-
    64-hex regex on digest) and add a new imageRef helper that
    emits {repo}@{digest} when digest is set, else {repo}:{tag}.
  - daemonset.yaml: image: line now routes through imageRef
    instead of hard-coding the ":" join, so the canonical OCI
    form is the only thing that can ship.

Round 3 (validator was too lenient):
  - Truncated SHA-256 digests slipped through: regex was
    ^sha256:[0-9a-fA-F]{32,}$, so half-digests (a common pattern
    when an operator copies the first 8 bytes from
    `docker images --digests` output) rendered as
    repo@sha256:<truncated>. The kubelet then fails the pull with
    an opaque manifest-mismatch error. Tightened to {64} exactly
    (SHA-256 is 32 bytes x 2 nybbles).
  - Whitespace-padded values rendered raw: --set-string
    'image.tag= v1.1.0 ' produced image: "repo: v1.1.0 " because
    the latest comparison trimmed but the renderer used the raw
    value. Choice: reject (don't silently normalize) so the
    operator sees their --set quoting typo. Applies symmetrically
    to image.digest.

tests/validate_image_tag_test.yaml (new, 22 cases - chart
helm-unittest count grew 24 -> 46):
- Negative: unset (both empty), latest in 4 case variants,
  whitespace on tag (leading/trailing/both), tag+digest mutex,
  sha256: prefix on tag, non-digest on digest, 32/63/65-hex digest
  (off-by-one + half-length), whitespace on digest.
- Positive: pinned release tag renders {repo}:{tag};
  v1.0.0-latest-rc accepted (substring match guard); 64-hex digest
  renders the canonical {repo}@sha256:... form; uppercase-hex
  digest preserved verbatim.

README updates: production-install section now shows both tag and
digest invocations side by side with a callout that digests live
on image.digest, not image.tag. Values table gains the image.digest
row. Troubleshooting section adds three new error-message entries
covering missing-pin, invalid-digest, and mutex-violation cases.

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 20, 2026
Aligns docs/design-docs/power-agent-helm-chart-plan.md with the
v1.2.0 chart that the preceding commit ships. The plan was authored
for the v1.0.0 NVML-only chart in PR #9682, refreshed to v1.1.0 in
the PR #9790 dual-actuator work, and went stale again once v1.2.0
landed image.digest + the canonical-OCI imageRef helper on top.

Four reviewer findings closed (v1.5 changelog row covers all four):

(1) Truncated SHA-256 digests slipped through the v1.1.0 helper -
already fixed in the preceding chart commit; this commit propagates
the rule into the §4.4 helper snippet and the §5.4 validation gates.

(2) Whitespace-padded tags rendered raw image references - same
treatment: §4.4 documents the trim-then-reject contract.

(3) §4.3 / §4.4 / §5.4 stale on image.digest:
  - §4.3 CodeRabbit-comment row rewritten to describe both
    helpers (validateImageTag + imageRef), both fields
    (tag + digest), the canonical repo@digest form, and the
    PR9682 follow-up that added the separate field.
  - §4.4 helper snippet replaced with the actual v1.2.0
    validator (full rule set + per-rule rationale comments)
    plus the new imageRef helper.
  - §5.4 expected helm-unittest output bumped 24 -> 46 passed
    with a one-line breakdown of the +22 new cases.

(4) §4.1 / §4.2 stale on file count + helper list + values surface:
  - §4.1 said 13 files (omitted .helmignore and the new
    validate_image_tag_test.yaml). Corrected to 14 files (7
    templates + 3 helm-unittests + 4 root files). The _helpers.tpl
    helper-list line gained imageRef and chart entries. The
    daemonset.yaml annotation now mentions it routes through
    imageRef. LOC estimate bumped ~1,430 -> ~1,700 with a
    breakdown of what v1.2.0 added on top of v1.0.0 / v1.1.0.
  - §4.2 values snippet had only image.tag with no image.digest.
    Added the field with the same per-rule comment block that
    ships in values.yaml (OCI form, 64-hex requirement, mutex
    with image.tag, PR9682 rationale).

Status header bumped to chart v1.2.0 with a one-line v1.2.0
rationale (additive opt-in field, no breaking change to existing
tag-pinned installs). Revision-history table gains the v1.5 entry
summarising all four findings; older v1.1 - v1.4 entries
untouched.

No design reversal: every §6 decision and the chart shape are
unchanged. Only the values surface (one additive field), helper
set (two helpers: imageRef new, validateImageTag rewritten), and
helm-unittest count (24 -> 46) grew. Every claim in the refreshed
doc was verified against on-disk state: Chart.yaml for the
version bump, values.yaml for the image.digest field comment,
_helpers.tpl for both helpers, tests/validate_image_tag_test.yaml
for the 22 enumerated cases (verified via `helm unittest`).

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 22, 2026
When rbac.namespaceRestricted=true the chart renders a namespace-scoped
Role + RoleBinding (templates/role.yaml + rolebinding.yaml) instead of
the cluster-scoped pair, but the DaemonSet's container command never
passes --namespace to the agent. power_agent.py:438-446 therefore takes
its else branch — list_pod_for_all_namespaces — which the namespace-only
token rejects with 403 Forbidden on every reconcile. Reconciles never
advance and no caps are ever applied (silent failure: the agent doesn't
exit; pods are just never enumerated).

The --namespace CLI flag itself already exists (power_agent.py:542) and
the list_namespaced_pod branch (:439-442) works correctly — the dev-pod
template (templates/dev-pod.yaml:57) has been wiring this all along via
--namespace=$POD_NAMESPACE. This commit extends the same downward-API
POD_NAMESPACE env-var pattern to the production DaemonSet, gated on the
existing power-agent.effectiveNamespaceRestricted helper so both:

  - production mode with rbac.namespaceRestricted=true
  - dev mode (forces effective=true unless overridden)

produce a DaemonSet whose argv matches the RBAC scope its token holds.

Verified by helm template against both modes:
  - default (false) → no --namespace, no POD_NAMESPACE, ClusterRole
  - true            → --namespace=$(POD_NAMESPACE), POD_NAMESPACE env
                      via downward API, Role + RoleBinding
  - helm lint passes in both modes

Refs: PR #9682 review, Power Agent live-test finding #3.
Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 22, 2026
Steady-state RSS of the agent (pynvml + kubernetes client + prometheus
exporter) is ~80 MiB on an idle 8-GPU node — the 128Mi default left
only ~48 MiB of headroom, breaking under both routine inspection
(`kubectl exec --container=power-agent -- python -c "..."` re-imports
the kubernetes client in the same cgroup; observed RSS spike > 128 MiB,
two OOM-kills during 2026-05-21 live testing on AKS dpp-dev-env) and
transient peaks in multi-pod conflict resolution where the UID →
annotation dict + PID → UUID map are both held in memory simultaneously
(linear in pod count per node).

Bump:
  limits.memory:   128Mi → 256Mi   (~3× steady-state RSS headroom)
  requests.memory:  64Mi →  96Mi   (more honest scheduling floor)

CPU envelope unchanged — agent reconciles every 15 s and the heavy
calls (pynvml + list_pod_for_all_namespaces) are well under 200m on
fleet inspection (median: 12 m, p99: 78 m).

Fleet cost: at 1000 GPU nodes the extra 128 MiB per node is 125 GiB,
which is negligible compared to the operational cost of a silently
OOM-killed power-cap controller (the failure mode is a node whose
caps slowly drift back to safeDefaultWatts on the next reconcile that
doesn't fit the budget, with no obvious alert signal — the agent's
Pod restart counter is the only indicator).

Verified: helm template + helm lint clean.

Refs: PR #9682 review, Power Agent live-test finding #4.
Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 25, 2026
Folds deploy/power_agent/{daemonset,rbac,dev-pod}.yaml into a single
Helm chart at deploy/helm/charts/power-agent/, resolving the three
CodeRabbit findings on PR #9682:

  * hardcoded metadata.namespace=default -> {{ .Release.Namespace }}
  * mutable image :latest -> required image.tag with fail-fast validator
  * `${POWER_AGENT_NAMESPACE}` envsubst placeholder -> native Helm templating

The chart supports three deployment shapes selectable via values:
production DaemonSet (default, cluster-wide RBAC), namespace-restricted
production (Role+RoleBinding), and an in-cluster dev-iteration Pod
mounting power_agent.py from a ConfigMap. Three template-time validators
reject foot-guns at install time: empty image.tag, mutex violations
between daemonset.enabled and dev.enabled, and dev mode without a
pinned dev.nodeName. Dev mode also automatically forces namespace-scoped
RBAC (least privilege), leveraging power_agent.py's --namespace flag.

Design rationale, scope decisions, and review-feedback responses are
captured in docs/design-docs/power-agent-helm-chart-plan.md, committed
alongside the chart.

components/power_agent/README.md flips its install recipe to
``helm install``, and the planner CI filter (.github/filters.yaml) is
retargeted from deploy/power_agent/** to deploy/helm/charts/power-agent/**.
Two examples/deployments/powerplanner/*.yaml header references live on
PR #9687 and will be updated during that PR's cascade rebase per plan
section 5.3.

Validated locally:
  helm lint                                -> 0 errors
  helm template (3 positive exercises)     -> expected resources
  helm template (3 negative exercises)     -> expected fail-fast errors
  components/power_agent/tests/            -> 43/43 passed
  .github/scripts/test-filters.js          -> 20/20 passed
  pre-commit (cross-cutting hooks)         -> all applicable passed

Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 25, 2026
When rbac.namespaceRestricted=true the chart renders a namespace-scoped
Role + RoleBinding (templates/role.yaml + rolebinding.yaml) instead of
the cluster-scoped pair, but the DaemonSet's container command never
passes --namespace to the agent. power_agent.py:438-446 therefore takes
its else branch — list_pod_for_all_namespaces — which the namespace-only
token rejects with 403 Forbidden on every reconcile. Reconciles never
advance and no caps are ever applied (silent failure: the agent doesn't
exit; pods are just never enumerated).

The --namespace CLI flag itself already exists (power_agent.py:542) and
the list_namespaced_pod branch (:439-442) works correctly — the dev-pod
template (templates/dev-pod.yaml:57) has been wiring this all along via
--namespace=$POD_NAMESPACE. This commit extends the same downward-API
POD_NAMESPACE env-var pattern to the production DaemonSet, gated on the
existing power-agent.effectiveNamespaceRestricted helper so both:

  - production mode with rbac.namespaceRestricted=true
  - dev mode (forces effective=true unless overridden)

produce a DaemonSet whose argv matches the RBAC scope its token holds.

Verified by helm template against both modes:
  - default (false) → no --namespace, no POD_NAMESPACE, ClusterRole
  - true            → --namespace=$(POD_NAMESPACE), POD_NAMESPACE env
                      via downward API, Role + RoleBinding
  - helm lint passes in both modes

Refs: PR #9682 review, Power Agent live-test finding #3.
Signed-off-by: Kai Ma <kaim@nvidia.com>
kaim-eng added a commit that referenced this pull request May 25, 2026
Steady-state RSS of the agent (pynvml + kubernetes client + prometheus
exporter) is ~80 MiB on an idle 8-GPU node — the 128Mi default left
only ~48 MiB of headroom, breaking under both routine inspection
(`kubectl exec --container=power-agent -- python -c "..."` re-imports
the kubernetes client in the same cgroup; observed RSS spike > 128 MiB,
two OOM-kills during 2026-05-21 live testing on AKS dpp-dev-env) and
transient peaks in multi-pod conflict resolution where the UID →
annotation dict + PID → UUID map are both held in memory simultaneously
(linear in pod count per node).

Bump:
  limits.memory:   128Mi → 256Mi   (~3× steady-state RSS headroom)
  requests.memory:  64Mi →  96Mi   (more honest scheduling floor)

CPU envelope unchanged — agent reconciles every 15 s and the heavy
calls (pynvml + list_pod_for_all_namespaces) are well under 200m on
fleet inspection (median: 12 m, p99: 78 m).

Fleet cost: at 1000 GPU nodes the extra 128 MiB per node is 125 GiB,
which is negligible compared to the operational cost of a silently
OOM-killed power-cap controller (the failure mode is a node whose
caps slowly drift back to safeDefaultWatts on the next reconcile that
doesn't fit the budget, with no obvious alert signal — the agent's
Pod restart counter is the only indicator).

Verified: helm template + helm lint clean.

Refs: PR #9682 review, Power Agent live-test finding #4.
Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng
kaim-eng force-pushed the pr1a/power-agent branch from 0eda1d7 to 1e1ef6f Compare May 25, 2026 13:43
kaim-eng added a commit that referenced this pull request May 25, 2026
… log, pod-UID dedup, argparse)

Folds the four CodeRabbit findings on the foundation PR back into this branch so each fix lives with the code it changes, rather than leaking into the downstream DCGM-actuator PR (#9790):

* _load_previously_managed_gpus: catch OSError (not just FileNotFoundError) and validate that the JSON root is a dict and managed_uuids is a list. Malformed state files now log a warning and return an empty set instead of crashing the agent at startup.

* _handle_sigterm: replace 'except Exception: pass' on pynvml.nvmlShutdown() with logger.exception so shutdown-time NVML faults appear in pod logs. We still fall through to _shutdown.set() so SIGTERM never hangs the container.

* _reconcile_gpu: dedup the (pod_uid, annotation) list by UID before applying multi-pod policy. A single pod with N PIDs on one GPU was being counted as N pods, falsely tripping the multi-pod-conflict branch and the multi_pod_gpu_total metric.

* main(): move 'import argparse' to module scope per the project's import-placement convention.

Regression coverage: existing components/power_agent/tests/ suite (43 tests) still passes locally; behavior-specific tests for these four fixes already live on PR #9790 and remain there.

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng

Copy link
Copy Markdown
Contributor Author

@nv-anants thanks for the review — all addressed in a65126fb88.

# Comment Resolution
1 Dockerfile: switch to python 3.12 Base is now python:3.12-slim-bookworm. This also fixed the Power Agent build failure in the /ok to test run: the ECR Docker Hub mirror returned 403 Forbidden on python:3.11-slim-bookworm — it only serves 3.12 (matches every other python Dockerfile in the repo), so 3.11 literally couldn't build here.
2 add both test jobs to post-merge power-agent-test + power-agent-helm-tests are now in post-merge-ci.yml too, so post-merge runs the same gates as PR CI.
3 avoid release.yml changes for now Reverted entirely — release.yml now has a zero diff vs main. Happy to start the public-release-artifact process separately with @dagil-nvidia when the time comes.
4 run the tests inside the built image Done. The Dockerfile gained a test stage (FROM runtime + pytest) whose RUN python -m pytest tests -v validates the exact shipped layers (same Python 3.12 + baked pynvml/kubernetes/prometheus-client). The power-agent-test job is now just docker build --target test …; the build/push job passes --target runtime so only the slim stage is published. This also fixed the Power Agent Tests failure (the old runner-env approach hit ModuleNotFoundError: No module named 'pydantic' from the monorepo-root filterwarnings=error config — the in-image stage + a standalone deploy/power-agent/pytest.ini rootdir avoid that).
6 name both test jobs Power Agent Both test jobs are now name: Power Agent, so all three group under one umbrella in the checks UI.

(#5 was the /ok to test trigger — thanks. That run was on 76169f1d which predates these fixes; this push should clear the two red Power Agent jobs. Mind re-running /ok to test on a65126fb88 when you get a chance?)

@nv-anants

Copy link
Copy Markdown
Member

/ok to test a65126f

Comment thread deploy/power-agent/Dockerfile Outdated
ECR's Docker Hub pull-through cache does not auto-expand bare official
image names (python -> library/python) the way a direct Docker Hub pull
does, so the DOCKER_PROXY-prefixed base image fails to pull in CI.
Spelling out library/python fixes the proxied pull and remains a valid
fully-qualified reference when DOCKER_PROXY is empty (local/dev builds
unaffected).

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng
kaim-eng temporarily deployed to external_collaborator June 24, 2026 16:12 — with GitHub Actions Inactive
@nv-anants

Copy link
Copy Markdown
Member

/ok to test a12c61d

…ction

Merge power-agent, power-agent-test, and power-agent-helm-tests into a single power-agent job (build -> unit tests -> helm lint/render) so the PR shows one "Power Agent" check. Three same-named inline jobs do not nest the way reusable-workflow jobs (e.g. vllm-runtime) do, so they previously rendered as three separate checks.

Add an optional push input (default true) to build-deploy-component so the test step builds the Dockerfile test target through the same action: routes the base image through the ECR Docker Hub mirror and reuses the runtime builder cache, while skipping the push for the never-shipped test stage.

Drop the removed jobs from backend-status-check needs.

Signed-off-by: Anant Sharma <anants@nvidia.com>
Calling build-deploy-component a second time for the test re-ran the builder bootstrap and failed with "existing instance ... no append mode". Build the test target directly with docker buildx build on the builder the runtime build already set up, routing the base image through the ECR Docker Hub mirror and reusing its layer cache.

Insert a builder-refresher step between the runtime and test builds (matching shared-build-image.yml) so a stale remote BuildKit connection is repaired before the test build.

Revert the unused push input added to build-deploy-component.

Signed-off-by: Anant Sharma <anants@nvidia.com>
The power-agent CI runs `pytest tests` with no -m filter, so the pre_merge/gpu_0/unit marks never selected anything. Remove the pytestmark lines (and the now-unused pytest import) and the marker registry from pytest.ini, and fix a stale power-agent-test job reference in the pytest.ini comment.

Signed-off-by: Anant Sharma <anants@nvidia.com>

@nv-anants nv-anants left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for all the changes @kaim-eng . I am still seeing some issues in CI, I have added a PR here to fix them - #10929

Feel free to merge it in here or make similar changes in your PR.

@kaim-eng
kaim-eng temporarily deployed to external_collaborator June 24, 2026 17:06 — with GitHub Actions Inactive
@kaim-eng

Copy link
Copy Markdown
Contributor Author

Thanks @nv-anants — merged your #10929 commits straight onto pr1a/power-agent (fast-forward, so authorship is preserved): c735aeff12, 3743a2ad4d, 89ced1eeb8, now on top of my library/python fix.

I see what was actually breaking CI now: the standalone docker build --target test step pulled library/python from Docker Hub directly (no DOCKER_PROXY), so it never went through the ECR mirror — your change routes the test build through ${ECR_HOSTNAME}/dockerhub/ on the same builder/cache, which is the real fix. Collapsing the three same-named Power Agent jobs into one sequential job (build → in-image pytest → helm lint/render) and dropping the unused marker registrations all make sense too.

Validated locally before pushing:

  • docker build --target test57 passed in-image (marker removal is clean under --strict-markers).
  • helm lint + helm template on the power-agent chart → green.

Since the commits are now on this branch, #10929 should auto-close on the next sync — feel free to close it if not.

deploy/power-agent/tests/ ships __init__.py, so it is a package literally
named "tests" -- the same name as the repo-root tests/ package. During the
repo-wide dynamo-runtime pytest collection (rootdir = repo root), Python has
already registered "tests" as the root package, so importing
tests.test_annotation_scope resolves into the root package and raises
ModuleNotFoundError. This surfaced as 6 collection errors that failed
dynamo-runtime test jobs and the backend/dynamo status-check aggregators.

The power-agent suite is a standalone suite run in-image (Dockerfile `test`
stage + its own deploy/power-agent/pytest.ini), not part of the repo-wide
run, so add it to the existing addopts ignore list -- matching the
established pattern for avoiding duplicate-module collection errors.

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng
kaim-eng temporarily deployed to external_collaborator June 24, 2026 17:31 — with GitHub Actions Inactive
@nv-anants

Copy link
Copy Markdown
Member

/ok to test 2da2530

@kaim-eng

Copy link
Copy Markdown
Contributor Author

@nv-anants CI is fully green now — should be good to approve/merge.

After merging your #10929 commits I traced the last remaining red, which #10929 didn't cover. The repo-wide dynamo-runtime pytest run (rootdir = repo root) was hitting 6 collection errors:

ERROR collecting deploy/power-agent/tests/test_annotation_scope.py
  ModuleNotFoundError: No module named 'tests.test_annotation_scope'
  (+ test_apply_cap, test_cgroup_parser, test_multi_pod_policy,
     test_reconcile_failsafe, test_shutdown)

Root cause: deploy/power-agent/tests/__init__.py makes that dir a package named tests, which collides with the repo-root tests/ package during whole-repo collection — so import tests.test_annotation_scope resolves into the root package and raises ModuleNotFoundError. That failed the dynamo-runtime test jobs and the dynamo-status-check / backend-status-check aggregators.

Fix (commit 2da2530272): added --ignore-glob=deploy/power-agent/tests/* to the existing addopts ignore list in pyproject.toml — same pattern already used there to "avoid duplicate-module collection errors". The power-agent suite still runs in full via the Dockerfile test stage in the Power Agent job (57 tests), so no coverage is lost; this only stops the repo-wide run from double-collecting it.

Validation:

  • Reproduced the collision + confirmed the ignore resolves it in a minimal repro, and against the real pyproject.toml config.
  • Full pipeline now: 90 success / 12 skipped / 0 failing. The previously-red dynamo-runtime / test / parallel|gpu|sequential + both status-check aggregators are green (1829 passed on the runtime suite — it now runs instead of crashing at collection).
  • Two unrelated flakes blipped mid-run (test_indexers_sync[file] router event-count race on arm64, and a vLLM DynamoCheckpoint deploy test) and both passed on auto-retry — neither touches power-agent.

Only thing left is a maintainer approval (branch shows REVIEW_REQUIRED). Thanks for the help getting the CI structure sorted.

Remove the design doc and the component/chart READMEs from this PR.
The power-planner feature (#9682-#9687, #9790) is only an end-to-end
working feature once the full stack lands, so the user-facing docs will
be (re)introduced in a single dedicated docs PR after the series merges,
rather than documenting a partially-wired feature here.

Removed:
- docs/design-docs/power-agent-helm-chart-plan.md
- deploy/power-agent/README.md
- deploy/helm/charts/power-agent/README.md

Cleaned dangling references in Chart.yaml, values.yaml, NOTES.txt, and
pytest.ini comments. No functional change: helm lint + prod/dev render
clean; in-image pytest config unaffected.

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng
kaim-eng temporarily deployed to external_collaborator June 24, 2026 20:57 — with GitHub Actions Inactive
@nv-anants

Copy link
Copy Markdown
Member

/ok to test 89f58ef

@nv-anants
nv-anants merged commit 82f5389 into main Jun 25, 2026
103 checks passed
@nv-anants
nv-anants deleted the pr1a/power-agent branch June 25, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actions deployment::k8s Relates to dynamo deployment in kubernetes documentation Improvements or additions to documentation feat size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants