Reduce timeouts and use kola managed reboot instead of ignition-triggered

[mayankfz]

Summary

Reduce timeouts and use kola managed reboot instead of ignition-trigger for acl.kdump.
But acl.kump is still flaky on qemu + aarch64, so coverage will be done from Azure.

Change Log

• Use kola managed reboot instead of ignition-trigger for acl.kdump

Type of Change

• New kola test
• Kola test fix/update
• Platform integration change (Azure, QEMU, etc.)
• CLI/command change
• CI/automation change
• Bug fix
• Documentation update

Associated Issues

https://dev.azure.com/mariner-org/ACL/_workitems/edit/22249
https://dev.azure.com/mariner-org/ACL/_workitems/edit/22250

Test Methodology

• Test details: pipeline runs ->
  https://dev.azure.com/mariner-org/ACL/_build/results?buildId=1197962&view=results

Merge Checklist

All applicable boxes should be checked before merging

• go build ./... passes (basic validation)
• docker build -t mantle . succeeds (full multi-arch build; required for build-affecting changes)
• go test ./... passes
• go vet ./... reports no issues
• Relevant kola tests pass against a test image
• Documentation has been updated to match any changes
• Ready to merge

[Copilot]

Pull request overview

This PR updates the acl.kdump kola test to avoid ignition/systemd-triggered reboots and to bound post-crash reconnection time, reducing overall flakiness and avoiding long stalls caused by platform.CheckMachine retry behavior.

Changes:

• Move UKI kdump addon activation to a kola-managed SSH step followed by m.Reboot() (instead of an ignition/systemd one-shot reboot).
• Replace platform.CheckMachine after the induced crash with a bounded SSH retry loop to cap worst-case wait time (~25 minutes).

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

kola/tests/misc/kdump.go:30

• The UKI kdump test is still registered to run on qemu/qemu-unpriv for all architectures. Later in this file the post-crash wait logic explicitly assumes arm64 qemu is excluded and uses a tighter reconnection deadline; if this test is run directly from mantle without an external exclusion layer, arm64 qemu runs are likely to be flaky/timeout. Consider encoding the exclusion in the test registration (e.g., SkipFunc for arm64 on qemu/qemu-unpriv) so the behavior is self-contained.

		Flags:       []register.Flag{register.NoKernelPanicCheck, register.NoEmergencyShellCheck},
		MinVersion:  semver.Version{Major: 3},
		Distros:     []string{"acl"},
		Platforms:   []string{"qemu", "qemu-unpriv", "azure"},
	})

kola/tests/misc/kdump.go:44

• This GRUB-mode kdump test is also registered on qemu/qemu-unpriv without an architecture guard. If arm64 qemu is expected to be excluded (as noted in the post-crash wait comment below), consider adding the same SkipFunc here to prevent flaky/slow arm64 qemu runs when executing mantle kola directly.

		Flags:       []register.Flag{register.NoKernelPanicCheck, register.NoEmergencyShellCheck},
		MinVersion:  semver.Version{Major: 3},
		Distros:     []string{"acl"},
		Platforms:   []string{"qemu", "qemu-unpriv"},
	})

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

kola/tests/misc/kdump.go:183

• The reconnect loop treats the first successful SSH connection as “VM back”, but SSH can be reachable while the system is still starting. That can make the subsequent vmcore checks flaky because they run immediately after reconnect. Consider probing for basic system readiness (e.g., systemctl is-system-running) instead of true so the loop exits only once boot has actually completed.

		if _, lastErr = c.SSH(m, "true"); lastErr == nil {

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Jiri Appl (jiria)]

Why register for qemu in the first place if we are going to skip it anyway?

[mayankfz]

This is what you asked for -> "Wonder if we should leave it enabled for qemu but not schedule the test for qemu". Would request if you could help me understand a little bit more on the expectation. Thanks

[Jiri Appl (jiria)]

Hmm, that sounds strange, missing context now. Sorry, mental cache flushed. Is the test too slow even on amd64 qemu? If so, I would say take out from qemu and leave a comment in place explaining why. If amd64 is doable, lets just disable for qemu/aarch64 for the similar comment for now (e.g. through the runtime skip if there is no better way).

[mayankfz]

No worries, have enabled the test for qemu + amd64.
pipeline runs ->

https://dev.azure.com/mariner-org/ACL/_build/results?buildId=1179467&view=results
https://dev.azure.com/mariner-org/ACL/_build/results?buildId=1179477&view=results

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

kola/tests/misc/kdump.go:177

