Skip to content

SK-251 // Don't panic on failed delete#250

Merged
ogorman89 merged 4 commits into
mainfrom
ian/failed-delete-panic
May 23, 2026
Merged

SK-251 // Don't panic on failed delete#250
ogorman89 merged 4 commits into
mainfrom
ian/failed-delete-panic

Conversation

@ogorman89
Copy link
Copy Markdown
Contributor

Description and Rationale

  • driver currently panics if a delete fails for any reason causing the simulation to fail
  • most frequently this is caused by an edited trace file (SKEL)

How

  • instead of propagating any error from the delete request to the kube API we just log it
  • all other errors will propagate like usual

Test Steps

  • This was instructive to test it took me a little while to reproduce, the easiest way to to create a trace with some basic applys and deletes in a sample cluster then use SKEL to delete the apply for one of the resources causing a delete without a created object within the trace. That is sufficient to reliably recreate the error

Previously the driver fails when the delete request returns a 404, crashing the driver:

Screenshot 2026-05-21 at 8 39 30 PM

After the error handling change we just see a warning in the driver pod log:

Screenshot 2026-05-21 at 8 25 02 PM

Other Notes

  • if we want to make it even more specific we could only suppress the 404 erros instead of all errors returned by delete() but that doesn't seem necessary.

  • [ X ] I certify that this PR does not contain any code that has been generated with GitHub Copilot or any other AI-based code generation tool, in accordance with this project's policies.

@linear
Copy link
Copy Markdown

linear Bot commented May 22, 2026

SK-251

@ogorman89 ogorman89 requested a review from drmorr0 May 22, 2026 03:41
@ogorman89 ogorman89 self-assigned this May 22, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.30%. Comparing base (05ae6be) to head (abc9754).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sk-driver/src/runner.rs 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
- Coverage   78.37%   78.30%   -0.08%     
==========================================
  Files          62       62              
  Lines        3852     3853       +1     
==========================================
- Hits         3019     3017       -2     
- Misses        833      836       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread sk-driver/src/runner.rs Outdated
.await?;
.await
{
tracing::warn!("delete failed, continuing: {:?}", e)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already import tracing::* so you can just use warn! here.

Copy link
Copy Markdown
Contributor

@drmorr0 drmorr0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am of two minds here.... there's part of me that just wants to scope this down to only 404s (if we get some other error we probably ??? want to know about it). On the other hand, I'm real tired of simulations failing for dumb shit like this.

I think on the balance, probably the right thing to do is to scope it down to 404s only, though.

@ogorman89 ogorman89 merged commit e03c31f into main May 23, 2026
6 of 7 checks passed
@ogorman89 ogorman89 deleted the ian/failed-delete-panic branch May 23, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants