Repository navigation
fix(app): guard onPreRemove against a nil k8sd client - #141
Draft
bschimke95 wants to merge 1 commit into
Draft
bschimke95 wants to merge 1 commit into
bschimke95 wants to merge 1 commit into
Conversation
microcluster invokes the PreRemove hook as rollback when bootstrap or
join fails, not only on explicit node removal. testenv.WithState's
snap mock never configured a K8sdClient, so snap.K8sdClient("")
returned (nil, nil) and onPreRemove panicked calling a method on a nil
interface, aborting the in-flight HTTP response and failing bootstrap
with an opaque EOF (TestClusterAPIAuthTokens and other WithState-based
tests across pkg/k8sd/app, pkg/k8sd/database, pkg/k8sd/api,
pkg/k8sd/features/cilium).
Skip the PENDING wait when the client is nil instead of crashing, and
wire a working GetClusterMember mock into the test harness so the
documented rollback path can complete.
Also replace TestOnPreRemoveNodeAbsentFromCluster's wall-clock
assertion (ctx.Err() against a 5s timeout) with a call-count check,
since elapsed time is not deterministic under CI scheduling load.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The core nil-client regression path lacks deterministic test coverage.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Prevents rollback-time nil-pointer panics in onPreRemove and stabilizes related tests.
Changes:
- Skip membership polling when no k8sd client exists.
- Configure the test state with a not-found client mock.
- Replace timing-based verification with call-count verification.
| File | Description |
|---|---|
pkg/k8sd/app/hooks_remove.go |
Guards the pre-remove wait against a nil client. |
pkg/utils/microcluster/state.go |
Adds a rollback-safe client mock. |
pkg/k8sd/app/hooks_remove_test.go |
Makes the membership-loop assertion deterministic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+47
to
49
| g.Expect(mockK8sdClient.GetClusterMemberCalledWithNames).To(HaveLen(1)) | ||
| }) | ||
| } |
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
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.
TestClusterAPIAuthTokensand a few other tests fail intermittently in CI with a nil-pointer panic inonPreRemove(https://github.com/canonical/k8sd/actions/runs/37477175533/job/112315519419). Microcluster callsPreRemoveas a rollback step when bootstrap/join fails, but our test harness (testenv.WithState) never configured aK8sdClienton its mock snap, so the hook called a method on a nil interface and crashed.TestOnPreRemoveNodeAbsentFromClusterfailed alongside it for a different reason: it asserted on elapsed wall-clock time, which isn't reliable under CI load.Change
onPreRemove: skip the PENDING wait if the k8sd client is nil, instead of panicking.testenv.WithState: give the mock snap a realGetClusterMembermock so the rollback path has something to call.TestOnPreRemoveNodeAbsentFromCluster: assert on call count instead ofctx.Err().