-
Notifications
You must be signed in to change notification settings - Fork 26
[release-0.9] Backport test fixes #375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release-0.9] Backport test fixes #375
Conversation
- Added wait period for CR to be deleted: MHC wasn't waiting for the CR deletion which failed the test because lease wasn't removed - Increased waiting for lease removal for 2 minutes: MHC will remove the lease after the CR is removed, it will requeue Reconciles until then. That requeing is causing an exponential backoff - making current 1 minute wait too short (the remediation that is triggered by the CR deletion will not trigger the lease removal because it'll short circuit since MHC will not be found as it's using the CR's name). Signed-off-by: Michael Shitrit <[email protected]> (cherry picked from commit 2ac34fd)
Sometimes cleanup fails due to API errors, which fails the next test as well. Just skip the cleanup here, it's not really needed... Signed-off-by: Marc Sluiter <[email protected]> (cherry picked from commit e817b0b)
not relevant for the tests, but cleans up logs Signed-off-by: Marc Sluiter <[email protected]> (cherry picked from commit eabd704)
Signed-off-by: Marc Sluiter <[email protected]> (cherry picked from commit 542f923)
Fix flaky e2e (cherry picked from commit 2545fc1)
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: slintes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bundle build errors... /retest |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I left a couple of minor notes
Eventually(func(g Gomega) { | ||
g.Expect(k8sClient.Get(context.Background(), ctrl.ObjectKeyFromObject(mhc), mhc)).To(Succeed()) | ||
g.Expect(*mhc.Status.CurrentHealthy).To(BeNumerically("==", len(workers.Items))) | ||
}, "5m", "5s").Should(Succeed(), "CR not deleted") | ||
|
||
By("waiting for CR deletion (after finilizers are removed by snr) in order to trigger lease removal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo s/finilizers/finalizer
. Also, maybe "snr" capitalized?
for _, cr := range list.Items { | ||
if annotationNodeName, exists := cr.GetAnnotations()[commonannotations.NodeNameAnnotation]; exists && annotationNodeName == nodeName { | ||
log.Info("remediation resource still exist") | ||
return fmt.Errorf("remediation exist") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's just a test, but I think that enhancing the error message with info about which remediation was found might be helpful.
until "$@"; do | ||
n=$((n+1)) | ||
if [ $n -ge $retries ]; then | ||
echo "Command failed after $retries attempts: $*" | ||
return 1 | ||
fi | ||
echo "Retry $n/$retries failed. Retrying in $wait seconds..." | ||
sleep $wait | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I'm going to reuse this :)
My comments were not blocking, better avoid conflicts /lgtm To get more reviews, feel free to unhold |
/hold cancel |
4d5918a
into
medik8s:release-0.9
Fix flaky tests by backporting several fixes
RHWA-100