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:
findSurvivors only treats a process as ours if its --address equals our socket path and its --config points at the same file as <data>/containerd/config.toml. The match is by identity, and that works because generateDefaultConfig rewrites 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.--config path. Then it isn't stopped. Our new daemon blocks on the bolt lock, waitForSocket connects to the old daemon, and checkSocketOwner fails (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.
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.Posted to the PR's comment threads when you submit.
manager.go and scaling.md are no longer changed, the #1411 changelog entry is gone, and zero-LastActivity sandboxes are still skipped in checkPoolForScaleDown. Existing services won't start scaling down on upgrade, so I'm closing this.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1428,
"verdict": "ready",
"event": "comment",
"summary": "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.\n\n**#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.\n\n**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.\n\n**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.\n\n**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:\n\n- **The leftover daemons match.** `findSurvivors` only treats a process as ours if its `--address` equals our socket path and its `--config` points at the same file as `\u003cdata\u003e/containerd/config.toml`. The match is by identity, and that works because `generateDefaultConfig` rewrites 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.\n- **A leftover daemon doesn't match.** For example, it was started with a different `--config` path. Then it isn't stopped. Our new daemon blocks on the bolt lock, `waitForSocket` connects to the old daemon, and `checkSocketOwner` fails (`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.\n\nBefore upgrading eu1, it's worth checking `/proc/\u003cpid\u003e/cmdline` on its stray containerd processes to confirm they were started with `--address \u003cdata\u003e/containerd/containerd.sock --config \u003cdata\u003e/containerd/config.toml`.\n\nThe merge order is the same as before: wait for #1427, retarget to `release/0.17`, then merge with a merge commit.\n\nI'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.",
"comments": [
{
"path": "components/containerd/containerd.go",
"line": 249,
"side": "RIGHT",
"body": "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 \u003cdata\u003e/containerd/containerd.sock --config \u003cdata\u003e/containerd/config.toml`, so they're cleaned up instead of blocking boot."
}
],
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6zvPSh",
"body": "Thanks, I agree this belongs in a minor rather than a patch. In this layer, `manager.go` and `scaling.md` are no longer changed, the #1411 changelog entry is gone, and zero-`LastActivity` sandboxes are still skipped in `checkPoolForScaleDown`. Existing services won't start scaling down on upgrade, so I'm closing this.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"draft": true,
"head_sha": "154027e4d87cf29bbf015adf622503fcfbc9a953"
}I think this is ready for a human to look at. I didn't find a correctness blocker in this layer. It's a release cut, though, and two rollout effects need a person to sign off, so I'm not recommending that it skip human review.
I read each backport against the 0.17 tree it now sits on, not against main. The helpers the new tests depend on are all present on this base: Store.NowFunc in pkg/entity/mock.go, plus reconcilePool, getPool and listSandboxesForPool in the sandboxpool tests. So these look like clean cherry-picks, not ones that only compile on main.
In pkg/rpc, the request counter covers the paths a drain needs to protect. startCallStream runs runCallStream synchronously inside ServeHTTP, and Clone gives each sub-state its own counter. The /_rpc/drain watch handler returns as soon as top is cancelled, so it can't hold a drain open. In containerd, the owner lock is opened through os.OpenFile, which sets close-on-exec, so the spawned daemon can't inherit the lock and outlive it. TestStart_LeavesLiveOwnersDaemonAlone covers the case that matters most there: a second server must not kill a live owner's daemon.
Where a human should look:
LastActivity was skipped by checkPoolForScaleDown. It now ages from createdAt (manager.go:696-699). The new readiness stamp in setRunning and in stale-PENDING recovery only covers sandboxes that start after the upgrade. Existing long-lived auto-mode sandboxes that never recorded ingress activity become idle on the first monitor tick after the upgrade. That includes services that only get TCP/UDP, node-port or direct service-to-service traffic. They scale to zero, and per the new scaling.md warning, that traffic won't wake them. That's the intended fix, but in a patch release it's a visible behavior change. The description's pre-ship note covers the containerd restart but not this. I've left a note on the changelog entry.controllers/sandboxpool isn't among them. That's where #1411's main new test (TestManagerScaleDownNeverLeasedSandboxes) lives, and it may also be where #1427's scale-down changes interact with this one. That test uses the in-memory entity server, so it shouldn't need the bridge permissions that kept controllers/sandbox off your list. Running it on the combined tree before tagging would be a quick check.The merge plan (retarget to release/0.17 after #1427 lands, then a merge commit so each fix can be reverted alone) makes sense to me. It does mean this PR has to wait for #1427.
createdAt, so they're retired as soon as the first scale-down tick runs after the upgrade. The readiness stamp added in setRunning only helps sandboxes that start afterwards. For services that only get TCP/UDP, node-port, or direct (non-ingress) traffic, that means scaling to zero with nothing to wake them, as the new scaling.md warning says. Someone needs to decide whether that goes into the pre-ship note next to the containerd one, or into an Upgrade Notes line here pointing those services to mode = "fixed". I'm raising it for a human to decide. It isn't a code defect.Verdict: ready