CLOUDP-349087: Fix TLS disable + scale up test #490
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MCK 1.5.0 Release NotesNew Features
Bug Fixes
|
Julien-Ben
added
the
skip-changelog
Julien-Ben
commented
Oct 3, 2025
| @pytest.mark.e2e_disable_tls_scale_up | ||
| def test_tls_is_disabled_and_scaled_up(replica_set: MongoDB): | ||
| replica_set.load() | ||
| replica_set["spec"]["members"] = 5 |
There was a problem hiding this comment.
The issue in the test is that it was doing the update in two steps (scale, and then disable tls), while the whole point is to change them at the same time. (on top of having a duplicate function name)
lsierant
reviewed
Oct 3, 2025
| // Check if TLS is being disabled. If so, we need to lock replicas at the current member count | ||
| // to prevent scaling during the TLS disable operation. This decision is made once here and | ||
| // applied to both the StatefulSet and OM automation config. | ||
| tlsWillBeDisabled, err := checkIfTLSWillBeDisabled(conn, rs, log) |
There was a problem hiding this comment.
what if we just block this with validation and say that it's not possible to change both member count and disabling TLS at the same time?
There was a problem hiding this comment.
Yes, after discussing it in DM, let's do that instead of adding complexity to the reconcile loop. This is not a common use case anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This test has been broken for a while. Because there were two functions with the name
test_tls_is_disabled_and_scaled_up, only one of them was running everytime.I think this scenario has low chance of existing in production, hence why we never had a ticket related to this bug.
On top of that it performed the update in two separate step, whereas to test this behaviour it should be done in one update.
I uncovered it as part of the larger refactoring of the controller. But to keep the PR scope reasonable, I extracted the related changes as they are self contained
The bug may exist in multi-cluster as well. This bug was first discovered in 2021: https://jira.mongodb.org/browse/CLOUDP-80768
The blocking mechanism will be implemented in a better way after the multi-cluster first refactor, since we will keep track of a global reconciler state, notably holding the target number of replicas for this reconciliation.
Proof of Work
In the commit (1d8847e) which just fixes the e2e test, it fails: evg task Showing that the reconciler had to be fixed as well.
The PR is correct if CI is green again.
Checklist
skip-changeloglabel if not needed