Skip to content

Commit c6db2d9

Browse files
ChrisJBurnsclaude
andauthored
Add storage-version migration docs + upgrade-guide walkthrough (#5451)
* Add storage-version migration docs + upgrade-guide walkthrough End-to-end user documentation for the StorageVersionMigrator controller now that the chart surface (PR #5418) is in main. Reference doc — docs/operator/storage-version-migration.md: - Describes the actual shipped mechanism: plain Get+Update on the main resource, per-CR conflict retry (max 3), RequeueAfter sentinel on the conflict path. - Admission-policy compatibility section: webhooks fire on every Update before the bytes-equality elision check, so policies (Kyverno/Gatekeeper/OPA) that reject same-spec round-trip Updates will prevent the migrator from converging. - ⚠ Skip-a-version upgrade trap section: clusters that bypass an intermediate release that runs the migrator will hit a helm upgrade failure at the version-removal release; kube-storage-version-migrator documented as the recovery path. - Label contract reflects the no-escape-hatch rule from PR-B — every storage-version root type must carry the migrate marker. - RBAC list matches what's actually on main. Upgrade-guide walkthrough — docs/operator/upgrade-guide/: - Reproducible kind-cluster end-to-end test of the v1alpha1→v1beta1 graduation, verifying storedVersions converges to [v1beta1] on all 12 graduated CRDs after enabling the migrator. - CR fixtures for v1alpha1 and v1beta1 of all 12 graduated kinds. Supersedes #5011, which carried earlier-draft versions that described the pre-review SSA-on-/status mechanism, the removed exclude marker, and the old env-var name. Part of #4969. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Scope upgrade walkthrough to the 12 graduated CRDs explicitly Addresses six review findings on PR #5451: F1 (HIGH): The chart ships 13 labeled CRDs, not 12. The 13th (mcpwebhookconfigs) is single-version v1alpha1 and not part of the v1alpha1 → v1beta1 graduation this walkthrough demonstrates. Adds a scope note in the intro and introduces an explicit GRADUATED_CRDS array in Step 2 used by every verification loop in steps 8–11 so the loops don't drift. F2 (HIGH): Step 11's bulk patch loop previously iterated every toolhive CRD. For mcpwebhookconfigs the jq filter "map(select( .name != "v1alpha1"))" produces an empty array and the apiserver rejects the patch. Scoping the loop to GRADUATED_CRDS fixes this — only multi-version CRDs are touched. F3 (MED): Step 8's baseline "every line ends with [v1alpha1,v1beta1]" was false for the 13th CRD which reads [v1alpha1]. Loop now iterates GRADUATED_CRDS only; the scope note explains why the 13th is excluded. F4 (MED): Step 9's convergence loop hardcoded "12" and silently exited if convergence never landed. Loop count is now derived from ${#GRADUATED_CRDS[@]} so a future PR adding a graduated CRD won't drift, and a post-loop conditional surfaces an actionable timeout message naming exactly what didn't converge. F5 (LOW): The companion-reading bullet described the reference doc as covering "label contract, opt-out, mechanism." The doc explicitly removed the exclude marker in PR-B; it's an opt-in model. Reworded. F6 (LOW): Steps 5 and 9 selected a pod via "kubectl get pods | grep | head -1", which can read the still-Terminating old pod after a helm upgrade and show the pre-upgrade env value. Replaced with a Deployment-spec jsonpath read that avoids the race entirely. Step 10 still uses pod selection (logs aren't readable off a Deployment) but with --field-selector=status.phase=Running. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Enable migrator flag explicitly in upgrade step 9 Step 9 set only the three image values on the helm upgrade, so the StorageVersionMigrator never turned on: the chart defaults operator.features.storageVersionMigrator to false, and helm upgrade does not reuse the previous release's --set values, so the env var rendered as false and storedVersions stayed stuck at [v1alpha1, v1beta1]. Following the guide verbatim never converged the CRDs. Add --set operator.features.storageVersionMigrator=true to step 9 and note why it is required. Verified end-to-end against a local kind cluster: all 12 graduated CRDs converge to [v1beta1] with the flag set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ad127f7 commit c6db2d9

4 files changed

Lines changed: 801 additions & 0 deletions

File tree

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
# Storage Version Migration
2+
3+
The ToolHive operator ships a `StorageVersionMigrator` controller that keeps every ToolHive CRD's `status.storedVersions` list clean, so a future operator release can drop deprecated API versions (e.g. `v1alpha1`) without orphaning objects in etcd.
4+
5+
The controller is **opt-in** via the `operator.features.storageVersionMigrator` chart value (or the `TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR` environment variable if you inject it directly via `operator.env`).
6+
7+
## Why this exists
8+
9+
When a CRD graduates from, say, `v1alpha1` to `v1beta1` with both versions served and `v1beta1` as the storage version, existing objects continue to work — they are transparently converted on read/write. But the API server records every version that has ever been used for storage in `CustomResourceDefinition.status.storedVersions`. Until that list is trimmed, the API server refuses to let you remove a version from `spec.versions`, because doing so would orphan any etcd-stored objects encoded at that version.
10+
11+
The cleanup is not automatic. Someone has to re-store every existing object at the current storage version, then explicitly patch `status.storedVersions` to drop the old entry. The `StorageVersionMigrator` controller does this for you, on every opted-in ToolHive CRD, continuously. See [upstream Kubernetes documentation](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/storage-version-migration/) for the underlying mechanism.
12+
13+
## How it works
14+
15+
For each opted-in CRD the controller does:
16+
17+
1. Live-reads the CRD via `APIReader` (bypassing the informer cache, so it sees the current `storedVersions`).
18+
2. Reads `spec.versions` to find the entry with `storage: true`.
19+
3. If `status.storedVersions` already equals `[<currentStorageVersion>]`, exits — nothing to do.
20+
4. Otherwise, paginates through every Custom Resource of the kind and issues a plain `Get` + `Update` against the main resource. The API server re-encodes the request body at the current storage version and compares the resulting bytes to what's in etcd. If they differ, etcd is rewritten at the new storage version. If they match (already migrated), the API server elides the write.
21+
5. Once every CR has been processed without errors, patches `CRD.status.storedVersions` to `[<currentStorageVersion>]` using an optimistic-lock merge — so concurrent API-server writes cause a clean retry rather than a silent overwrite.
22+
23+
### Concurrent-write safety
24+
25+
The migrator handles conflicts conservatively. If a per-CR `Update` returns `IsConflict` (another writer raced), the controller retries the per-CR Get + Update up to three attempts. If every retry conflicts, the migration pass returns a sentinel error and the controller requeues itself after 30 seconds without bumping the exponential backoff. `status.storedVersions` is only trimmed when every CR in a pass was successfully re-stored.
26+
27+
This is the upstream `kube-storage-version-migrator` semantics — a Conflict means another writer succeeded, which itself re-encoded the CR at the storage version, so the migration is effectively done for that CR even if our own Update didn't land.
28+
29+
### Admission webhook interaction
30+
31+
Each per-CR `Update` goes through the API server's admission chain (mutating then validating webhooks) before reaching the storage-encoder elision check. **Only the etcd write and watch fanout are elided** when the encoded bytes match what's already stored — admission webhooks fire on every Update regardless.
32+
33+
For ToolHive's own webhooks this is fine; they only reject changes that break spec invariants, and a same-spec round-trip Update cannot trigger those rejections. **If you run a cluster-wide admission policy engine** like Kyverno, Gatekeeper, or OPA, check that your policies don't reject same-spec round-trip Updates of ToolHive CRs before enabling the migrator. A policy that requires a `lastUpdatedBy` annotation, for example, would reject every migrator Update and the controller would never converge.
34+
35+
## The opt-in label
36+
37+
A CRD participates in migration only if it carries:
38+
39+
```yaml
40+
metadata:
41+
labels:
42+
toolhive.stacklok.dev/auto-migrate-storage-version: "true"
43+
```
44+
45+
The label is set at CRD-generation time via a kubebuilder marker on each Go root type:
46+
47+
```go
48+
//+kubebuilder:object:root=true
49+
//+kubebuilder:storageversion
50+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
51+
type MCPServer struct { ... }
52+
```
53+
54+
`task operator-manifests` bakes the label into the generated CRD YAML. Every ToolHive CRD that carries `+kubebuilder:storageversion` ships with the marker today.
55+
56+
A CI test (`TestStorageVersionRootMarkerCoverage` in `cmd/thv-operator/controllers/marker_coverage_test.go`) fails the build if a root type is added without the migrate marker. There is no self-serve "exclude" marker — every storage-version root type must opt in. If a future CRD legitimately should not be auto-migrated, the path is to add an entry to the `excludedRootTypes` allowlist in the test file via PR review, not via a self-serve marker.
57+
58+
### Adding a new CRD
59+
60+
Add the marker to the type alongside the existing kubebuilder markers:
61+
62+
```go
63+
//+kubebuilder:object:root=true
64+
//+kubebuilder:storageversion
65+
//+kubebuilder:subresource:status
66+
//+kubebuilder:metadata:labels=toolhive.stacklok.dev/auto-migrate-storage-version=true
67+
type NewShinyThing struct { ... }
68+
```
69+
70+
Then `task operator-manifests` to regenerate the CRD YAML. CI verifies the marker is present.
71+
72+
## Enabling the controller
73+
74+
Set the Helm feature flag at install or upgrade time:
75+
76+
```yaml
77+
operator:
78+
features:
79+
storageVersionMigrator: true
80+
```
81+
82+
This sets `TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR=true` on the operator Deployment and registers the reconciler with the manager.
83+
84+
Once enabled, the controller is dormant on CRDs whose `storedVersions` already equals `[<currentStorageVersion>]` — most of the time, most CRDs. It only does meaningful work when a CRD's stored-versions list is dirty (typically right after a graduation release).
85+
86+
## Per-CRD emergency escape hatch
87+
88+
Removing the label on a live cluster excludes that single CRD from migration immediately:
89+
90+
```bash
91+
kubectl label crd/mcpservers.toolhive.stacklok.dev \
92+
toolhive.stacklok.dev/auto-migrate-storage-version-
93+
```
94+
95+
Intended for incident response only. If you deploy the operator via GitOps (Argo CD, Flux) or `helm upgrade`, the chart will re-apply the chart-set label within seconds. For a long-term per-cluster opt-out, leave `storageVersionMigrator: false` and accept that you will need to handle storage-version cleanup yourself before any version-removal release.
96+
97+
## Interaction with version-removal releases
98+
99+
The `StorageVersionMigrator` must have run against your cluster *before* an operator release that drops a deprecated CRD version ships. The typical sequence is:
100+
101+
1. **Release N**: both versions served, newer version is storage, `StorageVersionMigrator` available (opt-in via the chart flag). Operators that enable the migrator have their `storedVersions` trimmed during this deprecation window.
102+
2. **Release N+1+**: the deprecated version is removed from `spec.versions`. Because every cluster that enabled the migrator already has clean `storedVersions`, the CRD update applies.
103+
104+
> **⚠ Skip-a-version upgrade trap.** If your cluster upgrades directly from a pre-migrator release to the version-removal release without ever running an intermediate release that runs the migrator, the helm upgrade will **fail** when the API server refuses to remove the deprecated version from `spec.versions`. To recover: deploy [kube-storage-version-migrator](https://github.com/kubernetes-sigs/kube-storage-version-migrator) once to clean `storedVersions`, then retry the upgrade. To avoid the trap entirely, install each release in sequence, and enable `storageVersionMigrator: true` at least one release before any version-removal release.
105+
106+
## Verification
107+
108+
For any ToolHive CRD in a cluster where the controller has run successfully:
109+
110+
```bash
111+
kubectl get crd mcpservers.toolhive.stacklok.dev \
112+
-o jsonpath='{.status.storedVersions}'
113+
# ["v1beta1"]
114+
```
115+
116+
If the list contains more than one entry, the controller has not yet finished migrating — check operator logs for reconcile errors and the `StorageVersionMigrationFailed` event on the CRD. The controller will also INFO-log `storage version migration not converging — sustained concurrent writes` after five consecutive conflict-only passes against the same CRD, which is the signal to investigate sibling reconcilers or admission policies that may be racing with the migrator.
117+
118+
## RBAC
119+
120+
The operator ServiceAccount carries (generated from kubebuilder markers, applied by the operator Helm chart):
121+
122+
- `customresourcedefinitions.apiextensions.k8s.io`: `get`, `list`, `watch`
123+
- `customresourcedefinitions/status.apiextensions.k8s.io`: `update`, `patch`
124+
- `*.toolhive.stacklok.dev`: `get`, `list`, `update`
125+
126+
The wildcard on `toolhive.stacklok.dev` resources is intentional: the set of opted-in CRDs is a runtime label decision, not a codegen-time enumeration. The runtime gate (`isManagedCRD` requiring the opt-in label) ensures the controller only writes to CRDs that explicitly opted in, even though the RBAC bound is wider.
127+
128+
## Related
129+
130+
- Issue: [stacklok/toolhive#4969](https://github.com/stacklok/toolhive/issues/4969)
131+
- PR-A — controller: [stacklok/toolhive#5362](https://github.com/stacklok/toolhive/pull/5362)
132+
- PR-B — opt-in labels + marker-coverage CI: [stacklok/toolhive#5391](https://github.com/stacklok/toolhive/pull/5391)
133+
- Kubernetes CRD versioning: [official docs](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/)
134+
- Upstream reference: [`kube-storage-version-migrator`](https://github.com/kubernetes-sigs/kube-storage-version-migrator)

0 commit comments

Comments
 (0)