[deckhouse-cli] Updating the debug archive and adding an archive for virtualization

[VaLosev]

In the current archive, which is collected with the command: d8 system collect-debug-info

• Renamed data collection files to group data from different components:
• Added a 3000-line limit for collecting logs from virtualization module controllers
• Added CRD collection from the cluster
• Fixed a bug related to collecting machine resources for CAPI. MCM machine has been moved to a separate file - instance-manager-mcm-cloud-machines.json
• The exec→tar cycle has been moved from Tarball() to a reusable runCommands so that it can be used in the new virtualization archive.

---

The ExpandPerModule bool field has been removed from Command; the decision to execute the command for all modules found in RequiredModule is now based on the presence of {module-name} in the File or Args fields. This ensures there is a single source of truth for this mechanism: the template itself.
A test has also been added to ensure that {module-name} is not used without RequiredModule.

---

Also added a separate command for collecting logs from all pods from ns - d8-virtualization: d8 system collect-debug-info virtualization
This was done because 3000-line logs are often insufficient for virtualization diagnostics, and logs from virt-handler pods, which are launched via DS on each node, are also very important - there can be many of them. (by number of nodes)

Collecting all these logs into the main archive could significantly increase its size, and the debug archive should remain a quick diagnostic tool so clients can quickly collect and send it.

So, in the event of virtualization issues, if the standard archive's logs are insufficient, a special virtualization archive can be requested.

The --command-timeout and --request-interval flags have been copied to this new archive, and a new --skip-ds-logs flag has been added to disable log collection from DS modules in the case of a large number of nodes.

d8 system collect-debug-info virtualization --help
Collect a separate debug archive with detailed data from the d8-virtualization namespace.

Usage:
  d8 system collect-debug-info virtualization [flags] > deckhouse-debug-virtualization-$(date +"%Y_%m_%d").tar.gz

Examples:
  # Collect the virtualization debug archive:
  d8 system collect-debug-info virtualization > deckhouse-debug-virtualization-$(date +"%Y_%m_%d").tar.gz

  # The --skip-ds-logs flag can be used to skip logs from DaemonSet-managed pods
  # (virt-handler, virtualization-dra, vm-route-forge, ...) to reduce archive size:
  d8 system collect-debug-info virtualization --skip-ds-logs > deckhouse-debug-virtualization-$(date +"%Y_%m_%d").tar.gz

Flags:
      --command-timeout duration    Timeout for each individual debug command execution (default 2m0s)
  -h, --help                        help for virtualization
      --request-interval duration   Minimum interval between debug command executions to avoid overloading the cluster (e.g. 200ms, 500ms, 1s). Zero disables rate limiting (default 0s)
      --skip-ds-logs                Skip collecting logs from pods managed by a DaemonSet (virt-handler, virtualization-dra, vm-route-forge, ...) to reduce archive size on clusters with many nodes

Global Flags:
      --context string      The name of the kubeconfig context to use
  -k, --kubeconfig string   Path to kubeconfig file. (default is $KUBECONFIG when it is set, otherwise the default kubeconfig path for the current OS user) (default "/Users/valery.losev/.kube/config")

[Copilot]

🟡 Changes recommended

There are user-facing behavioral issues (notably --exclude no longer matching module-expanded filenames as documented) and reliability issues from ignoring tar/gzip Close() errors that can produce silently corrupted archives.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR enhances d8 system collect-debug-info by reorganizing debug archive contents (renamed output files, additional collected resources) and extracting a reusable command-execution pipeline, while also introducing a dedicated virtualization subcommand to collect a separate, more detailed archive for the d8-virtualization namespace.

Changes:

• Refactored the tarball creation flow by extracting the exec→tar loop into a reusable runCommands helper.
• Renamed/added collected artifacts in the main debug archive (including CRD collection and additional virtualization module controller logs with tail limits).
• Added d8 system collect-debug-info virtualization to collect per-pod logs from d8-virtualization with an option to skip DaemonSet-owned pod logs.

