-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
new-build test flake #6818
Comments
that doesn't seem like a flake. i would expect that to always fail if it's going to fail. |
alternatively it flaked because dockerhub didn't respond so we didn't find the "centos:7" exact match on dockerhub i guess. |
yeah only conclusion i can reach here is dockerhub flake. |
maybe we should improve the error message in that case? |
@mfojtik how do you propose doing that? the only improvement I can see is that if we fail to reach dockerhub, we ought to report that as a warning on the output. (arguably it ought to even be an error because if we exact-match something else when we would have exact matched on dockerhub but we couldn't reach it, we're going to get unpredictable behavior. The downside to making it an error is a customer who has no access to dockerhub would always just get that error). @smarterclayton is this behavior(when dockerhub is unreachable) changing w/ all your refactoring? |
@bparees can we check if you have access to dockerhub by pinging it? it that fails, we don't consider this as an error. If dockerhub is available and we fail to pull the image, we report that as an error to user. |
@mfojtik if dockerhub is unavailable that doesn't mean it's always unavailable. so it could still be an error. |
My refactoring should take it into account, but is not complete. Basically On Tue, Jan 26, 2016 at 10:09 AM, Ben Parees notifications@github.com
|
I don't follow that statement. if i have a "centos" imagestream and there is a "centos" dockerhub image, then under normal circumstances (i can reach dockerhub) i'd expect a "multiple matches" error (there are two exact matches). if dockerhub is unreachable because my network is temporarily down, there are two possible outcomes:
if dockerhub is unreachable because we got a permission denied error we also have the same possible outcomes, though we could make more guesses about the "right" thing to do since we can assume it's a permanent failure to reach dockerhub, not a temporary one. |
p3 because (I believe) it's a dockerhub flake and tests should/will always fail if we can't reach dockerhub. leaving this issue open to address the question of new-app behavior when dockerhub is unreachable. |
"materially different outcome" = we should not create an app that we would not normally create if a transient error occurs The other statement is that if one or more sources is permafail, it should always be possible to create an app on at least one match (which I think the flags would allow today, but we probably need more test cases in that path). |
Ugh, it gets even worse:
Looks like it thinks an exact match is a partial match? Probably another bug caused by a flake, but in the other direction. |
i don't get why it would think that one is a partial match. we need to print out more match information on partial matches, like we do for multiple matches. |
More nonsense matching:
|
@stevekuznetsov that's the original error reported on this issue.... |
@smarterclayton can you include more information for partial matches in the error handling changes you're making? i'd expect it to print something more like "partial match for foo: [imagestream/image/template] [namespace]/foo:bar" |
Yes
|
@smarterclayton is heavily reworking error handling right now so assigning to him in hopes he'll address how we handle dockerhub failures. Note that we should probably be reporting failures to interrogate dockerhub, also. (as a warning). |
This hasn't happened since the last failure, and is basically either the etcd flake or the image import flake. Simplifying down to one. |
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/9382/consoleFull
The text was updated successfully, but these errors were encountered: