-
Notifications
You must be signed in to change notification settings - Fork 52
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
envtest: increase timeouts while testing PVC selection #1379
base: main
Are you sure you want to change the base?
Conversation
controllers/vrg_volrep_test.go
Outdated
@@ -395,7 +395,7 @@ var _ = Describe("VolumeReplicationGroupVolRepController", func() { | |||
}) | |||
}) | |||
It("updates the status", func() { | |||
Eventually(vrgResourceVersionGet).ShouldNot(Equal(vrgResourceVersion)) | |||
Eventually(vrgResourceVersionGet, timeout, interval).ShouldNot(Equal(vrgResourceVersion)) |
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.
After #1370, timeout is only one second, so this change looks good enough as a quick fix. We may have a real bug here, not watching the configmap or not updating it properly when it changes, or not considering the updated config map after if was updated. Lets see if this change fixes the flakiness.
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.
@ELENAGER This PR message should be in the commit message. After merging this, looking at git log will show no info why this change was made, and finding the PR from git log is hard. You need to find the commit on github using the commit hash, and then find the PR link in the commit.
For example:
commit 7b8cf5460c0ca2f163a58f9f3b5f55dab116402e (HEAD -> main, upstream/main)
Author: Shyamsundar Ranganathan <srangana@redhat.com>
Date: Thu May 9 15:16:10 2024 -0400
Convert sourced Rook YAMLs to URLs with kustomize
Signed-off-by: Shyamsundar Ranganathan <srangana@redhat.com>
We don't have a link to the PR or even the PR numebr in git log. We should always have all the info related to the commit in the commit message. The PR message should be a short overview of the entire change including one or more commits.
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.
Default Eventually timeout an dinterval are 1s and 10ms as per documentation, so adding this change should not actually change the overall behavior.
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.
@nirs I changed the PR message to a more clear and short one.
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.
@nirs I checked your suggestion about a real problem here: #1379 (comment). We are registering for a changes in configMap, and reconciling all existing VRGs upon every change. So, maybe it's just a timing issue anyway?
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.
@ELENAGER based on #1379 (comment) this is not a timeout issue. Can you share the logs proving that this is a timeout issue?
Increasing timeout to 10 seconds is way too much. I don't see any reason why in this tiny integration test one second is not enough to process changes.
I do not think this is a timeout issue, as the logs indicate the VRG reconcile finds no change that needs further reconciliation and stops further reconciles. At which point only an external change or event would reconcile the resource again, which we are not triggering in the tests as it stands. As a result increasing the timeout will not help in this case. |
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
bd47fc7
to
87d9f47
Compare
During the test case we are updating the configMap with an new value (ramenConfig.VolumeUnprotectionEnabled = false). VRG reconcile is reading the updated config map, but we still need time after updating the PVC till the VRG resource will be updated too. I see, that usually we are giving a longer timeout for Eventually function, but in this test it is default - 1 sec. I increased the timeout to 10 seconds and hope it might help.
Fixes: #1372