File summaries

File	Description
internal/system/cmd/collect-debug-info/virtualizationtar/virtualizationTar.go	Adds the new virtualization cobra subcommand and CLI flags.
internal/system/cmd/collect-debug-info/debugtar/virtualizationTarball.go	Implements the virtualization-focused tarball (pod discovery + per-pod logs).
internal/system/cmd/collect-debug-info/debugtar/debugTar.go	Renames/extends the main debug command list and extracts runCommands.
internal/system/cmd/collect-debug-info/collect-debug-info.go	Wires the new virtualization subcommand into collect-debug-info.

Review details

Suppressed comments (1)

internal/system/cmd/collect-debug-info/debugtar/debugTar.go:177

• Same issue as the CCM logs filename: {module-name} prefix breaks prefix-based --exclude values like csi-controller-logs and makes --list-exclude output less useful. Keeping the placeholder at the end preserves existing exclusion behavior.

		File:            "{module-name}-csi-controller-logs.txt",

• Files reviewed: 4/4 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Glitchy-Sheep]

Two things to fix before merging, both in the base archive:

1. --exclude and --list-exclude break for per-module files after the rename. See the inline comment.
2. MCM machines are dropped from the archive instead of being collected alongside CAPI machines. See the inline comment.

One thing to decide: renaming almost every file in the archive is a breaking change. It affects existing --exclude values, support scripts and the docs on the site. The card and the thread did not ask for it. If we keep it, please state it in the PR description and update the --exclude example in the help once the exclude logic is fixed.

Optional: --all-containers=true in the log commands would also capture sidecars, for example the second container of dvcr. kubectl defaults to the first container, so this is not blocking.

[ldmonster]

Data race on this shared buffer whenever a command times out

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 459 (shared buffers) read and reset at 486-499, after StreamWithContext may have returned early at 478-484

stdout/stderr are hoisted out of the loop and reused, but on the timeout path client-go abandons the goroutines still writing into them. client-go/tools/remotecommand/spdy.go:

select {
case p := <-panicChan:
	panic(p)
case err := <-errorChan:
	return err
case <-ctx.Done():
	return ctx.Err()   // returns without joining the copiers
}

The wg.Wait() that joins copyStdout/copyStderr (v2.go, doing io.Copy(p.Stdout, p.remoteStdout) straight into this buffer) lives inside that abandoned goroutine. defer conn.Close() does not join either — spdystream's Close() ends with go s.shutdown(...), and Stream.Reset() only unblocks a blocked Read, not an io.Copy already inside dst.Write.

Meanwhile the main goroutine does stdout.Bytes() (line 494) and stdout.Reset() (line 498) with no synchronization. bytes.Buffer is not goroutine-safe:

• a late Write after Reset() re-seeds the buffer, so those bytes are emitted as the next command's file content;
• tryGrowByReslice extends b.buf's length before the copy fills it, so a concurrent Bytes() can hand writeToTar a slice covering not-yet-written bytes (and Size is computed from that racy length).

Pre-existing mechanism, but newly reachable: every existing log command caps output so the 2m default rarely fires, whereas the new --tail=-1 per-pod commands make the deadline path routine. go test -race around a timing-out StreamWithContext should show it.

Fix: give each command its own buffers (declare them inside the loop), or join the stream before touching them.

[ldmonster]

A failed pod list produces a one-file archive that reports success

Affected lines: internal/system/cmd/collect-debug-info/debugtar/virtualizationTarball.go 64-69 (warn-and-continue) leading to 86-94 (success banner + return nil)

When fetchVirtualizationPods fails this only warns and falls through, so pods is nil and buildVirtualizationCommands returns just the single static d8-virtualization-pods-wide.txt. The deferred close still writes a well-formed tar+gzip trailer, "Virtualization debug archive collection completed." prints, and return nil -> exit 0.

