Fix Elasticsearch finalization after ILM cleanup - #40
Conversation
Treat missing data-stream backing indices as lifecycle cleanup only when they form the oldest contiguous prefix of the snapshot generations. Keep ordinary indices and the newest backing generation mandatory so restore startup and incomplete restores cannot be mistaken for success.
There was a problem hiding this comment.
Approach looks right: separating ILM-removable generations from mandatory indices, requiring the missing set to be an oldest contiguous prefix, and keeping every present index's primaries required. Four points inline — 1 and 2 are the ones I'd want resolved.
Checked by building the branch and probing complete() directly with the real backing-index naming; existing tests pass.
Nits: %d oldest data-stream backing indices reads wrong at 1, and make([]string, 0) differs from the var style in restore.go.
Use ordinary STS indices as the positive restore anchor, allow all lifecycle-managed generations to age out, reject lifecycle-only snapshots, and keep stall progress monotonic. Add realistic backing-index coverage and bump x/crypto to v0.56.0 for the current HIGH CVE fixes.
|
Latest commit
All CI checks now pass, including tests, lint, govulncheck, Trivy, Grype, and VEX. |
There was a problem hiding this comment.
Re-reviewed at a3224b9. All four earlier findings are addressed and the anchor reasoning now holds up: deleteAllSTSIndices deletes and verifies removal of every STS index before the restore, so a present required index really does prove the restore's cluster-state update applied — which is what makes the new lifecyclePresent == 0 path sound. Realistic backing-index names in the fixtures also lock in the ordering assumption.
Three things left, one of them a regression from the fix to my earlier stall-timer point.
The > comparison turns an ILM deletion into a permanent ceiling: I reproduced a stall failure while primaries were still climbing. That is the highest-value item. The lifecycle-only rejection and the retention wording are judgement calls worth a second look before merge.
|
Implemented the safer ILM handling in
Local tests, vet, build, lint, GoReleaser validation, module verification, and vulnerability checks pass. The review threads were answered and resolved. |
There was a problem hiding this comment.
Re-reviewed at d816ff3. The stall fix is confirmed working: my original repro — sts_topology at 3/10 primaries alongside a restored 8-shard backing index, ILM deleting it while topology climbs 3 -> 5 -> 7 -> 10, with a 1ns no-progress timeout — now runs through to SUCCESS instead of failing. Two consecutive deletions also survive. Rebaselining and skipping the stall check on the incomparable poll is the right shape, and the two new tests pin both halves of it.
Moving the lifecycle-only rejection ahead of confirmation, scale-down and index deletion resolves my concern properly. The objection I was raising was the blast radius — a snapshot rejected after the destructive steps left workloads down with only --finalize-only as an exit. Failing before anything is touched removes that entirely.
Your point about the write index is correct and I was wrong to suggest it: deleteAllSTSIndices computes its deletion list before calling RolloverDatastream, so the rollover-created write index is not in that list and survives into the restore. It would have been a false anchor. Worth keeping that reasoning in the commit message — it is the kind of thing that gets re-proposed later.
I also accept the retention-wording reply. The required-index anchor does establish that the metadata was applied, and naming every expired generation would be noise against the count you already log.
One new thing, from narrowing the predicate — low likelihood but it turns a working restore into a hard failure. Full suite passes locally; CI on d816ff3 was still running when I looked.
|
Addressed the remaining index-selection review in All review threads are resolved and CI passes. |
There was a problem hiding this comment.
Re-reviewed at 34db3b3. This is a better fix than the one I suggested: deriving the restore list from the snapshot with the same predicate means deletion, restore and completion monitoring cannot disagree by construction, and it fixes already-deployed ConfigMaps without a migration — which narrowing the glob would not have done. Verified the sibling stream is excluded and the joined string is what RestoreSnapshot receives; build and the cmd/elasticsearch + config suites pass locally, CI green on the commit.
That closes every finding from all three earlier rounds. One loose end left behind by this change, config hygiene rather than a defect.
|
Re-reviewed at Verified locally: build and the full test suite pass, and the sibling data-stream exclusion still holds through the new pattern intersection. CI green on the commit. |
Handle ILM retention without weakening restore-start detection: ordinary STS indices anchor completion, while missing data-stream generations may be an oldest prefix or the entire expired set. Lifecycle-only snapshots are rejected as unsafe to monitor.
Also updates
golang.org/x/cryptoto v0.56.0 for the current HIGH CVE fixes.Validation: full tests, vet, build, lint, module verification, source and five-platform binary govulncheck, and GoReleaser artifact verification.
Fixes #39