Wait for updated cache when patching status #245
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
|
/test 4.14-openshift-e2e |
2 similar comments
|
/test 4.14-openshift-e2e |
|
/test 4.14-openshift-e2e |
slintes
force-pushed
the
fix-potential-cache-issue
branch
from
6164ff2
to
c029077
Compare
|
/test 4.14-openshift-e2e |
slintes
force-pushed
the
fix-potential-cache-issue
branch
from
c029077
to
db8b085
Compare
|
/test 4.14-openshift-e2e |
Add a wait poll after status patch to ensure that the cache is updated before the next reconcile. This will prevent making incorrect decisions based on stale status data. Signed-off-by: Marc Sluiter <msluiter@redhat.com>
slintes
force-pushed
the
fix-potential-cache-issue
branch
from
db8b085
to
d7da3f3
Compare
|
/test 4.14-openshift-e2e |
| // nothing we can do anymore | ||
| return true, nil | ||
| } | ||
| return false, nil |
There was a problem hiding this comment.
Return an error?
We never return an error in these lines..
There was a problem hiding this comment.
why should I return an error here? 🤔
There was a problem hiding this comment.
func(ctx context.Context) (bool, error) is expecting an error as the last argument that the function returns, and we haven't returned any error here. I think it would be clearer to not return an error at all and return only a bool or return at least once an error. WDYT?
There was a problem hiding this comment.
the function signature is fixed, it's an argument to wait.Poll()
There was a problem hiding this comment.
Oho, I didn't notice that it's an argument to wait.Poll()
Signed-off-by: Marc Sluiter <msluiter@redhat.com>
|
/override ci/prow/4.12-openshift-e2e actual e2e succeeded, just some cleanup failed |
|
@slintes: Overrode contexts on behalf of slintes: ci/prow/4.12-openshift-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/override e2e-k8s |
|
@slintes: Overrode contexts on behalf of slintes: e2e-k8s In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| // and making wrong decisions based on it. The chance to run into this is very low, because we use RequeueAfter | ||
| // with a minimum delay of 1 second everywhere instead of Requeue: true, but this needs to be fixed because | ||
| // it bypasses the controller's rate limiter! | ||
| err := wait.PollWithContext(ctx, 200*time.Millisecond, 5*time.Second, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
NIT: I think I have noticed that wait.PollWithContext is deprecated and wait.PollUntilContextTimeout is advised to be used instead.
There was a problem hiding this comment.
I don't see that in the apimachinery version I use, but I will keep an eye on it when updating dependencies
|
/lgtm |
|
/hold cancel |
Add a wait poll after status patch to ensure that the cache is updated before the next reconcile. This will prevent making incorrect decisions based on stale status data.
ECOPROJECT-1530