envtest: increase timeouts while testing PVC selection #1379
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.
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.
@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.
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.
@nirs I changed the PR message to a more clear and short one.
There was a problem hiding this comment.
@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.
@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. |
ELENAGER
changed the title
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
ELENAGER
force-pushed
the
issue_1372
branch
2 times, most recently
from
bd47fc7
to
87d9f47
Compare
ELENAGER
changed the title
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