feat(cli): add cleanup subcommand to remove run leftovers from the namespace - #1801
feat(cli): add cleanup subcommand to remove run leftovers from the namespace#1801himanshu1573 wants to merge 1 commit into
Conversation
|
This looks and sounds like a great and well needed addition. Before we continue, please address the erroring CI CICD is failing on multiple jobs with similar error: Can you try to reproduce this locally and resolve this issue? I will |
| # at one command instead of maintaining their own `kubectl delete` list. | ||
| DATA_ACCESS_POD_LABEL = "role=llm-d-benchmark-data-access" | ||
| HARNESS_LAUNCHER_POD_LABEL = "app=llmdbench-harness-launcher" | ||
| HARNESS_SERVICE = "llm-d-benchmark-harness" |
There was a problem hiding this comment.
I believe a lot of these parameters (from line 18 to 27) today are able to be configured from the scenario file we specify for run.
This should also be the case here. Having them purely hardcoded will not be feasible. I do see a cli option, but having consistency with how we specify parameters for commans is key.
Can you confirm?
There was a problem hiding this comment.
Confirmed — you're right for most of this block. Checked each name for an actual config knob:
Scenario-configurable (fixing these):
| Constant | Config path | Rendered at |
|---|---|---|
HARNESS_LAUNCHER_POD_LABEL |
harness.podLabel |
20_harness_pod.yaml.j2:12 |
WORKLOAD_PVC |
storage.workloadPvc.name |
01_pvc_workload-pvc.yaml.j2:4 |
HARNESS_SERVICE |
labels.app |
07_service_access_to_harness_data.yaml.j2:4 |
*-profiles CM |
harness.name |
step_11_cleanup_post.py:63 |
No knob today (literal in the templates/code, so matching them by name is correct): role=llm-d-benchmark-data-access (06_*.j2:7), the three llm-d-benchmark-*-parameters/-preprocesses CMs (cli.py:1254), and llmdbench-harness-scripts (step_06_create_profile_configmap.py:9). Can make those configurable too, but that's a render-side change — happier keeping it out of this PR.
Also worth flagging: the resources split across namespaces. Launcher pods render into harness.namespace | default(namespace.name) (20_harness_pod.yaml.j2:10), while the PVC/service/data-access pod go to namespace.name (01_*.j2:5, 06_*.j2:8, 07_*.j2:5) — so a single --namespace can't reach everything when a scenario sets harness.namespace. Another reason to read this from the scenario.
Plan: cleanup takes --spec (already a top-level arg), resolves the merged config via RenderSpecification.eval(), and derives namespaces + those four values from it — same three-tier precedence step_11_cleanup_post.py already uses (executor/step.py:157): CLI flag > scenario > default. Plus tests for the overrides and precedence.
One question before I push: required or optional --spec? I left it out originally for the recovery case in #1789 — cleaning up after the scenario is gone, or after a standup failed partway. Required is more consistent with the other subcommands; optional keeps that path working. I lean optional, but your call.
There was a problem hiding this comment.
@himanshu1573 it should be required and used just as it is used in other subcommands. If you migrate those global variabels to the scenario or cli, that would be needed to be referenced in the actual scenario.
Unless did you somehow make cleanup not need --spec if so, how does that work?
There was a problem hiding this comment.
@Vezio Nothing clever — cleanup doesn't need --spec today because it never resolves anything. It's a flat kubectl pass over hardcoded values: pods by literal label (app=llmdbench-harness-launcher, role=llm-d-benchmark-data-access), the service and the four llm-d-benchmark-* / llmdbench-harness-scripts ConfigMaps by literal name, the profile CM by suffix scan (delete any CM ending in -profiles), and pvc/workload-pvc overridable only via --pvc-name. Each is a get --ignore-not-found then delete, so it no-ops on anything absent. That only "works" for scenarios that override none of those names — one that sets harness.podLabel, storage.workloadPvc.name, labels.app or harness.name silently leaves resources behind. Which is your point.
So: agreed, --spec becomes required and is used as the other subcommands use it. I'll:
- resolve via
RenderSpecification.eval()and deriveharness.podLabel,storage.workloadPvc.name,labels.appand{harness.name}-profilesfrom it, with the same precedencestep_11_cleanup_post.pyuses (executor/step.py:157): CLI flag > scenario > default - replace the
-profilessuffix scan with the exact name - clean both namespaces — launcher pods render into
harness.namespace | default(namespace.name)(20_harness_pod.yaml.j2:10), the PVC/service/data-access pod intonamespace.name, so one--namespacecan't reach both - add tests for the overrides and the precedence
On role=llm-d-benchmark-data-access (06_*.j2:7), the three llm-d-benchmark-*-parameters/-preprocesses CMs (cli.py:1254) and llmdbench-harness-scripts (step_06_create_profile_configmap.py:9): no scenario key exists for these anywhere — they're literal in the templates and render code, so adding keys touches the render path for every command, not just cleanup. I'd keep matching them by name here and do that separately; say the word if you want it in this PR.
One consequence: requiring --spec closes the recovery case from #1789 (cleaning up after the scenario is gone, or a standup that died partway) — that path now needs the original scenario in hand. I'll note it in the subcommand help.
CI is green on the current head after the rebase onto e525662.
There was a problem hiding this comment.
@Vezio Pushed. --spec is required now and cleanup goes through the same path as the other subcommands: it renders the scenario and reads the names from the plan config.
What comes from the scenario now: harness.podLabel, harness.name (for the *-profiles ConfigMap), labels.app (the service) and storage.workloadPvc.name. Precedence is CLI flag > scenario > default, same as executor/step.py. I also dropped the old -profiles suffix scan, it uses the exact name now.
It also cleans both namespaces. Launcher pods and their ConfigMaps go to harness.namespace | default(namespace.name), the PVC, service and data-access pod go to namespace.name, so one namespace was not enough. -p model-ns,harness-ns works like it does for teardown.
The data-access pod label and the llm-d-benchmark-* / llmdbench-harness-scripts ConfigMaps are still matched by name, since there is no scenario key for them anywhere yet. Happy to add keys for those in a follow-up if you want them configurable.
Tests: 14 cases covering the scenario overrides, the defaults, the CLI override and the two-namespace case. Full suite and ruff are green locally.
There was a problem hiding this comment.
I've added a new round of comments - several items from the previous review are still outstanding.
There was a problem hiding this comment.
@Vezio Pushed. cleanup is now a proper Step, same as teardown and run.
What changed
interface/cleanup.pyis argparse only (404 → 47 lines), like every other interface module.- The logic moved to
cleanup/steps/step_00_cleanup_resources.pyasCleanupResourcesStep(Step), dispatched throughStepExecutorfrom a new_do_cleanup()incli.py(copied from_do_teardown()). - No fallbacks. Every name comes from the rendered
config.yamlviaStep._require_config():namespace.name,harness.podLabel,harness.name,labels.app,storage.workloadPvc.name. A missing key raises the framework's existing error instead of guessing. - No duplicated loaders.
load_stack_configs,_resolve,_delete_by_labelare gone — the step usesStep._load_stack_config()andkube_helpers.delete_pods_by_label()/find_data_access_pod()/delete_pods_by_names(). - Reads
context.rendered_stackslike the other commands, instead of rescanning the plan directory. - Dropped
--pvc-name.--set storage.workloadPvc.name=...already does this on every subcommand.
The llm-d-benchmark-* / llmdbench-harness-scripts ConfigMaps are still matched by name — they're created by name in the standup/run code and have no scenario key (same list teardown/step_02 and run/step_11 use).
The diff got larger than the last round because the restructure touches cli.py, executor/step.py (Phase.CLEANUP) and executor/context.py (keep_pvc), but net it's −42 lines. Tests rewritten around the step (16 cases), full suite and ruff green locally.
|
@himanshu1573 The breakage on the CI/CD was fixed, please rebase so we can continue with the review. Thanks. |
00e966e to
b5cf445
Compare
|
@maugustosilva Rebased onto @Vezio still keen on your take on the |
|
@himanshu1573 just responded above |
b5cf445 to
967c6cd
Compare
| HARNESS_SCRIPTS_CONFIGMAP, | ||
| ) | ||
|
|
||
| # Fallbacks for the scenario keys below, mirroring config/templates/values/ |
There was a problem hiding this comment.
I mentioned that these sections, namely the hardcoded fall backs, shouldn't exist here. They should be determined based on the scenario provided in the specification.
There was a problem hiding this comment.
Removed. The DEFAULT_* constants are gone; every name is now read from the rendered config.yaml with Step._require_config(), and a missing key raises instead of falling back.
| ) | ||
|
|
||
|
|
||
| def load_stack_configs(plan_dir: Path | None) -> list[dict]: |
There was a problem hiding this comment.
This seems duplicated since we already handle scenario parsing in the cli and many other utilities exist today to load in scenarios.
There was a problem hiding this comment.
Removed. load_stack_configs() is gone — the step now uses Step._load_stack_config() on context.rendered_stacks, the same as the teardown steps.
967c6cd to
f2b31b1
Compare
| ) | ||
|
|
||
| # Results command is handled separately | ||
| # Results reads the local store only -- no specification, no plans |
There was a problem hiding this comment.
I'm confused, why this comment change?
There was a problem hiding this comment.
No good reason — leftover noise from an earlier iteration, unrelated to cleanup. Reverted, the comment is back to the original.
The PR is now purely additive: 693 insertions, 0 deletions. Nothing existing is touched.
…mespace llmdbenchmark run tears down the harness launcher pod when a run finishes, but leaves the data-access pod, the harness service, the benchmark ConfigMaps and the workload PVC behind. The PVC keeps billing against its storage backend, a stale claim wedges retries after a wrong-StorageClass failure, and the only way out was a hand-written kubectl delete naming CLI-internal resource names. Add 'llmdbenchmark --spec <specification> cleanup [--keep-pvc]', implemented as a phase like teardown and run: - interface/cleanup.py is argparse only; the logic lives in cleanup/steps/step_00_cleanup_resources.py as CleanupResourcesStep and is dispatched through StepExecutor from _do_cleanup() in cli.py - every resource name is read from the rendered config.yaml with Step._require_config(): namespace.name, harness.podLabel, harness.name (the '*-profiles' ConfigMap), labels.app (the harness service) and storage.workloadPvc.name. There are no hardcoded fallbacks; a missing key raises. Overrides go through the same --set/--namespace path every other subcommand uses - reuses the framework helpers instead of re-implementing them: Step._load_stack_config(), kube_helpers.delete_pods_by_label(), find_data_access_pod() and delete_pods_by_names() - operates on context.rendered_stacks -- the stacks this invocation rendered -- rather than rescanning the plan directory - cleans both namespaces: launcher pods and their ConfigMaps render into 'harness.namespace | default(namespace.name)' while the PVC, service and data-access pod render into 'namespace.name' - the llm-d-benchmark-* and llmdbench-harness-scripts ConfigMaps are created by name in the standup/run code and have no scenario key, so they stay matched by name (same list teardown step_02 and run step_11 use) - the data-access pod is deleted before the PVC so the pvc-protection finalizer can clear - idempotent: every delete uses --ignore-not-found, and a namespace with no benchmark resources exits 0 - --keep-pvc preserves the workload PVC so workload data survives between runs Fixes: llm-d#1789 Signed-off-by: Himanshu Prajapati <himanshuprajapati15072003@gmail.com>
f2b31b1 to
25e37f5
Compare
|
Marked as Do Not Merge for now since a large portion of the discourse and code written is AI generated. Only tests completed have been static as documented in the PR: So, the code has not been tested on a real cluster (or simulation in CICD). At some point this week, I will take liberty to test these newly added code paths. |
|
Fair point on the cluster testing. You're right that I had only done static tests. I ran it on a kind cluster today. v1.35.0, default I rendered the First, does the delete order actually matter?In a throwaway namespace I deleted the PVC first, while the pod still had it mounted: So it hangs. Deleting the pod cleared it right away, which shows the pod was the thing holding it: Then the actual commandPod at After it ran, the cluster agrees. Also checked
One gap I should flag
This matches Scope of what I testedI built the leftovers by hand in a test namespace rather than running a full standup. So this proves the finalizer hang is real and that deleting pods before PVCs avoids it. It does not prove On the AI note, that is fair. I have been leaning on it for the write-ups. This one is mine. |
3 participants
What
Adds
llmdbenchmark cleanup --namespace <ns> [--keep-pvc], which removes everything a benchmark run leaves behind in the harness namespace:app=llmdbench-harness-launcher)role=llm-d-benchmark-data-access)service/llm-d-benchmark-harnessllm-d-benchmark-preprocesses,llm-d-benchmark-run-parameters,llm-d-benchmark-standup-parameters,llmdbench-harness-scripts, and any*-profilesworkload profile ConfigMapspvc/workload-pvc(skipped with--keep-pvc;--pvc-namecovers scenarios that overridestorage.workloadPvc.name)Why
Closes #1789.
runtears down the launcher pod but leaves the rest in the namespace: the PVC bills against its storage backend until deleted, the resource names are CLI internals that consuming docs have to hardcode, and a stale claim wedges retries after a wrong-StorageClass failure.Design notes
pvc-protectionfinalizer can clear instead of leaving the claim stuck in Terminating.cleanuponly needs--namespace(env:LLMDBENCH_NAMESPACE) — it deletes by name/label, so it works after the user has deleted their scenario or stack. It is dispatched likeresults, before plan rendering.llmdbenchmark/interface/cleanup.py), so llm-d'shelpers/benchmark.mdcan replace its hand-maintainedkubectl deletelist with this command. The issue also floats labeling every CLI-created resource (app.kubernetes.io/managed-by) and deleting purely by selector; happy to follow up with that if preferred, but name/label-based deletion keeps this PR small and works for resources created by older CLI versions too.Testing
tests/test_cleanup_command.py(full deletion set,--keep-pvc, empty-namespace no-op, pod-before-PVC ordering, custom PVC name) following the existing fake-executor style.ruff check/ruff format --check(0.15.11, as CI runs) clean.--dry-runexercises the exact kubectl commands end-to-end.Docs: added a section to
docs/lifecycle.mdafter teardown, and listedcleanup.pyin the README package layout.