Repository navigation
atelet: clear node-local caches after a node reboot - #3376
Dmitry Berkovich (dberkov) merged 2 commits into
Conversation
1984c47 to
d5d64b6
Compare
|
LGTM, ty for the quick fix and keeping it simple! |
|
This appears to be an issue with the cache garbage collector. Since the same out-of-disk-space error would eventually occur if a node remains running for an extended period, what makes a VM reboot unique in this scenario? |
|
Dmitry Berkovich (@dberkov) This isn't an out-of-disk-space error, and it doesn't happen on a node that keeps running. A hard reboot is unique because it loses file data that hadn't reached the disk yet. The caches write each entry to a temp dir and then rename it into place. They don't flush the data to disk before the rename. After a hard reset, the rename can survive while the file contents never reached the disk. The entry then looks complete, but its files are empty or truncated. In my GKE repro, one was a zero-length Half-downloaded entries aren't the problem. Each cache already removes its temp dirs when atelet starts. The broken files are in finished entries, which look complete. The startup temp-dir cleanup skips them because they aren't in a temp dir, and GC skips them because nothing marks them as broken. The crash has a second, separate effect on GC. It can also break an actor's bundle spec, which lists the image layers that actor uses. That spec is GC's only record of which layers a running actor still needs, because a long-running actor's layers can look old and unused. When GC can't read a spec, it can't tell which layers to keep. Evicting anything could then delete a layer a running actor still needs, so GC evicts nothing from any image. Clearing the bundles after a reboot removes those broken specs. Keeping only the good entries would need a way to tell them apart. File cache entries record only a key and a time, so snapshot cache entries can't be checked. Image layers would need a re-hash of every layer after each reboot. Flushing each entry to disk before the rename would also prevent this. That's option 2 in #3351. It slows every cache fill, so it needs a measurement first, and it doesn't fix entries already broken on a node. This PR clears the caches instead. The cost is cold caches after a reboot, so the next actors on the node download their images and snapshots again. A follow-up could keep warm caches across planned reboots. atelet would flush the filesystem to disk when it shuts down gracefully, and record that it did. On the next start it would skip the clear. Only a crash would then clear the caches. |
| if err != nil { | ||
| serverboot.Fatal(ctx, "Failed to read the node's boot ID", err) | ||
| } | ||
| cleared, err := clearCachesAfterReboot(bootID, ateletpath.CacheBootIDFile, nodepath.ActorsDir, nodepath.WorkersDir(), |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This runs before validateSnapshotCacheFlags (line 301), which is what rejects a --snapshot-cache-dir outside /var/lib/ate. After a reboot, a misconfigured directory gets emptied and only then does atelet refuse to start. Move this block below the three cache flag validations so nothing is removed from a directory the flags would reject.
There was a problem hiding this comment.
Fixed. The three cache flag validations now run before the clear, so a rejected directory is never emptied. The sandbox asset dir's hard-link check still runs inside openSandboxAssetCache, so the reorder doesn't cover that one.
| b, err := os.ReadFile(markerPath) | ||
| switch { | ||
| case errors.Is(err, fs.ErrNotExist): | ||
| return false, writeCacheBootID(markerPath, bootID) |
There was a problem hiding this comment.
🤖 question 🟢 – A missing marker only records the boot, so the first start of this build on a node torn by an earlier reboot keeps serving the torn entries until the node reboots again. The description says deploying this onto the node main had left torn recovered it. Was that node rebooted between? If staying torn until the next reboot is the intended trade-off, it is worth saying so in the description, since operators hitting #3351 today will upgrade onto exactly such nodes.
There was a problem hiding this comment.
Yes, that's the intended trade-off. A missing marker only records the boot, because actors may be running on the caches during the upgrade. So a node that is already broken stays broken until it reboots again. The recovery claim in the description was wrong, and I removed it. The description now says such a node needs one reboot or a replacement after upgrading.
| restart the directories of pods still on the node keep protecting their | ||
| defaults with no wait on the syncer, and the startup sweep removes the | ||
| directories of pods that are gone. | ||
| - Cache writes aren't synced, so after a node reboot atelet empties the cache |
There was a problem hiding this comment.
🤖 nit 🟢 – The "What is on disk" table above lists every file under /var/lib/ate this package touches. Add a row for cache-boot-id so the marker is discoverable from the layout rather than only from this bullet.
There was a problem hiding this comment.
Added a cache-boot-id row to the table.
lets not close the real issue, since this is a temporary workaround only. The real fix needs to be different and integrate with file system sync. |
Cache writes aren't synced, so an unclean reboot can leave cache entries, image layers and bundle overlay specs empty or truncated. atelet then serves them, every restore on the node fails, and the image cache GC stops on the unreadable spec. At startup, compare the boot ID with the one recorded when the caches were last written. If the node has rebooted since, empty the image, sandbox asset and snapshot caches, and remove each actor's bundle dir and each worker's default-runtime dir, which hold specs naming cached layers and hard links to cached assets. A missing record only records the boot, since actors may be running on the caches when atelet first starts with this check. Signed-off-by: Ivy <igooch@google.com>
d5d64b6 to
96f3bf7
Compare
A rejected cache directory is then never emptied. Also list cache-boot-id in the sandbox asset cache's on-disk layout. Signed-off-by: Ivy <igooch@google.com>
05ff550 to
5a1c6bc
Compare
Part of #3351. This is a workaround. The real fix is for the caches to flush their writes to disk.
atelet doesn't sync cache writes, so an unclean reboot can leave cache entries, image layers and bundle overlay specs empty or truncated. atelet then serves them, every restore on the node fails, and the image cache GC stops on the unreadable spec.
At startup, atelet now compares the node's boot ID with the one it recorded when the caches were last written (
/var/lib/ate/cache-boot-id). If the node rebooted since, it empties the image, sandbox asset and snapshot caches, and removes each actor'sbundlesdir and each worker'sdefault-runtimedir. Those hold specs naming cached layers and hard links to cached assets, which atelet would otherwise reuse. Run and the worker sandbox syncer recreate them. This is the approach #3050 took for local snapshots. The rest of an actor's dir stays. Run and Restore rebuild it before use, and Terminate of an actor that crashed in the reboot names no worker, so it doesn't read the actor's sandbox record or asset links.The cost is cold caches after any reboot. When the record is missing, atelet only records the boot, since actors may be running on the caches the first time it starts with this check. So a node that is already broken stays broken after the upgrade until it reboots again. Reboot or replace such a node once after upgrading.
On a GKE node, I filled the snapshot and image caches, waited 10s and hard-reset the VM. On main this tore 92 image cache and 10 snapshot cache files, and both a golden restore and a new actor from the torn image failed with
unexpected end of JSON input. With this change, the caches were cleared at startup and both resumes succeeded.I used an AI agent (Claude Code) to write this change and its test, test it on a GKE node with a hard reset, and draft this description.