[PRODENG-3594] Migrate host connectivity layer to rig v2 - #651
Open
james-nesbitt wants to merge 10 commits into
Open
james-nesbitt wants to merge 10 commits into
james-nesbitt wants to merge 10 commits into
Conversation
james-nesbitt
added
smoke-test
Collaborator
Author
Manual testing in progressRunning a new smoke test against real infra (AWS profile
Rationale: the existing Will post the result (pass/fail + any findings) once it completes. Written by AI: claude-sonnet-5 |
Collaborator
Author
Manual test result: FAIL (infra/coverage gap, not a rig v2 regression)Ran Result: failed at base install — Investigated before reporting:
Take: likely an MCR channel/OS coverage gap ( Written by AI: claude-sonnet-5 |
Collaborator
Author
Root cause found: AMI drift + zypper vendor-change policy, NOT a rig v2 regression, NOT an MCR channel gapManually reproduced on a standalone SLES15 instance (bypassing Launchpad) to get past the error-swallowing bug fixed earlier in this thread. Root cause:
Confirmed channel-independent: reproduced the identical failure with Confirmed NOT a rig v2 regression: Confirmed undocumented in MCR's own docs: both Impact: All diagnostic AWS resources (2 standalone EC2 instances, security groups, keypairs) cleaned up and verified terminated/deleted — no orphaned spend. Written by AI: claude-sonnet-5 |
3 tasks
james-nesbitt
force-pushed
the
PRODENG-3594
branch
from
August 4, 2026 15:09
0d6d114 to
f866014
Compare
james-nesbitt
added
smoke-test
james-nesbitt
force-pushed
the
PRODENG-3594
branch
from
August 6, 2026 06:00
81a9b10 to
9b9ee68
Compare
james-nesbitt
force-pushed
the
PRODENG-3594
branch
2 times, most recently
from
September 15, 2026 18:56
e65bef4 to
537ebee
Compare
Rebased onto main. Resolved the SLES InstallMCR conflict to retain the --allow-vendor-change fix (PRODENG-3623 / #652) expressed in rig v2's API. Build/test fixes required by the migration: go.mod/go.sum tidied for github.com/k0sproject/rig/v2, validate_facts_test.go updated to rig v2 CompositeConfig/ssh.Config, and %w error wrapping in the EL/SLES/Ubuntu configurers. Adds TestUpgradeModernClusterFromLegacy. Signed-off-by: Kimmo Lehto <kimmo.lehto@gmail.com> Written by AI: claude-sonnet-5
Two bugs made every Linux smoke job hang until the harness killed it. MKE installed fine, then 'Validating MKE Health' looped 'waiting for MKE at https://<ip>/_ping to become healthy' every 30s until the test panicked. 1. GET -> HEAD regression (introduced by the rig v2 migration) CheckHTTPStatus was switched to remotefs.HTTPStatusInsecure, which issues a HEAD request on both PosixFS (curl -kIso) and WinFS (Method='HEAD'). The previous per-OS implementations issued a GET, and the MKE health endpoints do not answer HEAD with 200, so the check could never succeed. rig v2's Windows path additionally does not skip TLS verification on PowerShell 5.x, which breaks against MKE's self-signed certs. Reinstate HTTPStatus on the Linux and Windows configurers (GET, TLS skipped), restoring the pre-migration semantics, and have CheckHTTPStatus delegate to the configurer again. Keeps the per-OS logic in the per-OS layer. 2. pingHost deadlock (pre-existing on main, latent until the check fails) On the error path pingHost sent twice on errCh (the error, then nil) while errCh is buffered to len(hosts) and only drained after wg.Wait(). Any failure overflowed the buffer, blocked the second send, and left waitgroup.Done() unreached, so wg.Wait() blocked forever -- converting a bounded ~5 minute failure into an indefinite hang. Now sends exactly one value and defers Done. Renamed the pingHost host parameter for varnamelen after the added comment extended the function scope. Written by AI: claude-sonnet-5
Both Windows smoke jobs failed at Open Remote Connection with 'connect <ip>:5985: All attempts fail'. Windows hosts listen for WinRM over TLS on 5986; 5985 is the plaintext port. creasty/defaults applies rig's winRM port struct-tag default (5985) during Host.UnmarshalYAML, before rig ever sees the config, so the port is never zero by the time rig defaults it. rig v0 corrected this by bumping 5985 -> 5986 when useHTTPS was set; rig v2 only derives the port when it is zero (and only infers useHTTPS when the port is already 5986). A host with 'useHTTPS: true' and no explicit port -- exactly what the terraform modules generate -- was therefore left attempting TLS against the plaintext port. Restore the bump in Host.UnmarshalYAML so existing configs keep working without having to name the port explicitly. Adds TestHostWinRMHTTPSPortDefault, which covers the compatibility matrix (useHTTPS with no port, with 5985, with 5986, a custom port, and no useHTTPS). Verified the test fails without the fix, reproducing the CI symptom exactly (expected 5986, got 5985). Written by AI: claude-sonnet-5
Mixed Linux/Windows clusters failed OS detection on every Linux host: Detect host operating systems => failed to resolve configurer for <host>: unsupported OS: linux rig's os.DefaultRegistry holds [ResolveLinux, ResolveLinuxCompat, ResolveWindows, ResolveDarwin] and is a process-global. On a successful match it promotes the winning resolver to the front of the slice so later hosts hit it first. That optimisation is only sound while no resolver matches a superset of another. ResolveLinuxCompat breaks it: it is a fallback for hosts with no /etc/os-release and reports ID "linux" for any Linux host at all. Resolving a Windows host swaps ResolveWindows from index 2 to index 0, which leaves the order [Windows, LinuxCompat, Linux, Darwin] -- the fallback now sits ahead of the real resolver. Every Linux host detected after a Windows host is reported as "linux", matches no configurer and fails the phase. This is why smoke-fips and smoke-windows failed while smoke-modern, smoke-legacy and smoke-upgrade passed: only the mixed clusters ever resolve a Windows host, and the CI logs show the Windows host resolving immediately before the Linux host failed. Give the client its own registry, identical to rig's minus the compat fallback, via rig.WithOSReleaseProvider. Reordering is then harmless because every remaining resolver matches exactly one OS family. Dropping the fallback also restores rig v0's semantics: v0 had no compat resolver, so an unreadable os-release produced a real error instead of a silent misclassification. Every OS launchpad supports ships an os-release file, so the fallback could never yield a usable configurer. The AMI was not at fault -- verified against a live Ubuntu 22.04 FIPS instance, where rig resolves ID=ubuntu Version=22.04 correctly, 20 runs out of 20. Both tests fail without the fix, the first reproducing the exact CI symptom (expected "ubuntu", got "linux"). Written by AI: claude-sonnet-5
smoke-fips abandoned a Windows host on the first connect attempt: Open Remote Connection => connect 44.211.52.127:5986: All attempts fail: #1: retry: abort condition reached after 1 attempts: operation cannot be completed: create shell: http response error: 401 - invalid content type The Connect phase exists to wait for hosts to become reachable, and on Windows an auth rejection is part of that wait: the WinRM HTTPS listener answers before provisioning has finished configuring authentication, so a freshly booted host returns 401 for a while with entirely correct credentials. rig v2 wraps 401/403 as protocol.ErrNonRetryable, and this phase's RetryIf honours that, so the wait ended on attempt 1 of 60. rig v0 did not classify these errors, so launchpad retried them and the condition healed itself. The migration swapped the predicate from ErrCantConnect to ErrNonRetryable and inherited the new classification with it. The contrast is visible within a single CI run: smoke-windows retried an i/o timeout to attempt 36 of 60, while smoke-fips aborted a 401 at attempt 1. Both are the same underlying condition - a Windows host whose provisioning has not finished - and the same host connected fine on the previous run, so this is timing, not credentials. Treat 401/403 as retryable again while still honouring ErrNonRetryable for everything else: bad certificates, host key mismatches and misconfigured bastions, none of which waiting can fix. The cost is that genuinely wrong credentials take the retry budget to report rather than failing at once. That is the right trade here: a cluster that would have come up must not fail, and the budget is bounded. Matching is by substring because the WinRM library returns untyped formatted errors and rig exports no sentinel; rig's own isAuthError does the same, and both message shapes it covers are matched. The test drives shouldRetryConnect, the predicate handed to RetryIf. Its three auth cases fail without this change while the four others pass. Written by AI: claude-sonnet-5
james-nesbitt
force-pushed
the
PRODENG-3594
branch
from
September 16, 2026 10:15
537ebee to
d2f4da2
Compare
isExitCode3010 matched the exact substring "non-zero exit code: 3010", which is rig v1's wording. rig v2's WinRM transport formats the same error as "command exited with a non-zero exit code: exit code 3010" (note the doubled "exit code"), so the substring never matched and a successful-but-reboot-required MCR install (ERROR_SUCCESS_REBOOT_REQUIRED) was treated as a hard failure instead of triggering the reboot phase. Reproduced on smoke-windows and smoke-fips CI runs for PRODENG-3594: both failed identically with "failed to install container runtime: ... command exited with a non-zero exit code: exit code 3010" during "Install Mirantis Container Runtime on the hosts". Match on the exit code number via regexp instead of a fixed phrase, so future wording changes in either rig transport don't silently break reboot handling again. Added pkg/configurer/windows_test.go covering both known phrasings, an unrelated exit code, and a numeric substring collision (13010) that a naive `strings.Contains(err, "3010")` fix would have wrongly matched. Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
smoke-windows CI hung for the full 60-minute test timeout: a Windows host rebooted mid-uninstall, reconnected successfully, and the very next remotefs.Upload call then hung indefinitely. A goroutine dump from the test's own timeout panic traced the root cause to rig v2: command.Wait() (protocol/winrm/connection.go) and the remotefs rigrcp helper's command loop (remotefs/winfile.go) had no timeout at all, so a WinRM session that silently died post-reboot could block the caller forever. Filed and fixed upstream as k0sproject/rig#472 / k0sproject/rig#473. The upstream fix only helps if a caller actually supplies a bounded context: rig's plain Exec/ExecOutput hardcode context.Background(), which never has a deadline. Add that bound here as defense in depth, independent of when the upstream fix lands: - pkg/configurer/host.go: extend the Host interface with cmd.ContextRunner so configurers can call ExecContext/ ExecOutputContext. - pkg/product/mke/config/host.go: add matching ExecContext/ ExecOutputContext wrapper methods on Host, mirroring Exec/ ExecOutput's existing sudo/SudoDocker routing (the promoted methods from the embedded *rig.Client would silently skip that routing). - pkg/configurer/windows.go: bound every Exec/ExecOutput call in InstallMCR, UninstallMCR, and RestartMCR -- the MCR lifecycle operations that run immediately around host reboots -- with a new windowsExecTimeout (15 minutes, generous for legitimate slow installs/image pulls, not a tight SLA). remotefs.Upload calls in these functions are unaffected by this change; they are already self-bounded by the upstream winfile.go fix regardless of caller context. Points the go.mod replace directive at the fork commit carrying the upstream fix (k0sproject/rig#473) until it merges and is tagged upstream, at which point the replace should be dropped and the version bumped normally. Added TestExecCtxIsBoundedAndCancelable pinning that the new helper actually carries a deadline and that cancel works. Verified: go build, go vet, full go test --tags 'testing' ./pkg/..., golangci-lint run pkg/configurer/... pkg/product/mke/config/... (one pre-existing, unrelated gofumpt finding in hosts.go, unchanged by this commit). End-to-end verification is the smoke-windows re-run this was found on. Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
Unblocks this branch's CI. launchpad.tf embeds the generated
windows_password directly into the launchpad_yaml output's
password/adminPassword fields, which launchpad's config loader then
runs through envsubst (pkg/config/config.go). PRODENG-3751 made an
unescaped "$word" in that YAML fail config loading loudly if the
implied variable is unset, correctly replacing silent password
truncation -- but the test's password generator was never updated to
account for it, so smoke-windows/smoke-fips fail whenever the random
password happens to contain "$" (e.g. run with password
"Nq4@CLkhD6%HvwO&$K2f": "variable ${K2f} not set").
This is a workaround, not the fix: the real fix is to escape "$" as
"$$" where launchpad.tf embeds the password into that YAML, so a
literal "$" -- legitimate in a real password -- keeps working end to
end, then restore "$" to this generator's symbol set. Tracked under
PRODENG-3751.
Written by AI: claude-sonnet-5
Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
smoke-windows failed at the (much later) Label nodes phase:
failed to label node 100.53.69.105:5986 (): command result:
process finished with error: Process exited with status 1
("docker node update" requires exactly 1 argument. ...)
The empty "()" is swarm.NodeID(h) having returned an empty string
with no error. docker swarm join succeeding only means the join
command itself completed; it does not guarantee this host's own
docker engine has finished updating its local view of swarm state,
particularly right after the reconnect JoinWorkers already does for
Windows hosts (swarm join tears down and re-establishes the WinRM
connection). LabelNodes runs several phases later and had no reason
to expect this, so it used the empty NodeID as-is.
Add a bounded retry (20 attempts, 3s delay) after joining -- and
after the Windows reconnect -- confirming swarm.NodeID(h) actually
returns a non-empty NodeID before JoinWorkers considers the host
joined. Applies to all hosts, not just Windows, since the race is
generic (docker's local swarm-state sync lag), even though it was
only actually observed on Windows in this session's testing, likely
because the Windows reconnect makes the race window more visible.
No dedicated unit test: swarm.NodeID takes the concrete *Host type
(not an interface), consistent with the rest of this package, so
testing this meaningfully would need a live connection; verified via
build, vet, golangci-lint (clean), the full unit suite, and the
smoke-windows run this was found on.
Written by AI: claude-sonnet-5
Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
james-nesbitt
marked this pull request as ready for review
September 17, 2026 13:06
james-nesbitt
changed the title
james-nesbitt
added
non-breaking change
…duplicate label runs Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
james-nesbitt
requested review from
kke,
nekwar and
pgedara
and
a lite review from Copilot
and removed request for
Copilot
September 17, 2026 14:29
Collaborator
Author
|
Full CI suite green on a single clean run (35228036331/35228030712/35228036383), no duplicate/concurrent runs this time: unit-test (ubuntu/macos/windows), CodeQL, Analyze (actions/go), smoke-legacy (24m38s), smoke-modern (20m15s), smoke-fips (19m15s), smoke-windows (28m50s), smoke-upgrade (51m46s). No orphaned AWS resources left behind after the run. Written by AI: claude-sonnet-5 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Migrate host connectivity from rig v0.x to rig v2, plus the bug fixes needed to get a mixed Linux/Windows cluster installing and uninstalling cleanly on rig v2.
Why
Replaces #645. #645 is opened from a fork (
kke/launchpad:rig-v2) and has drifted frommain(mergeStateStatus: DIRTY,mergeable: CONFLICTING). This PR started as a rebased snapshot of #645 for reference, but since all further fixes were made and validated here, andmainwrite access is not available onkke/launchpad, this is now the primary PR for the migration.How
rig-v2onto currentmain, resolving conflicts from several PRODENG fixes that landed onmainafter [PRODENG-3594] Migrate host connectivity layer to rig v2 #645 diverged.pkg/product/mke/phase/validate_facts_test.go: a test helper missed by the original migration still referenced removed rig v0 types.ERROR_SUCCESS_REBOOT_REQUIRED) detection: rig v2's WinRM error phrasing differs from rig v0's, so the fixed-string match never fired.command.Wait()and theremotefsupload helper had no timeout, so a session that silently died after a host reboot could block forever. Filed and fixed upstream as WinRM command.Wait() and remotefs upload can hang forever on a dead session k0sproject/rig#472 / fix: bound WinRM command.Wait() and remotefs upload against dead sessions k0sproject/rig#473; this PR also adds a launchpad-side bounded-context workaround (go.modcurrently points at the fork commit carrying the upstream fix via areplace, pending it merging and being tagged upstream).JoinWorkers:docker swarm joinsucceeding doesn't guarantee the host's own docker engine has caught up, which a later phase could observe as an empty NodeID.$from the smoke test's generated Windows password (PRODENG-3751 follow-up: launchpad.tf needs to escape$when embedding it into the generated YAML; tracked there).Two further pre-existing bugs were found during this work and fixed separately, unrelated to rig v2: WinRM userdata injection (PRODENG-2579, #662) and envsubst credential corruption (PRODENG-3751, #663/#664).
Testing
go build ./...,go vet ./..., fullgo test ./...clean.golangci-lint runclean (aside from one pre-existing, unrelatedgofumptfinding).Links
Checklist
Written by AI: claude-sonnet-5