deployment: retry failed watch when observed controller has old resource version #12910
Conversation
|
[test] |
mfojtik
force-pushed
the
retry-rc-watch
branch
from
809f8e0 to
3dffd46
Compare
|
[test] flake: #12911 |
| "github.com/openshift/origin/pkg/deploy/api" | ||
| deployutil "github.com/openshift/origin/pkg/deploy/util" | ||
| ) | ||
|
|
||
| var ( | ||
| // ErrUnknownDeploymentPhase is returned for WaitForRunningDeployment if an unknown phase is returned. | ||
| ErrUnknownDeploymentPhase = errors.New("unknown deployment phase") | ||
| ErrTooOldResourceVersion = errors.New("too old resource version") |
There was a problem hiding this comment.
Isn't this error typed in the watch library?
There was a problem hiding this comment.
It's strange that you recreate the error here and return it in place of the previous error which is essentially the same error?
There was a problem hiding this comment.
@Kargakis nope, I don't see any.
There was a problem hiding this comment.
@Kargakis well I can return the original error, but I will have to type cast it twice :)
|
@smarterclayton @ncdc I've seen similar BZ about lifecycle hooks (same error message). Does something changed in how the watches work in last ~2 weeks? |
|
If the lifecycle hook bug is the one with the mysql pod, I didn't find any hooks on that DC so we may want to check why we used the watch in the first place. |
|
@Kargakis yes, that is what I'm looking at right now. (however, I don't think we touched the lifecycle hooks recently, so I'm interested if something changed in watches that causes this errors to surface now). |
|
@Kargakis you ok merging this? |
|
Can't we at least unit test this? I am scared of untested api changes going in. |
|
@Kargakis agree, unit test sounds reasonable. |
|
@openshift/api-review since this is essentially an api change |
mfojtik
force-pushed
the
retry-rc-watch
branch
from
3dffd46 to
d214e60
Compare
|
@Kargakis unit test added, PTAL |
|
Evaluated for origin test up to d214e60 |
|
API change lgtm |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/219/) (Base Commit: 05377d4) |
|
lgtm [merge] |
|
Evaluated for origin merge up to d214e60 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/257/) (Base Commit: efab088) (Image: devenv-rhel7_5923) |
Fixes: #12905
This is pretty hard to reproduce or wrap with unit test :-(
@Kargakis PTAL