refactor(k8s): decompose kubernetes.py into focused mixin modules by coketaste · Pull Request #120 · ROCm/madengine

coketaste · 2026-05-01T21:34:31Z

Summary

Decompose kubernetes.py (3,535 lines) into 4 focused mixin modules using the same pattern as the existing KubernetesLauncherMixin, reducing the main file from 3,535 to 778 lines
No behavioral changes — pure structural refactoring with all 47 original methods preserved

Motivation

kubernetes.py had grown to 3,535 lines with 40+ methods spanning 5+ distinct responsibilities, making it difficult to navigate and review. The largest single method
(_prepare_template_context) was 538 lines.

New modules

┌───────────────────────────┬───────┬───────────────────────────────────────────────────────────────────┐
│          Module           │ Lines │                          Responsibility                           │
├───────────────────────────┼───────┼───────────────────────────────────────────────────────────────────┤
│ k8s_pvc.py                │   239 │ PVC lifecycle — create, delete, storage class resolution          │
├───────────────────────────┼───────┼───────────────────────────────────────────────────────────────────┤
│ k8s_scripts.py            │   352 │ Script/tool loading for ConfigMap embedding                       │
├───────────────────────────┼───────┼───────────────────────────────────────────────────────────────────┤
│ k8s_template_context.py   │   858 │ Jinja2 template context preparation, env vars, tools config       │
├───────────────────────────┼───────┼───────────────────────────────────────────────────────────────────┤
│ k8s_results.py            │ 1,389 │ Result collection, CSV parsing, perf record building              │
├───────────────────────────┼───────┼───────────────────────────────────────────────────────────────────┤
│ kubernetes.py (remaining) │   778 │ Core lifecycle: init, validate, prepare, deploy, monitor, cleanup │
└───────────────────────────┴───────┴───────────────────────────────────────────────────────────────────┘

MRO

KubernetesDeployment → KubernetesLauncherMixin → KubernetesResultsMixin
→ KubernetesTemplateContextMixin → KubernetesScriptsMixin
→ KubernetesPVCMixin → BaseDeployment → ABC → object

Test plan

All 426 unit tests pass (including 24 K8s-specific)
All 139 integration tests pass
Python syntax validated for all 5 files
Import chain and MRO verified
Zero methods lost or duplicated (47/47 accounted for)
Deployment public API (DeploymentFactory, KubernetesDeployment) unchanged
Flake8 clean (no new lint issues introduced)

Split the 3,535-line kubernetes.py into 6 focused files using the mixin pattern (same approach as existing KubernetesLauncherMixin): - kubernetes.py (779 lines): core lifecycle (init, validate, prepare, deploy, monitor, cleanup) - k8s_results.py (1,390 lines): results collection, PVC artifacts, perf CSV reporting - k8s_template_context.py (856 lines): Jinja2 context, env vars, data config, tools - k8s_scripts.py (352 lines): script/tool loading for ConfigMap embedding - k8s_pvc.py (239 lines): PVC lifecycle management All 426 unit tests and 139 integration tests pass unchanged. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Remove unused imports (re, sanitize_k8s_container_name), fix blank lines before nested class methods, and strip trailing whitespace. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Copilot

Pull request overview

Refactors the Kubernetes deployment implementation by decomposing the previously monolithic kubernetes.py into focused mixin modules, keeping the public deployment API intact while improving navigability and separation of concerns.

Changes:

Split Kubernetes deployment responsibilities into new mixins for PVC lifecycle, script/tool bundling, template context building, and results collection/reporting.
Updated KubernetesDeployment to compose these mixins while leaving core lifecycle methods in kubernetes.py.
Reduced kubernetes.py to primarily orchestration logic (init/validate/prepare/deploy/monitor/cleanup).

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`src/madengine/deployment/kubernetes.py`	Switches `KubernetesDeployment` to a mixin-based composition and removes inlined implementations moved to mixin modules.
`src/madengine/deployment/k8s_template_context.py`	New mixin for building the Jinja2 template context, env var merging, data config, and tools config enrichment.
`src/madengine/deployment/k8s_scripts.py`	New mixin for bundling common scripts/tool wrappers (and Primus overlays) into ConfigMaps.
`src/madengine/deployment/k8s_results.py`	New mixin for pod log/PVC artifact collection, performance parsing/aggregation, and perf reporting outputs.
`src/madengine/deployment/k8s_pvc.py`	New mixin for results/data PVC storage class resolution and PVC create/delete workflows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Promote Unreleased to [2.0.3], add K8s mixin refactor entry, and document known issue where a node is reported FAILED due to kubelet log collection errors despite the pod succeeding. Add corresponding troubleshooting section to deployment.md. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Cleanup used the full job name while creation truncated to 15 chars, so stale collector pods were never deleted for jobs with names longer than 15 characters. Extract a shared collector_pod_name() helper in k8s_results.py and use it in both sites. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

+                    # Import Context and create minimal instance
+                    # Data provider needs this to function
+                    self.context_for_data = type('obj', (object,), {
+                        'ctx': {},
+                        'sh': lambda cmd: os.popen(cmd).read().strip()


+            pod_names: Full Kubernetes pod names for this job (ordered)
+        """
+        from .kubernetes import assign_pvc_subdirs_to_pods
+
+        pvc_name = f"{deployment_id}-results"


+To verify:
+```bash
+# Check actual pod status — if Succeeded, the workload ran fine
+kubectl describe pod <pod-name> | grep Status
+
+# Check the node's kubelet health
+kubectl get nodes
+kubectl describe node <node-name> | grep -A5 Conditions
+```


 - **Profiling**: `rocm_trace_lite` now sets `RTL_MODE=lite` explicitly; added tool `rocm_trace_lite_default` with `RTL_MODE=default` for A/B overhead comparison. `rtl_trace_wrapper.sh` passes `rtl trace --mode …` when `RTL_MODE` is set.

+- **Kubernetes deployment refactor**: Decomposed the monolithic `kubernetes.py` (~2800 lines) into focused mixin modules — `k8s_pvc.py` (PVC lifecycle), `k8s_results.py` (log/artifact collection and performance aggregation), `k8s_scripts.py` (script extraction and ConfigMap building), and `k8s_template_context.py` (Jinja2 template context assembly). `KubernetesDeployment` now composes these mixins; no functional changes.
+
+### Fixed
+
+- **K8s collector pod name mismatch**: The cleanup code in `kubernetes.py` used the full job name (`collector-{job_name}`) while the creation code in `k8s_results.py` truncated it (`collector-{deployment_id[:15]}`). For any job name longer than 15 characters (i.e. virtually all real jobs), cleanup would fail to delete the collector pod, leaving it running and potentially blocking PVC deletion on the next deploy. Extracted a shared `collector_pod_name()` helper so both sites use the same truncated name.


coketaste and others added 2 commits May 1, 2026 20:05

style(k8s): fix lint issues introduced by mixin refactor

a40da99

Remove unused imports (re, sanitize_k8s_container_name), fix blank lines before nested class methods, and strip trailing whitespace. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

coketaste self-assigned this May 1, 2026

Copilot AI review requested due to automatic review settings May 1, 2026 21:34

coketaste requested review from Cemberk, Rohan138, gargrahul and leconcio as code owners May 1, 2026 21:34

Copilot started reviewing on behalf of coketaste May 1, 2026 21:35 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/madengine/deployment/kubernetes.py

coketaste and others added 2 commits May 6, 2026 20:07

Copilot AI review requested due to automatic review settings May 6, 2026 20:36

coketaste merged commit aa94fb7 into develop May 6, 2026
2 checks passed

Copilot started reviewing on behalf of coketaste May 6, 2026 20:37 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(k8s): decompose kubernetes.py into focused mixin modules#120

refactor(k8s): decompose kubernetes.py into focused mixin modules#120
coketaste merged 4 commits into
developfrom
coketaste/v2-k8s-refactor

coketaste commented May 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

coketaste commented May 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants