🐛 (ZENKO-5303) make voting and priority right - #2452
Conversation
Hello darkisdude,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
DarkIsDude
changed the title
…om commit 8c1857d96d5 git-subtree-dir: solution-base/images/mongodb-sharded/debian-12 git-subtree-split: 8c1857d96d58dcb94f03edf2f4f1fb31dfd86e5c
…rom commit a74af2ad208 git-subtree-dir: solution-base/images/mongodb-exporter/debian-12 git-subtree-split: a74af2ad208da6c2bda4e190102c51c8160c4a20
…it 9f43f0e24f5 git-subtree-dir: solution-base/images/os-shell/debian-12 git-subtree-split: 9f43f0e24f508d92e270c1722c89f759c6ac5f16
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
The change itself looks ok; however I am not sure we should do this.
I think the configuration was done only on first startup by design : this ensures that after first startup, each instance will just use the configuration it has and let (human) operator manage it.
This means there is a gap in the chart indeed: if the instance is restarted during this startup, it may be left in an incorrect state... But this is a tradeoff: either we enforce the state of replicas (i.e. your change) -even though we don't actually know the intent of the human deploying the chart- and risk setting the wrong state for more advanced setups ; or we keep the existing/upstream approach to automatically handle the nominal path only, and leave recovery for humans...
→ if we were writing an operator, I would say it should recover automatically -and we just need to add whatever necessary in the CR to make intent clear
→ however this is a chart -mostly static, with not much way to get instant- so I would rather stay conservative
→ practically we never experienced this issue until we removed part of the chart (what you already fixed) : so the risk seems very low to keep it as upstream, and we can add an extra check in installer to validate the overall state during/after chart deployment for extra safety?
| # granting voting rights, leaving the node stuck without them. | ||
| if mongodb_secondary_node_has_voting_rights "$node" "$port"; then | ||
| info "Node already has voting rights" | ||
| else |
There was a problem hiding this comment.
this will not behave properly when we have 9-replicas, where 2 nodes are always "non voting". This is expected, and we must not try to change that (in particular, the voting nodes must be in precisely the expected DC for proper redundancy)
→ the change will make the startup slower on these extra secondaries -(re-)trying to make them voting-, possibly with the extra risk of changing the set of voters...
There was a problem hiding this comment.
@DarkIsDude did you try that case?
Looking at the code below, it seems that the script would exit if it fails to configure secondary node voting: does that function already silently handle the case where we reached the max number of voters already?
There was a problem hiding this comment.
Yes — tested on a real 9-replica shard, both with the current development/2.15 image and with this branch. Short answer to your question: no, it is not silently handled — it exits. But that is pre-existing behaviour, not something this PR introduces.
First: your review caught a real bug, just not the one we were discussing. mongodb_secondary_node_has_voting_rights() grepped mongosh's output for true, and mongosh always prints a connection banner containing directConnection=true. So the check always returned true, the grant branch became dead code, and no secondary ever got voting rights. Observed directly: a fresh secondary joined with a free voter slot available and stayed at votes=0 forever (9 members / 6 voters). By extension a fresh install would leave only pod-0 — which self-initiates — with a vote. Fixed using an explicit HAS_VOTES_YES/HAS_VOTES_NO sentinel. Re-tested after the fix: same scenario now yields votes=1 priority=1 → 7 voters.
Secondly: "the extra risk of changing the set of voters": The only path where this branch differs from development/2.15 is data dir empty and node already in rs.conf() — i.e. PVC loss on an existing member.
Last one: "does that function silently handle reaching the max number of voters?" No. Reproduced, scaling 3 → 9:
mongod: Replica set configuration contains 8 voting members, but must be at least 1 and no more than 7
pod-7: ERROR ==> Secondary node did not get marked as secondary (after 24 × 5s of retries)
→ exit 1 → container restart
It does recover: on restart the data dir is no longer empty, config is skipped, and the member settles as a working non-voting secondary. Net cost is one crash + ~3 min per extra member, and the final topology is the expected 9 members / 7 voters.
This branch behaves identically (9 members / 7 voters, one crash each on pods 7 and 8), with a more accurate message: Secondary node did not get granted voting rights rather than the old, misleading "did not get marked as secondary".
Happy to add a members.filter(m => m.votes > 0).length < 7 guard to skip the doomed promotion entirely if you'd like that in this PR. Put the behaviour is unchanged.
There was a problem hiding this comment.
Happy to add a members.filter(m => m.votes > 0).length < 7 guard to skip the doomed promotion entirely if you'd like that in this PR. Put the behaviour is unchanged.
Ack for the unchanged behavior, but I guess it's best to fix it, right?
(should probably be upstreamed as well)
delthas
left a comment
delthas
left a comment
There was a problem hiding this comment.
- Makefile clone optimization: LGTM
- Granting voting rights to existing replicas: will break on > 7 replicas (as mentioned by François). Not sure how to proceed
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
maeldonn
left a comment
maeldonn
left a comment
There was a problem hiding this comment.
+1 for @francoisferrand
|
@francoisferrand @maeldonn @delthas the upstream has been merged, what should we do ? |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
There was a problem hiding this comment.
Granting voting rights to existing replicas will break on > 7 replicas : need to add extra condition to either
- skip
mongodb_configure_secondary_node_votingif there are already too many voters - or detect (and ignore) the "too many voters" error
- or ignore the
mongodb_configure_secondary_node_votingerror if we see (after the call/error) that there are 7 voters already
(in particular, must check the behavior with the 9-replicas setup we have: to ensure the change would not change the voters: which are manually set to be precisely in the expected datacenter/room, and thus meet the availability constraints specified by customer)
…base/images/mongodb-sharded/debian-12'
…base/images/mongodb-exporter/debian-12'
…base/images/os-shell/debian-12'
514ca64 to
709d40e
Compare
mongosh prints a connection banner containing 'directConnection=true', so grepping the output for "true" always matched and the check always reported that the node already had voting rights. The grant was therefore never executed: secondaries stayed at votes=0/priority=0 forever. Emit an explicit HAS_VOTES_YES/HAS_VOTES_NO sentinel and match on it, and use .some() rather than .filter().length. Issue: ZENKO-5303
mongodb_is_secondary_node_ready had the same flaw as the voting check: mongosh prints a connection banner containing 'directConnection=true', so grepping the output for "true" always matched. The guard therefore returned true on its first call and never actually waited for the node to reach SECONDARY before voting rights were granted. Emit an explicit IS_SECONDARY_YES/IS_SECONDARY_NO sentinel and match on it, and use .some() rather than .filter().length. Note this restores a wait that never took effect: a node still performing its initial sync now blocks here for up to MONGODB_INIT_RETRY_ATTEMPTS * MONGODB_INIT_RETRY_DELAY instead of proceeding immediately. Issue: ZENKO-5303
Tested on a real 9-replica shard, on both 2.15 and this branch. Good catch, but the check was worse than suspected: it grepped mongosh output for true, which always matches the directConnection=true banner — so no secondary ever got votes at all. Fixed in f313b49, plus 5f71df4 for the same bug in mongodb_is_secondary_node_ready. The >7 crash is real but pre-existing (reproduced on 2.15): ~2 min of retries, exit 1, then the restart finds a non-empty data dir, skips config, and the member settles as non-voting — final topology is still 9 members / 7 voters. Voters can't drift in the nominal case: configure_replica_set only runs when the data dir is empty (libmongodb-sharded.sh:74), so restarts and rolling updates never touch rs.conf(); and at 7 voters the ceiling rejects any promotion. The only real risk is "empty disk + ≤6 voters" (6+1=7 is accepted), i.e. node replacement, since storage is node-local. We can add your idea — a pre-check on the voter count rather than parsing the error string if you prefer but this will differ from the upstream ? |
mongosh prints a connection banner containing 'directConnection=true' on stdout, so a helper that decides a boolean with grep -q "true" always reports true. That is how mongodb_secondary_node_has_voting_rights shipped, which made the voting-rights grant unreachable. Add tests over the vendored libmongodb.sh: - replay the real mongosh output (banner + result) against mongodb_secondary_node_has_voting_rights and mongodb_is_secondary_node_ready, asserting both answers are honoured; - forbid the idiom repo-wide, so any future helper matching a bare true/false on mongosh output fails the build. The library sources its dependencies from absolute /opt/bitnami paths and cannot be sourced outside the image, so the functions under test are lifted out and run with mongodb_execute_print_output stubbed. Verified the tests fail on the pre-fix implementation and pass on the current one. Issue: ZENKO-5303
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
The history of this PR seems weird : there are commits to remove "vendored image dirs ahead of subtree bootstrap"
The whole point of using git subtree is that we don't revenger from scratch (= loose local changes), but can instead repeatedly merge with the usual git semantics
→ please remove the history, and update upstream with merge only (bootstrap has been done already, no need to redo it
| trap 'git worktree remove --force "$$wt" 2>/dev/null' EXIT && \ | ||
| git worktree add --no-checkout --detach "$$wt" "$$sha" && \ | ||
| git -C "$$wt" sparse-checkout set --no-cone bitnami/$*/$(call bitnami_path,$*) && \ | ||
| git -C "$$wt" checkout && \ |
There was a problem hiding this comment.
why do we need a checkout?
| git fetch --depth 1 --filter=blob:none $(BITNAMI_REMOTE) $(call bitnami_ref,$*) | ||
| -git branch -D $(call vendor_branch,$*) | ||
| git subtree split --prefix=bitnami/$*/$(call bitnami_path,$*) $(call bitnami_ref,$*) -b $(call vendor_branch,$*) | ||
| sha=$$(git rev-parse FETCH_HEAD) && \ |
There was a problem hiding this comment.
no need for Git, this is actually $(call bitnami_ref,$*).
So best to do it the other way:
sha=$(call bitnami_ref,$*) &&
git fetch --depth 1 --filter=blob:none $(BITNAMI_REMOTE) $${sha} &&
| # granting voting rights, leaving the node stuck without them. | ||
| if mongodb_secondary_node_has_voting_rights "$node" "$port"; then | ||
| info "Node already has voting rights" | ||
| else |
There was a problem hiding this comment.
Happy to add a members.filter(m => m.votes > 0).length < 7 guard to skip the doomed promotion entirely if you'd like that in this PR. Put the behaviour is unchanged.
Ack for the unchanged behavior, but I guess it's best to fix it, right?
(should probably be upstreamed as well)
6 participants
#2449
UPSTREAM : bitnami/containers#95156
What does this PR do, and why do we need it?
It fixes a bug in the MongoDB sharded image bootstrap (
libmongodb.sh) that can leave every replica set with only one voting member, turning that single node into a hard single point of failure: if it goes down, the shard (and therefore the datastore) goes read‑only and Zenko is down.A 30‑second MongoDB primer (for non‑Mongo readers)
Our data lives in replica sets: groups of MongoDB nodes (here, 3 per shard and 3 for the config server) that each hold a copy of the data.
votes(0 or 1): can this member vote in an election? Majority is counted over the sum of votes.priority(0+): can this member be elected PRIMARY?priority: 0means "never become PRIMARY".votes: 1, priority: 1. It survives losing any one node: the remaining 2 votes are still a majority, and a surviving member can be elected PRIMARY.A member that is only partially configured —
votes: 1, priority: 0, or worsevotes: 0, priority: 0— still holds data but cannot help keep the set alive.The bug
When a SECONDARY first joins, the bootstrap script does this (
mongodb_configure_secondary):rs.add(... votes: 0, priority: 0)— add the node without voting power so it doesn't disturb the existing majority while it copies data (this is MongoDB's recommended safe procedure).mongodb_node_currently_in_cluster).SECONDARYstate).votes: 1, priority: 1.The confirmation in step 2 reads
rs.status()and greps the output for the node:The problem: with the broken image,
mongodb_executeis a thin wrapper arounddebug_execute, which throws away stdout unlessBITNAMI_DEBUG=true(it isn't, by default):So
resultis always empty, the grep always fails, andmongodb_node_currently_in_clusteralways returns false. That makes step 2 (mongodb_wait_confirmation) time out and the bootstrap aborts with:The container exits, Kubernetes restarts the pod, and on the second boot the data directory already exists, so the bootstrap takes the "deploy with persisted data" path and skips replica set configuration entirely. The node is left frozen at
votes: 0, priority: 0. Steps 3–4 never run, so the SECONDARY is never promoted.Net result: only the bootstrap PRIMARY (
*-0) ends up with a vote → 1 voter per replica set → single point of failure.Root cause: an incomplete fork of the Bitnami image
Bitnami stopped publishing the
mongodb-shardedimage, so we vendored its scripts into the repo (ZENKO-5110, #2366). The vendoring happened in two commits, and they are not equal:libmongodb.shmongodb_execute()defs0ae8c8a32f6c7e42Compare: the only relevant difference is the last 43 lines of
libmongodb.sh.Upstream's
libmongodb.shis assembled by concatenating script fragments, and it deliberately definesmongodb_executetwice:debug_execute mongodb_execute_print_output "$@").# Copyright …header and# shellcheck disable=SC2148— the tell‑tale "no shebang" marker of a separate concatenated file) — the real version that callsmongoshdirectly and returns output.In bash the last definition wins, so upstream's effective
mongodb_executeis #2 (returns output) — which is exactly whatmongodb_node_currently_in_clusterneeds.The 2.15 re‑vendoring (
2f6c7e42) truncated the file at 1669 lines and dropped that final fragment. Only the output‑discarding wrapper was left, silently revertingmongodb_executeand breaking the confirmation check. The 2.14 vendoring had copied the whole file, so 2.14.5 worked. We didn't add a fix in 2.14 — we just vendored completely there, and lost it in 2.15.It was easy to miss because dropping a duplicate function definition leaves valid bash that runs fine; the only signal was the line count (1712 vs 1669), and the failure only surfaces as a silent bootstrap race that is invisible until a node dies.
How we confirmed it (two clusters, same Mongo version)
A cluster on image base
2.14.5was healthy (3 voters); a cluster on2.15.1was broken (1 voter). We confirmed the chain end‑to‑end from the livers.conf()(broken cluster: secondaries atvotes: 0, priority: 0; healthy cluster:votes: 1) and the pod boot logs (broken cluster fails at "Unable to confirm…"; healthy one gets past it). The diff between the two images'libmongodb.shwas exactly the 43‑line fragment above.The fix
Two commits:
🐛 add missing libmongo.sh fork— restores the dropped 43‑line fragment, so the file matches upstream again.mongodb_executeis once more the output‑returning definition,mongodb_node_currently_in_clustercan readrs.status(), and the bootstrap no longer aborts before granting voting rights. This is the root‑cause fix (faithful re‑sync with upstream). (in the first PR)🐛 ensure secondary keeps votes and priority after restart— makes the voting‑rights grant idempotent and re‑runnable, so a node can no longer be left stranded without votes:mongodb_secondary_node_has_voting_rightschecks whether the member already hasvotes > 0 && priority > 0;mongodb_configure_secondarynow grants voting rights whenever they are missing — even if the node is already in the cluster, instead of only on the freshly‑added path. So if a previous attempt added the node atvotes/priority 0and then failed (or was restarted) before promotion, the next run finishes the job and converges it tovotes: 1, priority: 1;"did not get marked as secondary"printed for the voting step) is corrected to"did not get granted voting rights".After this change a fresh deployment reliably ends with all members at
votes: 1, priority: 1(true HA), and a member that is already in the replica set but under‑privileged is repaired rather than silently left non‑voting.Which issue does this PR fix?
Fixes ZENKO-5302.
Special notes for your reviewers:
votes: 0still needs a one‑time manualrs.reconfig()on each replica set (shard-Nandconfigsvr) to set members 1 and 2 tovotes: 1, priority: 1. The script change guarantees correct behaviour for new bootstraps and for any path wheremongodb_configure_secondaryruns against a member that lacks voting rights.