And that one entry is itself empty: kubectl -n d8-virtualization get pod -o wide has no --ignore-not-found, so on an absent/empty namespace it writes to stderr and nothing to stdout, exits 0, and writeToTar stores a 0-byte member. Net result: a valid ~100-byte .tar.gz containing one empty file, with $? == 0.

The copied-from-Tarball idiom does not transfer: there, a failed fetchActiveModules costs ~15 of ~63 commands; here the pre-flight fetch is the entire payload. Suggest returning the error (or at least a non-zero exit) when the pod list fails, and writing a collection-errors.txt marker into the archive so a degraded bundle is self-describing.

Separately: the named return err is left non-nil across this warn-and-continue. It is harmless today only because line 88 unconditionally reassigns it before any return — but the defer at 76-84 reads err, so inserting any early return between here and line 88 would surface this swallowed error and suppress the close errors. err = nil after the warning (or a distinct variable) would make that safe.

[ldmonster]

First subcommand added, but Args is still nil — a typo silently runs the full archive

Affected lines: internal/system/cmd/collect-debug-info/collect-debug-info.go 56-77 (command literal with no Args:) + 83 (AddCommand)

collectDebugInfoCmd has a RunE and no Args:. Traced through cobra v1.10.2:

1. Find -> innerfind descends to collect-debug-info; findNext("virtualisation") misses, so it returns this command with args ["virtualisation"].
2. legacyArgs (args.go): if !cmd.HasSubCommands() { return nil } no longer short-circuits, but the unknown-command branch is guarded by !cmd.HasParent() — and this command has a parent (system) — so it returns nil. The "unknown command" error is root-only.
3. ValidateArgs: if c.Args == nil { return ArbitraryArgs(...) } -> nil.
4. RunE: func(cmd *cobra.Command, _ []string) discards the positional arg.

So d8 system collect-debug-info virtualisation > vm.tar.gz runs the full ~63-command cluster-wide collection into vm.tar.gz, exit 0, no "Did you mean" suggestion — after a long wait, and not the archive the user asked for.

Suggested change

	collectDebugInfoCmd.AddCommand(virtualizationtar.NewCommand())
	collectDebugInfoCmd.Args = cobra.NoArgs
	collectDebugInfoCmd.AddCommand(virtualizationtar.NewCommand())

(Worth setting Args: cobra.NoArgs on the new virtualization command too.)

[ldmonster]

This early return makes --exclude strictly narrower for exactly the two commands that set ExcludeKey

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 656-658 - the early return skips 660-671 (base-name match + prefix-family loop)

Returning here skips the extension-stripped base check below and the HasPrefix(base, excluded+"-") group loop, which every other command still gets. Measured for the expanded File: "d8-cloud-provider-aws-ccm-logs.txt", ExcludeKey: "ccm-logs":

--exclude ccm-logs                            => true
--exclude d8-cloud-provider-aws-ccm-logs.txt  => true
--exclude d8-cloud-provider-aws-ccm-logs      => false   <-- works for all 61 others
--exclude d8                                  => false   <-- works for all 61 others
--exclude d8-cloud-provider-aws               => false

For comparison, File: "d8-cert-manager-logs.txt" (no ExcludeKey) answers true to all four forms. These two are the only d8--prefixed files of 63 that survive --exclude d8 — and they are two of the largest per-provider log dumps, i.e. exactly what someone shrinking an archive is reaching for.

Simpler and symmetric — collect the candidate keys and test membership, no early return:

keys := []string{cmd.File, trimExt(cmd.File)}
if cmd.ExcludeKey != "" {
	keys = append(keys, cmd.ExcludeKey)
}

Side note surfaced while measuring this: the group-prefix loop is pre-existing, but the category rename widened its blast radius a lot (--exclude cluster 2->9 files, instance-manager 0->11, kube-system 6->10, d8 7->12), and none of those 9 category tokens is printed by --list-exclude. Every one of the 63 files is now reachable via a token the tool never tells you exists.

[ldmonster]

One flaky kubectl get module silently drops every module-gated command, exit 0

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 554-556 (the continue), fed by 402-405 in Tarball (warn-only on fetch failure); 15 commands carry RequiredModule

fetchActiveModules returns nil on every error path, Tarball only warns, and this continue then drops all 15 commands carrying a RequiredModule: cloud-provider machine-deployment + the per-provider CCM/CSI logs, both cert-manager artifacts, the whole istio set (resources, CRs, envoy config dump, istiod/ingressgateway/user-proxy logs), cilium health status, and the three new d8-virtualization-* log files.

The run still prints Debug archive collection completed. and exits 0, so the operator ships a superficially complete archive missing 15 files, with one stderr WARNING that is easy to miss when stdout is redirected. fetchActiveModules shares commandTimeout, so the 2m default is itself a plausible trigger on a loaded cluster.

Pre-existing, but this PR widens it (13 gated commands on main -> 15 here). Given the all-or-nothing failure mode, consider either failing hard when the module list cannot be fetched, or running the gated commands anyway and letting them produce empty files — and writing a marker into the archive either way.

[ldmonster]

Ungated cloud-only query, and the bash -c '... | jq ...' shape hides the failure completely

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 173-187 (de-gated MCM pods command + new ungated sapcloud query) - contrast 137-142, which keeps RequiredModule for the same API group

This command and instance-manager-machine-controller-manager.json (line 174, which lost RequiredModule: "cloud-provider" in this diff — it is present on main) now run on clusters with no MCM at all. instance-manager-capi-machines.json (line 123) is in the same position for machines.cluster.x-k8s.io.

The inconsistency is visible two entries up: instance-manager-cloud-machine-deployment.txt (line 138) still gates the same sapcloud API group on cloud-provider, and instance-manager-static-machine-deployment.txt (line 146) uses --ignore-not-found.

Two things worth knowing about the failure mode:

