fix: surface unknown eviction conditions in draincluster

[Akshita kumari (akshita317)]

Description of your changes

Fixes #659.

The draincluster command waits for each eviction to reach a terminal state and then inspects its conditions, but it only handled the True/False cases of the Valid and Executed conditions. A missing or Unknown condition fell through:

• an Unknown Executed condition was neither nil nor False, so the loop treated the eviction as successfully executed and reported the drain as complete;
• a missing/Unknown Valid condition was likewise not surfaced.

This masked uncertain evictions as successful drains. This change implements the pre-existing // TODO: add safeguards to check if eviction conditions are set to unknown.

Now a missing or Unknown Valid or Executed condition marks the drain unsuccessful and logs that the drain could not be confirmed, instead of silently proceeding. The existing True/False handling is unchanged.

I have:

• Associated this change with a known KubeFleet Issue (Bug, Feature, etc).
• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

go build ./tools/fleet/..., go vet, and the existing go test ./tools/fleet/cmd/draincluster/... all pass.

Note on tests: the existing suite covers the drain helper functions (fetchClusterResourcePlacementNamesToEvict, cordon, etc.), but not the main drain loop, because it creates an eviction and waits for it to reach a terminal state — which the fake client cannot drive without a refactor. If you'd like, I'm happy to extract the eviction-condition evaluation into a small pure helper in a follow-up so this safeguard (and the surrounding True/False handling) can be unit-tested directly.

Special notes for your reviewer

Behavior change is limited to previously-unhandled missing/Unknown conditions; True/False paths are untouched.

[Copilot]

Pull request overview

This PR tightens draincluster’s eviction-condition handling so that drains are no longer reported as successful when eviction Valid/Executed conditions are missing or Unknown, aligning behavior with the intent in Issue #659 and the existing TODO.

Changes:

• Treat missing/Unknown Valid condition as an unsuccessful drain and surface it via logging.
• Treat missing/Unknown Executed condition as an unsuccessful drain and surface it via logging.

Comments suppressed due to low confidence (1)

tools/fleet/cmd/draincluster/draincluster.go:202

• The invalid-eviction special-casing is comparing validCondition.Reason against condition.EvictionInvalid*Message constants, but those constants are used as the Condition.Message when the controller marks an eviction invalid (the Reason is ClusterResourcePlacementEvictionInvalid). As a result, this branch won’t match and an invalid eviction can fall through to the Executed check and be reported as a successful drain. Also, when Valid is False for any other message, the drain should be marked unsuccessful instead of proceeding.

		if validCondition.Status == metav1.ConditionFalse {
			// check to see if CRP is missing or CRP is being deleted or CRB is missing.
			if validCondition.Reason == condition.EvictionInvalidMissingCRPMessage ||
				validCondition.Reason == condition.EvictionInvalidDeletingCRPMessage ||
				validCondition.Reason == condition.EvictionInvalidMissingCRBMessage {

[Akshita kumari (akshita317)]

Thanks for the review! Good call — I've extracted the Valid/Executed evaluation into a pure evaluateEviction helper and added table-driven tests covering the True/False/Unknown/missing combinations (and the invalid-but-drained reasons). No behavior change.

Note: the failing staticcheck and e2e-tests (resourceplacement) checks look unrelated to this change — staticcheck ./... passes cleanly for me locally on the full repo, and the resourceplacement e2e doesn't exercise the draincluster CLI. Happy to rebase or dig in if either persists.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

tools/fleet/cmd/draincluster/draincluster.go:245

• evaluateEviction currently treats any condition status other than False/Unknown as success (e.g., an empty/invalid Status value will be classified as executed). Since the intent is to avoid masking uncertainty, it would be safer to only treat explicit ConditionTrue as success and treat any other non-False value as indeterminate.

	validCondition := eviction.GetCondition(string(placementv1beta1.PlacementEvictionConditionTypeValid))
	if validCondition == nil || validCondition.Status == metav1.ConditionUnknown {
		return evictionResultIndeterminate
	}
	// An invalid eviction whose reason is a missing/deleting CRP or a missing CRB

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

tools/fleet/cmd/draincluster/draincluster.go:251

• The invalid-but-drained shortcut is matching against validCondition.Reason, but the eviction controller sets these constants in the condition Message (Reason is a generic ClusterResourcePlacementEvictionInvalidReason). As written, real evictions that are invalid due to missing/deleting CRP or missing CRB will be misclassified and could be treated as indeterminate/failed instead of the intended success path.

	// An invalid eviction whose reason is a missing/deleting CRP or a missing CRB
	// still means the cluster is drained.
	if validCondition.Status == metav1.ConditionFalse &&
		(validCondition.Reason == condition.EvictionInvalidMissingCRPMessage ||
			validCondition.Reason == condition.EvictionInvalidDeletingCRPMessage ||
			validCondition.Reason == condition.EvictionInvalidMissingCRBMessage) {
		return evictionResultInvalidButDrained

[Akshita kumari (akshita317)]

Thanks — both good catches, addressed in the latest push:

• Missing default/executed case: the drain switch now has an explicit evictionResultExecuted case plus a default that marks the drain unconfirmed, so a future/unexpected evictionResult can never fall through to a "successful" drain.
• Non-True statuses: evaluateEviction now treats only an explicit True/False status as definitive — Unknown, empty, or any unexpected status maps to indeterminate. Added a test case for an empty/invalid Executed status (it caught the original gap).

On the red unit-and-integration-tests check: it's an unrelated flaky integration test — pkg/controllers/bindingwatcher TestAPIs timed out after 10s on an Eventually enqueue check (watcher_integration_test.go:751). The cmd/hubagent/options and cmd/memberagent/options tests both passed (ok), and my changed package's tests pass locally. This push re-runs CI; happy to rebase if the flake persists.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Akshita kumari (akshita317)]

Following up on this one, as it has been sitting on a red check since early August.

The failing e2e-tests (default) job does not exercise anything this PR changes — the diff is limited to tools/fleet/cmd/draincluster/, a standalone CLI, while the failure is in test/e2e/webhook_test.go:1715 (CRP placement / webhook AfterAll teardown). That same job also fails intermittently on main (e.g. run 30952331356).

The branch is now ~27 commits behind main, so I am happy to rebase it to get a fresh CI run — just say the word. I can also squash the four commits into one if you would prefer that for the merge.

[Simon Waight (sjwaight)]

Yetkin Timocin (@ytimocin) as raiser of original issue can you please review. Thanks.