• The 45s "hard cap" on the SSH probe only limits how long this loop waits on the result; it does not cancel the underlying c.SSH call. If the SSH dial/handshake/session.Run hangs, the goroutine can block forever and each retry spawns another goroutine, potentially leaking goroutines and connections on a half-alive VM. Consider adding a real SSH timeout/cancellation mechanism (e.g., set ssh.ClientConfig.Timeout and/or deadlines on the underlying net.Conn) so probes can be bounded without leaking goroutines.

		// 45s hard cap per probe (dial + handshake + exec).
		done := make(chan error, 1)
		go func() { _, err := c.SSH(m, "true"); done <- err }()
		select {
		case err := <-done:

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

kola/tests/misc/kdump.go:179

• The new SSH-reconnect loop claims a “45s hard cap per probe”, but the timeout is only applied to waiting on the goroutine — the underlying c.SSH() call can still block indefinitely (e.g., during ssh.NewClientConn handshake), leaving a stuck goroutine behind. If the machine stays half-alive long enough, this loop can leak multiple goroutines (one per timed-out probe), which is both a correctness and resource/cleanup concern for the test runner.

Consider enforcing the timeout at the SSH implementation layer (e.g., set a deadline on the TCP conn around ssh.NewClientConn in network/ssh.go, or add a context/timeout-aware SSH API) so the probe actually terminates, and then you can keep the bounded retry loop without leaks.

		go func() { _, err := c.SSH(m, "true"); done <- err }()
		select {
		case err := <-done:
			lastErr = err
		case <-time.After(45 * time.Second):

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

kola/tests/misc/kdump.go:102

• The aarch64/QEMU skip is only implemented in kdumpUKITest, but acl.kdump.grub still runs on qemu/qemu-unpriv without any arm64-usr guard. Given the stated TCG slowness, this test will still attempt the full crash-dump cycle on aarch64 QEMU and likely time out/flap. Add the same runtime skip to kdumpGRUBTest (or move the skip into the shared kdumpVerifyAndCrash).

	// GRUB test only - skip on UKI-booted images (not applicable, not a defect).
	if _, err := c.SSH(m, "sudo test -d /boot/EFI/Linux"); err == nil {
		c.Skip("GRUB kdump test not applicable on a UKI-booted image")
	}

kola/tests/misc/kdump.go:156

• This comment references kola_enforcing.yaml, but there is no such file in this repo. That makes the rationale for skipping aarch64/QEMU misleading; document the actual mechanism (runtime skip in the test code).

	// typically takes 2-3 min on amd64, aarch64 qemu is excluded via
	// kola_enforcing.yaml because TCG emulation is much slower.

kola/tests/misc/kdump.go:186

• The per-probe 45s timeout doesn’t actually stop the underlying c.SSH call. If the SSH handshake/session blocks indefinitely, the goroutine will leak (and each retry can leave another stuck goroutine/connection). Limit to at most one in-flight probe goroutine so timeouts don’t accumulate resources.

		// 45s hard cap per probe (dial + handshake + exec).
		done := make(chan error, 1)
		go func() { _, err := c.SSH(m, "true"); done <- err }()

[Copilot]

🟡 Changes recommended

There are correctness and robustness issues in the updated kdump test logic (platform skip consistency, misleading comments, and potential goroutine/connection leaks in the SSH probe loop) that should be addressed before merging.

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

Review details

Suppressed comments (1)

kola/tests/misc/kdump.go:163

• This comment references excluding aarch64 QEMU via "kola_enforcing.yaml", but that file is not present in this repository (only referenced in CONTRIBUTING via an external link). Since the code already has (or should have) a runtime skip for qemu+arm64-usr, the comment should describe the in-code behavior to avoid misleading future readers.

	// Wait for the machine to come back. The full crash-dump-reboot cycle
	// typically takes 2-3 min on amd64, aarch64 qemu is excluded via
	// kola_enforcing.yaml because TCG emulation is much slower.

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

[Copilot]

🟡 Changes recommended

The new SSH probe timeout loop can still leave indefinitely blocked SSH goroutines (handshake has no deadline) and can accumulate stuck probes, undermining the intended reliability improvement.

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

Review details

Suppressed comments (1)

kola/tests/misc/kdump.go:188

• The 45s "hard cap per probe" isn’t actually enforced on the underlying SSH attempt: when the time.After branch fires, the goroutine running c.SSH can remain blocked indefinitely (SSH handshake in network/ssh.go has no deadline). Retrying in a loop can accumulate stuck goroutines and still leave the test hanging later when it needs real SSH again. Consider fixing the SSH handshake to have a deadline (e.g., set read/write deadlines on the TCP conn during ssh.NewClientConn, or add a context-aware SSH helper) and then rely on a single bounded probe loop here.

		// 45s hard cap per probe (dial + handshake + exec).
		done := make(chan error, 1)
		go func() { _, err := c.SSH(m, "true"); done <- err }()
		select {
		case err := <-done:
			lastErr = err
		case <-time.After(45 * time.Second):
			lastErr = fmt.Errorf("ssh probe timed out after 45s")
		}

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

[Copilot]

🔵 Needs a closer look

The new set -euo pipefail grub.cfg parsing uses a sed | head pipeline that can fail under pipefail, causing intermittent test failures before the reboot/verification logic runs.

Review details

Suppressed comments (2)

kola/tests/misc/kdump.go:122

• With set -euo pipefail, the sed ... | head -n 1 pipeline can abort the script if /oem/grub.cfg ever contains multiple set linux_append= lines: head exits after the first line and sed may receive SIGPIPE, making the pipeline fail under pipefail. Prefer a single sed that quits after the first match to avoid relying on head.

		if [ -f /oem/grub.cfg ]; then
			existing=$(sed -n "s/^set linux_append=\"\([^\"]*\)\".*/\1/p" /oem/grub.cfg | head -n 1)
		fi

kola/tests/misc/kdump.go:168

• The comment says aarch64 qemu is excluded via kola_enforcing.yaml, but this repository doesn’t contain that file and the code already enforces the skip at runtime via skipKdumpOnSlowQEMU. This is misleading for anyone running the test directly from Mantle (and risks bitrot if the external config changes).

	// Wait for the machine to come back. The full crash-dump-reboot cycle
	// typically takes 2-3 min on amd64, aarch64 qemu is excluded via
	// kola_enforcing.yaml because TCG emulation is much slower.

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite