fix(operator): honor kubectl rollout restart on ToolHive workloads - #6378
Conversation
The operator treated kubectl.kubernetes.io/restartedAt as drift and reverted kubectl rollout restart. Preserve that annotation on desired pod templates and on runner SSA so a bounce sticks. Fixes stacklok#6344
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-operator-specialist, go-idioms-and-test-coverage, architecture-and-design, error-handling-and-resilience, general-code-quality
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | Pod-template labels drift-tolerance loosened beyond the annotation this PR is about | 9/10 | HIGH | Fix |
| 2 | New Get in applyStatefulSet silently swallows non-NotFound errors |
9/10 | HIGH | Fix |
| 3 | Test coverage inconsistent: only one of three controllers (and no part of client.go) got a regression test for this fix |
9/10 | HIGH | Fix |
| 4 | New Get in applyStatefulSet duplicates a fetch shouldSkipStatefulSetApply already made |
8/10 | MEDIUM | Fix |
| 5 | kubectl.kubernetes.io/restartedAt defined twice with inconsistent visibility/naming |
7/10 | MEDIUM | Fix |
| 6 | PreserveKubectlRestartedAt mutates the caller's desired map in place instead of cloning |
7/10 | LOW-MEDIUM | Fix |
| 7 | TOCTOU: the Get-then-Force-Apply sequence can revert a concurrent, newer rollout-restart to the stale value it just read | 6/10 | MEDIUM | Discuss |
Overall
This fixes a real bug (#6344) the way the maintainer asked for: preserving kubectl.kubernetes.io/restartedAt instead of inventing a new bounce API. PreserveKubectlRestartedAt itself is small, nil-safe, and well tested, and it's wired into all three controllers' ensureDeployment in the right place (before the template gets overwritten).
The drift-detection change went further than that scoped fix, though. Pod-template labels (which kubectl rollout restart never touches) were also switched from exact-equality to a subset check in two of the three controllers, inconsistent with the third — that quietly removes the operator's ability to ever clean up a stale label it stops desiring. The runner-side StatefulSet path (pkg/container/kubernetes/client.go) reimplements the same idea with a duplicated API call, a swallowed non-NotFound error, and a small window where a concurrent kubectl rollout restart could get reverted by this very code path. None of that is a fundamental problem with the approach — each is a contained, cheap fix — but a few of them sit on the exact controllers/paths this bug report names, so worth addressing before merge.
Test coverage gap (not tied to one line)
Only virtualmcpserver_controller_test.go got a new "kubectl rollout restart annotation is not drift" case. mcpserver_controller.go and mcpremoteproxy_controller.go received the identical maps.Equal → MapIsSubset change with no equivalent test, and pkg/container/kubernetes/client.go's new Get-and-preserve logic in applyStatefulSet has no test at all. Given mcpserver_controller.go is the controller issue #6344 was filed against, that's the one most worth covering first.
Generated with Claude Code
Restore exact pod-template label equality, fail Get errors other than NotFound, reuse one StatefulSet read, clone the desired annotation map, and share the kubectl.kubernetes.io/restartedAt constant.
|
@RaviTharuma Thanks for the quick fixes, I think you have some lint errors though |
skipStatefulSetApply is already called from DeployWorkload. The Client.shouldSkipStatefulSetApply method was unused and failed golangci unused.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6378 +/- ##
==========================================
- Coverage 77.75% 77.70% -0.05%
==========================================
Files 750 751 +1
Lines 72601 72623 +22
==========================================
- Hits 56448 56433 -15
- Misses 16148 16185 +37
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Cross-links
Summary
kubectl rollout restarton ToolHive Deployments/StatefulSets was reverted onthe next reconcile because
kubectl.kubernetes.io/restartedAtwas treated asdrift. There is already a CR bounce annotation; this honors the standard
kubectl path instead of adding another API.
kubectl.kubernetes.io/restartedAtwhen computing the desiredpod template (MCPServer, MCPRemoteProxy, VirtualMCPServer)
MapIsSubset)Force: truewould otherwise wipe it)kubectl rollout restartnext to the existing CR annotationFixes #6344
Type of change
Test plan
go test -ldflags=-extldflags=-Wl,-w ./cmd/thv-operator/pkg/controllerutil/ ./cmd/thv-operator/controllers/ -run 'TestPreserveKubectlRestartedAt|TestMapIsSubset|TestVirtualMCPServerPodTemplateMetadataNeedsUpdate'go test -ldflags=-extldflags=-Wl,-w ./pkg/container/kubernetes/kubectl rollout restart deploy/<mcpserver>should keepkubectl.kubernetes.io/restartedAtand actually bounceAPI Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.No CRD schema change. The GitOps CR annotation
(
mcpserver.toolhive.stacklok.dev/restarted-at) is unchanged.Does this introduce a user-facing change?
Yes.
kubectl rollout restarton ToolHive proxy Deployments and MCPServerworkload StatefulSets is honored instead of being reverted.
Special notes for reviewers
This is the approach from #6344: preserve kubectl's annotation rather than
invent a new bounce API. The CR annotation remains the GitOps-friendly path.