linode: make the tagged-StorageClass swap race-safe and non-fatal - #1312
linode: make the tagged-StorageClass swap race-safe and non-fatal#1312travagliad wants to merge 1 commit into
Conversation
Follow-up to #1311 (review). The delete-then-apply of linode-block-storage-retain was fragile: under set -euo pipefail, if LKE's addon reconciler recreates the untagged StorageClass in the window between the two commands, the apply fails with "updates to parameters are forbidden" (parameters are immutable) and the script exits with the cluster already created and billing -- a worse outcome than the leak this closes. Recreate it with `kubectl replace --force` (one step, tolerates immutable parameters), retried once and non-fatal: losing the race now degrades to an untagged volume covered by the imperative tagging backstop, never a failed provision that orphans the cluster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DBhvZuEUQU8ym347Xat4Yy
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # `replace --force` recreates it in one step (a plain apply would hit "parameters | ||
| # forbidden"); non-fatal with one retry so losing a race with LKE's addon reconciler | ||
| # degrades to an untagged volume (imperative tagging backstop) -- never a failed | ||
| # provision that leaks the already-created cluster. |
There was a problem hiding this comment.
🟡 These four lines explain the mechanism and why apply was rejected — that is the PR body's job, and it already says all of this verbatim. Lines 135-136 above already carry the invariant that motivates the whole block (parameters immutable → volumes born untagged), so the only thing left worth keeping in the file is why losing the swap is survivable, not how replace --force differs from apply or what the failure would have cost.
| # `replace --force` recreates it in one step (a plain apply would hit "parameters | |
| # forbidden"); non-fatal with one retry so losing a race with LKE's addon reconciler | |
| # degrades to an untagged volume (imperative tagging backstop) -- never a failed | |
| # provision that leaks the already-created cluster. | |
| # An untagged SC here is survivable: tag-lke-resources.sh tags the volumes | |
| # imperatively at provision and before delete. |
| allowVolumeExpansion: true | ||
| EOF | ||
| } | ||
| _apply_tagged_sc || { sleep 3; _apply_tagged_sc || log "WARN: tagged StorageClass not applied; volumes fall back to imperative tagging"; } |
There was a problem hiding this comment.
🟡 The graceful-degradation claim only holds for the one failure mode the PR body reasons about. replace --force is delete-then-create as two API calls, so if the create half fails for any reason other than the reconciler having recreated the SC (transient API error, webhook, apiserver hiccup), the delete has already succeeded and the cluster is left with no linode-block-storage-retain and no default StorageClass. Both attempts then fail the same way, and the WARN says "volumes fall back to imperative tagging" when in fact no volumes get provisioned at all: the deps/PMM PVCs stay Pending and the run burns to kubectl rollout status --timeout=20m (line 233) before it fails — a billing cluster held ~20 minutes longer than the fast set -e exit this replaces.
Make the degradation the code's guarantee rather than an assumption about which half failed — assert a default SC exists before continuing, and fail fast if it does not:
| _apply_tagged_sc || { sleep 3; _apply_tagged_sc || log "WARN: tagged StorageClass not applied; volumes fall back to imperative tagging"; } | |
| _apply_tagged_sc || { sleep 3; _apply_tagged_sc || log "WARN: tagged StorageClass not applied; volumes fall back to imperative tagging"; } | |
| kubectl get storageclass linode-block-storage-retain >/dev/null |
|
|
2 participants
Problem
Follow-up to #1311, addressing a review finding on the just-merged change.
create-lkeswapped the tagged StorageClass in withkubectl delete … ; kubectl apply …. That is fragile underset -euo pipefail: LKE's addon reconciler can recreate the (untagged) defaultlinode-block-storage-retainin the window between the two commands. Theapplythen tries to update an existing StorageClass, hitsupdates to parameters are forbidden(a StorageClass'sparametersare immutable — the very premise of #1311), and the script exits non-zero with the cluster already created and billing. The EXIT trap only tags/diagnoses, so nothing tears the cluster down until the 24h TTL reaper — a worse outcome than the leak #1311 closes.Change
Recreate the StorageClass with
kubectl replace --force(a single delete+create, so immutableparametersare replaced rather than rejected), wrapped so it is non-fatal: one retry, then a warning. Losing the race with the reconciler now degrades to an untagged volume — still covered by the imperative tagging backstop (tag-lke-resources.shat provision and before delete) — instead of a failed provision that orphans the cluster.Validation
shellcheck -S warningandbash -nclean.replace --force's create step meets LKE's freshly-recreated SC (AlreadyExists) and the retry recreates it; if both attempts lose, a default StorageClass still exists (LKE's untagged one), so the provision proceeds and volumes fall back to imperative tagging — never a dead, billing cluster.🤖 Generated with Claude Code
https://claude.ai/code/session_01DBhvZuEUQU8ym347Xat4Yy
Generated by Claude Code