Flaky test fixes

[lucian-tosa]

Summary

This PR aims to reduce the flakiness of the following tests:

e2e_multi_cluster_sharded_snippets

Increased the timeout of test_running, since in failing runs, by the time the diagnostics are collected, the resources become ready.

e2e_multi_cluster_appdb_upgrade_downgrade_v1_27_to_mck

Increased the timeout of test_scale_appdb. Similarly, the assertion on appdb status fails, but by the time diagnostics are collected, the resource becomes ready.

e2e_appdb_tls_operator_upgrade_v1_32_to_mck

In this test we have a race condition.

om-appdb-upgrade-tls   1          7.0.18    Running              Pending         Disabled         17m
om-appdb-upgrade-tls   1          7.0.18    Running              Running         Disabled         17m
om-appdb-upgrade-tls   1          7.0.18    Pending              Running         Disabled         17m
om-appdb-upgrade-tls   1          7.0.18    Pending              Running         Disabled         18m
om-appdb-upgrade-tls   1          7.0.18    Pending              Running         Disabled         18m
om-appdb-upgrade-tls   1          7.0.18    Running              Running         Disabled         19m

There is a moment during the operator upgrade where the resource has the status of AppDB and OM set to running. This happens very briefly before the operator starts reconciling OM and sets the OM status to Pending. In that moment, the test will very quickly pass both assertions and move on to assert healthiness by connecting to OM. This will fail since OM was not actually ready.

Reaching phase Running for resource AppDbStatus took 216.2561867237091s
Reaching phase Running for resource OmStatus took 0.0025169849395751953s

To fix this, I added a persist_for flag in our assertion methods. This makes sure that the phase we are currently asserting is reached and persists for a number of retries.

Proof of Work

Retried the above tests a few times, and all pass
https://spruce.mongodb.com/version/6911c25146ed0e00077796e3/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

Checklist

• Have you linked a jira ticket and/or is the ticket in the title?
• Have you checked whether your jira ticket required DOCSP changes?
• Have you added changelog file?
  • use skip-changelog label if not needed
  • refer to Changelog files and Release Notes section in CONTRIBUTING.md for more details

[github-actions]

⚠️ (this preview might not be accurate if the PR is not rebased on current master branch)

MCK 1.6.1 Release Notes

Bug Fixes

• Backed up the agent password in a secret for SCRAM authentication to prevent unnecessary password rotations.
• MongoDB Adding missing ownerrefs to ensure proper resource deletion by kubernetes.
• Single Cluster Deleting resources created by CRD now only happens on multi-cluster deployments. Single Cluster will solely rely on ownerrefs.
• MongoDB, MongoDBOpsManager: Improve validation for featureCompatibilityVersion field in MongoDB and MongoDBOpsManager spec.
  The field now enforces proper semantic versioning. Previously, invalid semver values could be accepted,
  potentially resulting in incorrect configurations.
• Roles configured via Ops Manager UI or API will no longer be removed by the operator

Other Changes

• Future releases will include a new asset, release_info_.json, which provides detailed information about each MongoDB Controllers for Kubernetes release, including a clear list of all container images. This will help customers, especially those running in air-gapped environments, easily identify all required images for a given release.

[m1kola]

There is a moment during the operator upgrade where the resource has the status of AppDB and OM set to running. This happens very briefly before the operator starts reconciling OM and sets the OM status to Pending.

Great observation! I think we should address this root cause instead of hacking around and making tests pass.

It will likely take more time so I suggest to separate this work from other flake fixes (where we rightfully increase timeouts).

[lucian-tosa]

I think we should address this root cause instead of hacking around and making tests pass.

@m1kola Do you think it is worth investing the time to look into the code for setting different status values? Is it something critical for customers? Otherwise, I would opt for fixing the test, which is a cheap but important fix for our CI stability

[lsierant]

is persist_for essentially equivalent to "required consecutive successes to pass"?

[lsierant]

if yes, then pls leave a brief comment or rename it, it took me a bit thinking to get the meaning of this param

[lsierant]

and one of the reason was that when looking for "persist for" suggest that the "problem persists" or the goal is not achieve yet and the situation persists. When something succeeds it's not often described as a situation that "persists". But I might be nitpicking here.

[lucian-tosa]

is persist_for essentially equivalent to "required consecutive successes to pass"?

Yes

Not the best name, I know, I couldn't find a better one. I can add a comment. But do you have a better option? no_of_successful_passes?

[m1kola]

While I'm not a fan of Gomega and Ginkgo I think we should separate "wait for something" from "consistently passes meets something" like they do:

• Wait for a condition - https://onsi.github.io/gomega/#eventually
• Ensure consistent pass of a condition - https://onsi.github.io/gomega/#consistently (edited: corrected the link)

[lucian-tosa]

As we discussed, I created a new method assert_persist_phase, and opened a ticket to track the root issue
please take another look

[Julien-Ben]

Thanks ! I like that we have the two high level methods assert_reaches_phase and assert_persist_phase separated now.

[m1kola]

I also like how we now have assert_reaches_phase and assert_persist_phase, but still find wait_for to be a bit confusing. It's difficult to get what it does just by looking at the signature.

I'm not going to block on this, but if you can think of a way so we can leave wait_for as it was before and introduce another low level function with a simple signature - that would be awesome.

[m1kola]

I think we should address this root cause instead of hacking around and making tests pass.

@m1kola Do you think it is worth investing the time to look into the code for setting different status values? Is it something critical for customers? Otherwise, I would opt for fixing the test, which is a cheap but important fix for our CI stability

We are discussing this in Slack at the moment.

[Julien-Ben]

I'm in flavor of merging this because we have too many flaky tests at the moment and this is annoying. I agree that the proper thing to do is to fix the root cause, but it's unsure when we will have time dedicated to that.

[m1kola]

Thanks for opening the ticket for the root cause as we discussed.

I would like to suggest to open separate PRs for separate fixes next time. For example, timeout increases could've been merged much sooner without having to wait for a bigger discussion.

[m1kola]

I also like how we now have assert_reaches_phase and assert_persist_phase, but still find wait_for to be a bit confusing. It's difficult to get what it does just by looking at the signature.

I'm not going to block on this, but if you can think of a way so we can leave wait_for as it was before and introduce another low level function with a simple signature - that would be awesome.