Handle graceful termination in microvm ateom - #893
Conversation
a584eb0 to
d674bb8
Compare
|
Benjamin Elder (@BenTheElder) PTAL I've patched the e2e test fixes from #904 just to get a passing run, but will submit and rebase once that lands. |
|
that's in, taking a look at the other commit |
d674bb8 to
3766157
Compare
| // Obtain a kata-agent client to signal the guest: reuse the log-forwarding | ||
| // connection if it's open, else dial a fresh one (best-effort). A dial we open | ||
| // here is closed here; ra.guestAgent belongs to the log forwarder. | ||
| agent := ra.guestAgent |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This reads ra.guestAgent with lock released, and a concurrent teardown writes it. gracefulShutdown drops the lock before calling here so a suspend can still land, and CheckpointWorkload is deliberately not gated by rejectIfDraining. Its teardownActor does ra.guestAgent.Close() followed by ra.guestAgent = nil, so the field is written while this goroutine reads it — an unsynchronised read/write that -race will flag.
The observable damage is mild: a stale pointer means signalling a closed client and logging the error, and a nil one means fifteen seconds of dialAgentRetry against a VM that is being destroyed. Both are bounded and already tolerated.
The comment above maps.Clone has the right instinct about the map; the same reasoning applies one level down to the struct it hands out. Capturing what the drain needs — the agent and workloadIDs — into a value while the lock is still held would close it, and workloadIDs is immutable after construction so only the agent really needs copying.
Part of #23
Trap SIGTERM in the ateom, and forward SIGTERM to all application containers.
Once all application containers exit, ateom will exit itself.