Skip to content

🐛 (ZENKO-5303) make voting and priority right - #2452

Open
DarkIsDude wants to merge 20 commits into
development/2.16from
bugfix/ZENKO-5303/make-voting-and-priority
Open

🐛 (ZENKO-5303) make voting and priority right#2452
DarkIsDude wants to merge 20 commits into
development/2.16from
bugfix/ZENKO-5303/make-voting-and-priority

Conversation

@DarkIsDude

@DarkIsDude DarkIsDude commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

#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.

  • One node is the PRIMARY (takes writes); the others are SECONDARIES (copies).
  • To stay alive, a replica set must keep a majority of votes online and be able to elect a PRIMARY. Two settings per member matter:
    • 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: 0 means "never become PRIMARY".
  • A healthy 3‑node set has 3 members with 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 worse votes: 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):

  1. 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).
  2. Confirm the node is now listed in the replica set (mongodb_node_currently_in_cluster).
  3. Wait for it to finish its initial sync (reach SECONDARY state).
  4. Grant voting rights: reconfigure it to votes: 1, priority: 1.

The confirmation in step 2 reads rs.status() and greps the output for the node:

result=$(mongodb_execute ... <<< "rs.status().members")
grep -q "'$node:$port'" <<<"$result"

The problem: with the broken image, mongodb_execute is a thin wrapper around debug_execute, which throws away stdout unless BITNAMI_DEBUG=true (it isn't, by default):

debug_execute() { if is_boolean_yes "${BITNAMI_DEBUG:-false}"; then "$@"; else "$@" >/dev/null 2>&1; fi; }

So result is always empty, the grep always fails, and mongodb_node_currently_in_cluster always returns false. That makes step 2 (mongodb_wait_confirmation) time out and the bootstrap aborts with:

ERROR ==> Unable to confirm that <node> has been added to the replica set!

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-sharded image, so we vendored its scripts into the repo (ZENKO-5110, #2366). The vendoring happened in two commits, and they are not equal:

Commit Branch libmongodb.sh mongodb_execute() defs Result
0ae8c8a3 development/2.14 1712 lines 2 ✅ works (→ 2.14.5)
2f6c7e42 development/2.15 1669 lines 1 ❌ broken (→ 2.15.1)

Compare: the only relevant difference is the last 43 lines of libmongodb.sh.

Upstream's libmongodb.sh is assembled by concatenating script fragments, and it deliberately defines mongodb_execute twice:

  1. Early in the file — the legacy wrapper that discards stdout (debug_execute mongodb_execute_print_output "$@").
  2. As a final appended fragment (it carries its own # Copyright … header and # shellcheck disable=SC2148 — the tell‑tale "no shebang" marker of a separate concatenated file) — the real version that calls mongosh directly and returns output.

In bash the last definition wins, so upstream's effective mongodb_execute is #2 (returns output) — which is exactly what mongodb_node_currently_in_cluster needs.

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 reverting mongodb_execute and 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.5 was healthy (3 voters); a cluster on 2.15.1 was broken (1 voter). We confirmed the chain end‑to‑end from the live rs.conf() (broken cluster: secondaries at votes: 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.sh was exactly the 43‑line fragment above.


The fix

Two commits:

  1. 🐛 add missing libmongo.sh fork — restores the dropped 43‑line fragment, so the file matches upstream again. mongodb_execute is once more the output‑returning definition, mongodb_node_currently_in_cluster can read rs.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)

  2. 🐛 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:

    • new helper mongodb_secondary_node_has_voting_rights checks whether the member already has votes > 0 && priority > 0;
    • mongodb_configure_secondary now 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 at votes/priority 0 and then failed (or was restarted) before promotion, the next run finishes the job and converges it to votes: 1, priority: 1;
    • the misleading error message ("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:

  • Already‑broken clusters won't fully self‑heal from the image alone. On a pod restart the bootstrap takes the persisted‑data path, which skips replica‑set (re)configuration entirely, so an existing cluster already stranded at votes: 0 still needs a one‑time manual rs.reconfig() on each replica set (shard-N and configsvr) to set members 1 and 2 to votes: 1, priority: 1. The script change guarantees correct behaviour for new bootstraps and for any path where mongodb_configure_secondary runs against a member that lacks voting rights.
  • Should we keep the second fix ? We can use a dedicated PR for that as we already have this issue on older cluster (4.2.3 have it).

@DarkIsDude DarkIsDude self-assigned this Jun 29, 2026
@bert-e

This comment was marked as resolved.

@bert-e

This comment was marked as resolved.

@DarkIsDude DarkIsDude changed the title Bugfix/zenko 5303/make voting and priority 🐛 (ZENKO-5303) make voting and priority right Jun 29, 2026
…om commit 8c1857d96d5

git-subtree-dir: solution-base/images/mongodb-sharded/debian-12
git-subtree-split: 8c1857d96d58dcb94f03edf2f4f1fb31dfd86e5c
@bert-e

This comment was marked as resolved.

@DarkIsDude
DarkIsDude requested review from a team, delthas and maeldonn June 30, 2026 14:52
@DarkIsDude
DarkIsDude marked this pull request as ready for review June 30, 2026 14:52
@bert-e

This comment was marked as resolved.

@francoisferrand francoisferrand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@delthas delthas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

@bert-e

This comment was marked as resolved.

maeldonn

This comment was marked as resolved.

@DarkIsDude

This comment was marked as resolved.

@bert-e

This comment was marked as resolved.

@maeldonn
maeldonn removed their request for review August 3, 2026 16:38

@francoisferrand francoisferrand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granting voting rights to existing replicas will break on > 7 replicas : need to add extra condition to either

  • skip mongodb_configure_secondary_node_voting if there are already too many voters
  • or detect (and ignore) the "too many voters" error
  • or ignore the mongodb_configure_secondary_node_voting error 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)

@SylvainSenechal SylvainSenechal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. nvm

@DarkIsDude
DarkIsDude force-pushed the bugfix/ZENKO-5303/make-voting-and-priority branch from 514ca64 to 709d40e Compare August 13, 2026 09:30
@DarkIsDude

Copy link
Copy Markdown
Contributor Author

Granting voting rights to existing replicas will break on > 7 replicas : need to add extra condition to either

(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)

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 ?

@delthas delthas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two threads on solution-base/images/Makefile covering the bootstrap baselines and the vendor-sync breakage.

Comment thread solution-base/images/Makefile Outdated
Comment thread solution-base/images/Makefile
@delthas

delthas commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Sorry I had to defer to Claude largely here for the comments because the subtree stuff is hard to explain easily

…rom commit e68aec05c645

git-subtree-dir: solution-base/images/mongodb-exporter/debian-12
git-subtree-split: e68aec05c6458626b233cac95bdc092b3a7194a9
a1d443b

git-subtree-dir: solution-base/images/os-shell/debian-12
git-subtree-split: a1d443b
@DarkIsDude
DarkIsDude force-pushed the bugfix/ZENKO-5303/make-voting-and-priority branch 2 times, most recently from c769d23 to c91a37b Compare August 21, 2026 10:57
@DarkIsDude
DarkIsDude requested a review from delthas August 25, 2026 08:40

@francoisferrand francoisferrand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no rush there, should it not go in 2.16 instead?

Comment thread solution-base/images/Makefile
Comment thread solution-base/images/Makefile
@DarkIsDude
DarkIsDude changed the base branch from development/2.15 to development/2.16 August 28, 2026 09:31
@bert-e

bert-e commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Branches have diverged

This pull request's source branch bugfix/ZENKO-5303/make-voting-and-priority has diverged from
development/2.16 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

  • Merge origin/development/2.16 into bugfix/ZENKO-5303/make-voting-and-priority
  • Rebase bugfix/ZENKO-5303/make-voting-and-priority onto origin/development/2.16

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

solution-base/images/*/debian-12 were vendored with a plain file copy in
2f6c7e4, never with `git subtree add`, so git knows no squash marker for
those prefixes and `git subtree merge --prefix=... --squash` fails with
"can't squash-merge: '...' was never added.". The vendor-sync targets were
therefore never runnable, next to the two other reasons fixed in the previous
commit.

`git subtree add` refuses an existing prefix, so clear the directories first.
The Zenko changes they carried are restored right after the bootstrap
commits, leaving a tree identical to this commit's parent; only the history
gains the markers. Upstream is then followed with `git subtree merge --squash`
alone, without re-vendoring.

Issue: ZENKO-5110
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
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
Now that every BITNAMI_<image>_REF is a SHA, bumping an image is a manual
step: describe it in the images README, drop the stale manual
compare-and-copy flow from how_to_upgrade.md, correct the CI job name, and
record the images vendoring path in the reviewer instructions.

Issue: ZENKO-5303
The root README linked to ./kubernetes, which no longer exists, and the
ctst README's devcontainer link was one level short.

Issue: ZENKO-5303
@DarkIsDude
DarkIsDude force-pushed the bugfix/ZENKO-5303/make-voting-and-priority branch from 4d4d0ed to 9445688 Compare August 28, 2026 10:44
@bert-e

bert-e commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Incorrect fix version

The Fix Version/s in issue ZENKO-5303 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 2.16.0

Please check the Fix Version/s of ZENKO-5303, or the target
branch of this pull request.

@delthas delthas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants