Repository navigation
Backport small fixes to v0.17 for v0.17.3 - #1428
Conversation
vmagent remote-writes metrics with a bearer token, which makes it the one bundled component besides BuildKit that verifies public certificates. v1.150.0 was built on go1.26.6, so it still trusted chains through the eMudhra intermediates that CVE-2026-111943 distrusts, and a forged certificate on the remote-write path could capture the token. v1.155.0 is the first VictoriaMetrics release on a fixed Go (1.27.3). The only breaking change since v1.150.0 is to vmselect's /admin/tenants, which vmagent does not touch.
A server that dies without running its stop hooks (a panic, a SIGKILL) leaves its embedded containerd running under init, still holding the socket and the bolt metadata lock. Every daemon started after that blocks forever opening the store, while waitForSocket happily connects to the old one. Long-lived hosts ended up running a months-old containerd binary with stale environment, plus one stuck daemon for every restart since. Start now takes an exclusive flock on owner.lock in the base dir and holds it until Stop. The kernel drops it when the process dies, so holding it proves no live server owns whatever containerd is still running there; a second server on the same dir refuses to start instead of killing the first one's daemon. With the lock held, Start finds every containerd running from our config and stops it (SIGTERM, then SIGKILL after 30s). It has to take all of them, not just the socket owner: once the owner releases the bolt lock, a stuck daemon left alive would grab it and take the socket. Candidates match on the config file's identity through /proc/<pid>/root rather than its path, since a containerd in a container can use the same path for an unrelated file, and signals go through pidfd-backed os.Process handles so a reused pid is never hit. Shims are separate processes, so sandboxes ride through. Once the socket is up, Start checks via SO_PEERCRED that the daemon answering is the one it just started, and it fails fast if that daemon exits before listening.
Every server shutdown that followed recent API use stalled for up to 30 seconds in cluster-foundation's stop, with nothing running. The HTTP/3 drain waited for every accepted connection to close, and a peer that goes away without closing its connection leaves the server holding it until the QUIC idle timeout, 30s after its last packet. Both prod upgrade journals (v0.16.2 and v0.17.0) show the ~16s of idle shutdown ending 30s after some peer went quiet. By the time the drain starts, Shutdown has cancelled the State's top context and ServeHTTP refuses new requests, so the only work a drain can protect is handlers already running. The server now counts those, and the WebTransport drain ends once none has run for a short settle, leaving Shutdown to close what remains. A dev server that took 26.6s to stop after a single `m app list` now stops in 0.6s.
Context.rpcClient built an rpc.State for every connection and dropped it, so a command exited without sending QUIC CONNECTION_CLOSE and the server held the connection until its 30s idle timeout. A server shutting down within that window waited on it. The Context now keeps the states it opens and closes them in Close, which every command defers. State.Close already sends the close frame. With only this change, stopping a dev server 3s after `m app list` takes 0.35s instead of 26.6s. The previous revision is what keeps shutdown fast regardless: older CLIs, commands killed mid-flight, and peers whose network drops all still leave a dead connection behind.
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
eaea539 to
154027e
Compare
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
I think this is ready for a human to look at. The upgrade-behavior concern I raised is gone, and the one thing left needs a person to sign off on the rollout, not a code change.
#1411 is out of the batch. controllers/sandboxpool/manager.go, the scaling.md warning and the #1411 changelog entry are all gone from this layer's diff. checkPoolForScaleDown still skips sandboxes with a zero LastActivity (manager.go:698-699), so long-running TCP/UDP-only services keep their current behavior on upgrade. Holding it for the next minor resolves the concern, and I've closed that thread.
Sandboxpool tests. The description now covers controllers/sandboxpool on the combined tree. You report one failure, TestManagerLongStartupFailureBackoff, which you say fails the same way on plain v0.17.2. I haven't run it myself, and with #1411 gone this layer doesn't touch that package anyway.
The release commit. 5a90252 and 154027e only change the changelog. The #1387 entry from #1427 now sits under ## v0.17.3 with the three entries for this batch.
What a human should check before tagging: the containerd rollout on hosts that already have leftover daemons (eu1, by your note). Two outcomes are possible:
- The leftover daemons match.
findSurvivorsonly treats a process as ours if its--addressequals our socket path and its--configpoints at the same file as<data>/containerd/config.toml. The match is by identity, and that works becausegenerateDefaultConfigrewrites the file in place, so the inode stays the same. Matching daemons get SIGTERM, then SIGKILL after 30s, and the containerd version changes on that restart. That's the outcome the changelog describes. - A leftover daemon doesn't match. For example, it was started with a different
--configpath. Then it isn't stopped. Our new daemon blocks on the bolt lock,waitForSocketconnects to the old daemon, andcheckSocketOwnerfails (containerd.go:247-258). So the server fails to boot, where before it limped along on the old daemon. Failing fast is what #1416 intends, but for that host it means an outage instead of a silent degradation.
Before upgrading eu1, it's worth checking /proc/<pid>/cmdline on its stray containerd processes to confirm they were started with --address <data>/containerd/containerd.sock --config <data>/containerd/config.toml.
The merge order is the same as before: wait for #1427, retarget to release/0.17, then merge with a merge commit.
I'm not recommending that this skip human review. It's a release cut that kills processes on production hosts, and someone who knows those hosts should sign off on the rollout.
🍪 full review note · reviewed at 154027e
| if err := c.waitForSocket(ctx, config.SocketPath); err != nil { | ||
| err = c.waitForSocket(ctx, config.SocketPath, waitDone) | ||
| if err == nil { | ||
| err = checkSocketOwner(config.SocketPath, cmd.Process.Pid) |
There was a problem hiding this comment.
I'm flagging this for a human to check on the affected hosts. It isn't a defect. If a stray containerd on this socket isn't matched by findSurvivors, for example because it was launched with a different --config path, it's never stopped. Our new daemon then blocks on its bolt lock, waitForSocket succeeds against the stray one, and this check fails Start. The server then refuses to boot, where before this change it ran on the stray daemon. That's the intended fail-fast, but before rolling v0.17.3 onto eu1 someone should confirm its leftover daemons' cmdlines carry --address <data>/containerd/containerd.sock --config <data>/containerd/config.toml, so they're cleaned up instead of blocking boot.
Backports #1425, #1416, and #1408 to release/0.17, and stamps v0.17.3 as the last commit, so the release doesn't need another review round. These are the small fixes that go out alongside #1427.
This is stacked on #1427 so CI can run now. It needs retargeting to release/0.17 once #1427 merges, and it should be merged with a merge commit so each fix can still be reverted by itself.
All three are straight cherry-picks with identical diffs to main:
One thing to know before this ships: the first restart on this build stops any leftover containerd daemons, so a host in that state changes containerd version on that restart, even though sandboxes keep running. eu1 appears to be in that state.
I pulled #1411 from this batch. It starts scaling down long-running auto-mode sandboxes with no recorded ingress activity, which would take TCP/UDP-only services to zero on upgrade. That's too big a change for a patch, so it can ship in the next minor.
Tested on the combined tree: it builds, and
pkg/imagerefs,components/containerd,pkg/rpc,pkg/concurrency, andservers/httpingresspass. Incontrollers/sandboxpool, onlyTestManagerLongStartupFailureBackofffails, on a UTC vs local time comparison, and it fails the same way on plain v0.17.2.TestRunCommandPreservesExplicitShellExitalso failed incli/commandsbefore this was combined, the same way as on plain v0.17.2.