1. --ignore-not-found would not help. A missing resource type fails at builder time (the server doesn't have a resource type %q), and IgnoreErrors explicitly "will filter errors that occur when by visiting the result (but not errors that occur by creating the result in the first place)".
2. Because there is no pipefail, bash -c 'kubectl ... | jq ...' exits 0 when kubectl fails (bash -c 'false | jq .'; echo $? -> 0). So streamErr == nil, the ERROR: collecting ... branch never fires, kubectl's stderr is discarded by stderr.Reset(), and a zero-byte file lands in the archive, indistinguishable from "the resource exists and is empty".

Not data loss, but it turns a diagnosable error into a silent empty file. Either restore the gate for symmetry with line 138, or add set -o pipefail to these bash -c commands so failures are actually reported.

[ldmonster]

Inferring expansion from the placeholder allows duplicate tar entry names

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 599-610 (needsModuleExpansion) + 558-567 (expansion writes File per module); test gap at internal/system/cmd/collect-debug-info/debugtar/debugTar_test.go 13-24

This returns true when {module-name} appears in Args only, but the expansion above templates it into File as well — so a command with a fixed File and a placeholder in Args gets duplicated once per matched module with the same File.

Reproduced: {File: "cloud-provider-secrets.txt", Args: ["-n", "d8-{module-name}", "get", "secrets"], RequiredModule: "cloud-provider"} with cloud-provider-aws + cloud-provider-yandex Ready yields two commands both named cloud-provider-secrets.txt. Real GNU tar: tar -tvf lists both members, tar -xf keeps only the last — the first module's data is silently gone, with no hint in the archive.

Latent today (no current command has that shape), but the new debugTar_test.go will not catch it: its body starts if cmd.RequiredModule != "" { continue }, so it only inspects commands without RequiredModule — and a command that expands must have one.

A mechanism-level guard would make the whole class harmless instead of merely detectable, e.g. in writeToTar:

if strings.Contains(c.File, "{module-name}") {
	return fmt.Errorf("unresolved {module-name} placeholder in archive entry %q", c.File)
}

That also covers virtualizationCommands, which the test does not iterate and which never goes through filterAndExpandCommands at all.

[ldmonster]

cluster-crd.json is an unfiltered full-CRD dump added to every default run

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 366-370

kubectl get customresourcedefinitions -o json with no filtering returns every CRD's complete spec.versions[].schema.openAPIV3Schema. On a DKP cluster with virtualization + istio + cilium + storage that is realistically tens of MB — buffered whole in the CLI's shared bytes.Buffer, and decoded whole into unstructured objects by kubectl inside the deckhouse leader container (the cgroup covers both).

If it exceeds the 2m default --command-timeout, runCommands prints a WARNING and then writes stdout.Bytes() anyway, so a truncated, unparseable cluster-crd.json lands in the archive looking like a normal entry. (Same hazard already applies to cluster-events.json and d8-all.json, so this adds to an existing pattern rather than introducing it — hence flagging it as a risk rather than a certain break.)

--ignore-not-found=true is close to dead weight here: kubectl's own help says it "has no effect when no resources are found" for collections, and it would not rescue a missing type either.

A jq 'del(.items[].spec.versions[].schema)' filter, or -o custom-columns=NAME:.metadata.name,VERSIONS:.spec.versions[*].name, would cut this to tens of KB and keep the diagnostic value. (Two things I checked that are not problems: managedFields are already stripped by OmitManagedFieldsPrinter, and kubectl chunks at 500 by default.)

[ldmonster]

internal/system/README.md was not updated — and one stale line is security-relevant

Affected lines: internal/system/README.md 230-239 (stale --exclude rule + the redaction warning) and internal/system/README.md 54 (command map, no virtualization leaf, no --skip-ds-logs)

The diff touches no .md file. internal/system/README.md is the only documentation for collect-debug-info, and it documents archive contents by exact filename:

• line 239: "Only global-values.json is redacted (its kubeRBACProxyCA and registry dockercfg); container logs and the raw audit-policy Secret are included unredacted." Those members are now cluster-global-values.json and kube-system-audit-policy.json. Someone following this line to decide an archive is safe to share will grep for global-values.json, find nothing, and conclude there is no unredacted-secret risk.
• line 234: "ccm-logs also drops the per-cloud ccm-logs-<module>.txt" — the file is now d8-<module>-ccm-logs.txt, and it is dropped via the new ExcludeKey path, not the documented base-name rule.
• line 230: "~60 diagnostic commands" (now 63), and the module-gated list omits virtualization.
• line 54 / the flag table: no entry at all for this new virtualization subcommand or --skip-ds-logs, though the README gives every other leaf command a command-map line and a flag table.

So the only discoverability for this command is --help.

[ldmonster]

This is fetchActiveModules copied — and the copies have already drifted

Affected lines: internal/system/cmd/collect-debug-info/debugtar/virtualizationTarball.go 100-154 vs internal/system/cmd/collect-debug-info/debugtar/debugTar.go 506-544 (fetchActiveModules); also internal/system/cmd/collect-debug-info/debugtar/virtualizationTarball.go 53-95 vs internal/system/cmd/collect-debug-info/debugtar/debugTar.go 391-436 (Tarball), and internal/system/cmd/collect-debug-info/virtualizationtar/virtualizationTar.go 80-101 vs internal/system/cmd/collect-debug-info/collect-debug-info.go 96-122 (collectDebugInfo)

22 non-blank lines are identical to fetchActiveModules (debugTar.go:506-544): same param shape, same ExecInPod + "create executor: %w", same two buffers, same context.WithTimeout/defer cancel(), same StreamWithContext, same %w (stderr: %s) shape, same json.Unmarshal(stdout.Bytes(), ...).

The drift is already visible: this one guards if stdout.Len() == 0 { return nil, nil } before unmarshalling; fetchActiveModules does not, so an empty exec response gives nil, nil here and parse module list: unexpected end of JSON input there. That divergence exists because the block was copied rather than shared. The same executor + two-buffers + StreamWithContext dance appears at ~8 sites repo-wide — the natural home for a helper is next to ExecInPod in internal/utilk8s/operatepod.go.

Two more copies in the same change:

• VirtualizationTarball vs Tarball: 24 identical non-blank lines — the const namespace/containerName block, the GetDeckhousePod 4-liner with the same error wrap, the gzip/tar construction, and the entire 9-line deferred double-close. runCommands was extracted in this very change for exactly this reason; it stopped one frame short of a shared writeArchive(commands, excludeMap, banners, opts).
• collectVirtualizationDebugInfo vs collectDebugInfo: a 14-line verbatim copy (two flag reads + SetupK8sClientSet + three identical error wraps). utilk8s.NewDynamicClient(cmd) already does exactly this shape for the dynamic client; the missing sibling is NewClientSet(cmd).

Also worth noting: this shells out to kubectl get pods -o json through pod-exec even though a typed kubeCl kubernetes.Interface is already in scope and used one line earlier by GetDeckhousePod. kubeCl.CoreV1().Pods(ns).List(...) is the established pattern in this repo, needs no pods/exec RBAC and no kubectl in the container, and would let the hand-rolled podList struct go away in favour of corev1.PodList (plus metav1.GetControllerOf for the owner check).

[ldmonster]

Field-by-field rebuild silently drops fields — it already drops RequiredModule

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 561-566

Every expanded copy comes out with RequiredModule: "". Harmless today (nothing downstream reads it — runCommands uses Cmd/Args/File, isFileExcluded uses File/ExcludeKey, writeToTar uses File), but this diff is itself the evidence that it is a trap: adding ExcludeKey to the struct forced a hand-edit here to stop it being dropped too.

Copy the struct and overwrite only what is substituted — three lines, drop-proof, and verified behaviourally identical on every field the downstream code reads:

Suggested change

	result = append(result, Command{
	expanded := cmd
	expanded.File = strings.ReplaceAll(cmd.File, "{module-name}", moduleName)
	expanded.Args = replaceModuleName(cmd.Args, moduleName)
	result = append(result, expanded)

[ldmonster]

Dead line, and ExcludeKey is derivable state that nothing enforces

Affected lines: internal/system/cmd/collect-debug-info/debugtar/debugTar.go 639-649 (excludeBaseName, dead line at 646) - the ExcludeKey field spans 29-31, set at 190 and 197, copied at 565, consumed at 656-658

After the rename the placeholder sits in the middle of names (d8-{module-name}-ccm-logs.txt), so no File ends in -{module-name}: 0 of 63 reach this TrimSuffix. It is doubly unreachable for current data, since both placeholder commands set ExcludeKey and return at line 640.

And ExcludeKey's two values are exactly reproducible from File — I checked a one-line rule (strip the d8-{module-name}- / -{module-name} segment, then trim) across all 63 entries and it reproduces today's GetExcludableFiles() byte-identically. Teaching excludeBaseName that rule would cover head/middle/tail positions for every present and future command with zero per-command data, and let you delete the field, its doc comment, the two literals, the branch at 640, the early return at 656, and the hand-copied ExcludeKey: cmd.ExcludeKey at 565 — six sites replaced by one function.

As it stands nothing checks that ExcludeKey agrees with File: setting it to "totally-unrelated" is silently accepted (isFileExcluded(ccm-logs) == false, isFileExcluded(totally-unrelated) == true), and omitting it on a new placeholder command makes --list-exclude advertise a literal d8-{module-name}-foo-logs token that can never match anything. The new test does not cover either case.

The doc comment at lines 29-31 also points at this dead line to justify the field, which leaves both naming conventions simultaneously supported and neither documented as preferred.