volumemgr: persist deferred content-tree deletes - #6085
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6085 +/- ##
==========================================
+ Coverage 22.38% 22.84% +0.46%
==========================================
Files 492 503 +11
Lines 92128 93954 +1826
==========================================
+ Hits 20620 21463 +843
- Misses 69834 70636 +802
- Partials 1674 1855 +181 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eriknordmark
changed the title
|
Claude finished @rene's task in 5m 10s —— View job Code Review
The approach is sound: replacing the in-memory
The acknowledged limitation (cancel keys on ContentID rather than image reference, so a redeploy with a new ContentID leaves the old record until expiry) is harmless and clearly documented. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| status.Key(), len(rec.Blobs), expiry) | ||
| if err := ctx.pubDeferredDelete.Publish(rec.Key(), rec); err != nil { | ||
| log.Errorf("deleteContentTree: publish deferred delete %s: %s", rec.Key(), err) | ||
| } |
There was a problem hiding this comment.
If Publish fails, execution falls through to RemoveAllBlobsFromContentTreeStatus + unpublishContentTreeStatus. At that point blobDeferredProtected returns false (no record was written), so blobs with RefCount=0 are immediately removed from CAS — silently defeating the deferral. Consider falling back to doDeleteContentTree on error:
if err := ctx.pubDeferredDelete.Publish(rec.Key(), rec); err != nil {
log.Errorf("deleteContentTree: publish deferred delete %s: %s, falling back to immediate delete", rec.Key(), err)
doDeleteContentTree(ctx, status)
return
}There was a problem hiding this comment.
Publish never fails, but code typically ignores the errors. We could alternatively do a log.Fatal here.
| // instead of being deleted then re-downloaded. Persist the (possibly | ||
| // decremented) RefCount rather than dropping it, so a later within-boot | ||
| // expiry sees the real count. | ||
| if blobDeferredProtected(ctx, blob.Sha256) { |
There was a problem hiding this comment.
blobDeferredProtected does a full O(records × blobs-per-record) scan on every call, and it is called once per blob inside RemoveRefFromBlobStatus. Deleting an app whose image has many layers therefore runs this scan once per layer. Fine for the typical case (a handful of deferred records with a handful of blobs each), but if the defer timer is long and many apps are cycling, the overhead compounds. A pre-built map[sha]bool computed once per checkDeferredDelete sweep (and stored on the context) would make all individual checks O(1).
|
|
||
| for _, sha := range rec.Blobs { | ||
| blob := ctx.LookupBlobStatus(sha) | ||
| if blob == nil { |
There was a problem hiding this comment.
blob == nil after populateInitBlobStatus is unusual (it means the blob was already evicted from CAS or never recorded). When that happens the corresponding CAS blob is silently skipped — no RemoveBlob is attempted, and the blob lingers in CAS until the next ordinary GC sweep. That's acceptable, but worth a log.Warnf so it's visible in field logs.
9b91511 to
022ae34
Compare
022ae34 to
eb27466
Compare
A content-tree delete deferred by timer.defer.content.delete kept the content's CAS blobs only via in-memory state and the still-published ContentTreeStatus, both lost on reboot. After the EVE-kvm->EVE-k boot-disk conversion (which deletes the running app before repartitioning and reboots) the blobs were re-adopted unreferenced and the boot-time GC could reap them before the app was redeployed, forcing a full re-download of an image whose bytes were still on the preserved /persist. Record each deferred delete as a Persistent DeferredContentDeleteStatus (content ID, image reference, blob list, expiry). It is the single source of truth: the in-memory deferredDelete slice and the practice of leaving the ContentTreeStatus published to hold a RefCount are gone. While a record is unexpired the blob and image GC reapers skip the listed content, so the blobs survive the conversion reboot regardless of GC-vs-redeploy timing; the redeploy re-creates the content tree, cancels the record, and reuses the blobs. At expiry the record drives the delete, leaving any blob a live content tree now references. Drain any pending global config before servicing deletes: when the controller raises timer.defer.content.delete and deletes an app in the same update, the nondeterministic select order could otherwise handle the delete while the defer value was still 0, taking the immediate-delete branch and reaping the blobs the operator meant to retain. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eb27466 to
c87a490
Compare
2 participants
Motivation: a bug found while testing the kvm-to-k (EVE-kvm↔EVE-k) conversion — deferred content-tree deletes did not survive the conversion reboot. This fix makes timer.defer.content.delete more useful in general but required for the kvm-to-k tests which are being developed.
Description
One way to both test and use the WIP EVE-kvm to EVE-k conversion without having
to re-download app images/containers is to ensure that the content tree is not
deleted (by setting the defer timer), delete the app instance, upgrade/convert
to EVE-k, then re-deploy the app instance. That assumes that the defer timer
works correctly even when there are delays and device reboots. This PR makes it
reliable in those conditions.
A content-tree delete deferred by
timer.defer.content.deletekeeps thecontent's CAS blobs so a quick re-deploy reuses them instead of deleting and
re-downloading. That deferral was tracked only in memory plus the
still-published
ContentTreeStatus, both lost on reboot. So when a reboot fallsinside the defer window — notably the EVE-kvm→EVE-k boot-disk conversion, which
deletes the running app before repartitioning and reboots — the blobs were
re-adopted unreferenced and the boot-time GC could reap them before the app was
redeployed, forcing a full re-download of an image whose bytes were still present
on the preserved
/persist.This records each deferred delete as a
PersistentDeferredContentDeleteStatus(content ID, image reference, blob list, expiry). It becomes the single source of
truth: the in-memory
deferredDeleteslice and the practice of leaving theContentTreeStatuspublished only to hold a RefCount are removed. While a recordis unexpired the blob and image GC reapers skip the listed content, so the blobs
survive the reboot regardless of GC-vs-redeploy timing; the redeploy re-creates
the content tree and reuses them. At expiry the record drives the delete, leaving
any blob that a live content tree now references.
WIP / known follow-up:
cancelDeferredDeletekeys on content ID, but aredeploy is assigned a new content ID, so the original record is not cancelled
and lingers until its expiry (harmless — at expiry the now-referenced blobs are
skipped). Keying cancellation on the image reference / blob set would drop the
record promptly on redeploy.
How to test and validate this PR
(
pkg/pillar/cmd/volumemgr/deferredcontentdelete_test.go).timer.defer.content.delete, deploy an app, delete the app instance, convertto EVE-k (boot-disk repartition + reboot), then re-deploy the app. The
DeferredContentDeleteStatusrecords survived the conversion reboot, the bootGC ran and the guards kept the blobs and image, and the app redeploy
re-downloaded 0 bytes.
Changelog notes
No user-facing changes.
PR Backports
Checklist
And the last but not least:
check them.