Repository navigation
Conversation
… failure-analysis skill kURL-testgrid #1080 added unified cancel endpoints and host-side reconciliation, superseding the skip-then-finish two-step and the note that API cancellation never stops running VMs: - POST /api/v1/ref/{refId}/cancel and /v1/ref/{refId}/cancel cancel a whole run (pending skipped, running finished with cancelled_at) and runner hosts delete the VMs/PVCs/PVs/secrets within one 30s ReconcileCancelledVMIs loop; GET /v1/instances/cancelled is the runner-facing feed. - Documented how to verify a host's tgrun has the reconcile code, and the cases cancel still cannot reach: finished-but-leaked test VMs (with the /v1/instance/{id}/status discriminator) and stuck sendlogs VMs (reaped only at 8h, Shortcut #140012). - Added the dispatch-gate diagnostic: a Pending virt-launcher pod stops that host's dequeue loop (canScheduleNewVM), which is how zombie-VM buildup presents as 'no new tests getting scheduled'.
|
Doomer review
Automatic approval: Enabled |
![doomer-ai[bot]](https://avatars.githubusercontent.com/in/5032069?s=60&v=4){kind=small}
Cancelled instances (cancelled_at set) are deliberate aborts, not test failures; since kURL-testgrid#1080 sets cancelled_at on every cancelled instance, filter them out of fetch.py's failure set and say so in the skill.
…p runbook kURL-testgrid #1084 (stuck -sendlogs VMIs reaped at 30 minutes) and - #1087 (sibling VMs reaped 10 minutes after initialprimary completes, logs shipped first) close the leak paths this section previously sent operators to clean by hand. Manual deletion is now only needed for hosts on an older tgrun or a finished instance whose initialprimary VM never stopped.
The cancel endpoints and runner-side reconciliation (kURL-testgrid #1080) plus the testgrid-checker's latest-comment-prefix behavior were undocumented here; add them to the Testgrid API section.
| if not i.get("isSuccess") | ||
| and not i.get("isUnsupported") | ||
| and not i.get("isSkipped") | ||
| and not i.get("cancelledAt") # cancelled instances are not failures |
There was a problem hiding this comment.
If you have the time
Line 111 changes fetch.py behavior, but the PR claims docs-only. List the filter change in the PR body or split it out, and confirm the field name against a live API response before merge.
Contributor
Author
There was a problem hiding this comment.
Done — the PR body no longer claims docs-only: it now lists the fetch.py filter change explicitly, including the live-API verification table for cancelledAt (cancelled-running vs cancelled-pending vs success vs failure) pulled from the API just now.
|
Review complete. Approval is waiting on 1 unresolved review conversation. Address the comments or explain your disagreement in those conversations. Doomer will check again when they are resolved. Reviewed commit: |
This branch has not been deployed
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.
What this PR does / why we need it:
Updates the
testgrid-failure-analysisskill's cancellation and host-cleanup runbooks for the unified cancel mechanism added in replicatedhq/kURL-testgrid#1080 (merged 2026-10-14), which supersedes the skip-then-finish two-step the skill previously documented — plus two small adjacent corrections found in review.Changes:
POST /api/v1/ref/{refId}/cancel/POST /v1/ref/{refId}/cancel/POST /v1/instance/{id}/cancel(pending → skipped, running → finished withcancelled_at, response counts, Cancelled UI state), the runner-facingGET /v1/instances/cancelled24h feed, and the host-sideReconcileCancelledVMIsbehavior (matches by testid annotation or{instanceId}-prefix, also deletes PVCs/PVs/cloud-init secrets immediately instead of waiting for the ~8h age-based cleanup). Keeps the legacy/v1/skip+/v1/instance/{id}/finishendpoints documented as the fallback for DB-only zombies.-sendlogsVMs reaped on a 30-minute leash) and Automated Kubernetes version update #1087 (sibling VMs reaped 10 minutes after initialprimary completes, logs shipped first) are noted, so manual VMI deletion is only needed for hosts on an oldertgrunor for a finished instance whose initialprimary never stopped. Documents the discriminator endpoints (/v1/instance/{id}/statusreturns{}while running vs{"isSuccess":...}when finished;/v1/instance/{id}/cancelled).virt-launcher-*pod makes a host's runner stop picking up work (canScheduleNewVM), which is how zombie-VM buildup presents as "no new tests getting scheduled". Includes per-host check commands; notes runner heartbeats are statsd-only.fetch.py(small behavior change, not docs-only): the failure filter now excludes instances withcancelledAtset — cancelled-while-running instances otherwise report as failures (cancel setsisSuccess=false,finishedAt,failure_reason='cancelled', and leavesisSkippedfalse) and would be pointlessly log-fetched. The SKILL.md "what it does" line matches.testgrid-checkerlatest-comment-prefix behavior. (This file also puts the PR outside the workflows'paths-ignore: ['.claude/**'], so the full build-test suite runs — see "Steps to reproduce" for verification that this changes no behavior.)cancelledAtfield verified against the live API (runpr-6204-900a11b-containerd-2.3.6-.../-ekco-..., fetched 2026-10-16):All other statements were verified live against the production Testgrid fleet on 2026-10-15/16: cancelled-while-running instances from a cancelled batch had zero VMIs left on the hosts, and a status-discriminating cleanup pass deleted only VMs belonging to finished/cancelled instances while sparing in-flight tests.
Which issue(s) this PR fixes:
None.
Special notes for your reviewer:
Skill/docs + one two-line behavior filter in
fetch.py. kURL-testgrid#1080's rollout note matters here: the cancel API exists in tgapi regardless, but each runner host'stgrunmust be new enough to reconcile (strings /bin/tgrun | grep -c ReconcileCancelledVMIs); the prd hosts were verified to have it as of 2026-10-15 22:00Z. The sendlogs/sibling fixes (#1084/#1087) are merged but likewise need a host rollout to take effect.Steps to reproduce
Read the rendered SKILL.md sections "Cancelling queued or stuck work via the API" and "Cleaning up VMs on the Testgrid hosts". The fetch.py filter change is covered by the live-API field table above; CI runs the full build-test suite (AGENTS.md is outside the workflows'
.claude/**paths-ignore).Does this PR introduce a user-facing change?
Does this PR require documentation?
NONE