treat notfound and badrequest instantiate errors as fatal #9828
Conversation
| @@ -79,7 +79,7 @@ func (c *BuildConfigController) HandleBuildConfig(bc *buildapi.BuildConfig) erro | |||
| if kerrors.IsConflict(err) { | |||
| instantiateErr = fmt.Errorf("unable to instantiate Build for BuildConfig %s/%s due to a conflicting update: %v", bc.Namespace, bc.Name, err) | |||
| utilruntime.HandleError(instantiateErr) | |||
| } else if buildgenerator.IsFatal(err) { | |||
| } else if buildgenerator.IsFatal(err) || kerrors.IsNotFound(err) || kerrors.IsBadRequest(err) { | |||
There was a problem hiding this comment.
What constitutes bad request here?
There was a problem hiding this comment.
whatever the instantiate api says is a bad request, eg:
https://github.com/openshift/origin/blob/master/pkg/build/generator/generator.go#L222-L226
There was a problem hiding this comment.
And the generateBuildConfig and updateBuildConfig methods. Are all those
fatal?
On Wed, Jul 13, 2016 at 5:21 PM, Ben Parees [email protected]
wrote:
In pkg/build/controller/config_controller.go
#9828 (comment):@@ -79,7 +79,7 @@ func (c *BuildConfigController) HandleBuildConfig(bc *buildapi.BuildConfig) erro
if kerrors.IsConflict(err) {
instantiateErr = fmt.Errorf("unable to instantiate Build for BuildConfig %s/%s due to a conflicting update: %v", bc.Namespace, bc.Name, err)
utilruntime.HandleError(instantiateErr)
} else if buildgenerator.IsFatal(err) {} else if buildgenerator.IsFatal(err) || kerrors.IsNotFound(err) || kerrors.IsBadRequest(err) {whatever the instantiate api says is a bad request, eg:
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9828/files/64a7ffb0efa0dc6bf954fe8e43d249281a80c7ab#r70711225,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6V1MG5OvuArozXzYhdT5cpD0wA6ks5qVVbQgaJpZM4JL2_i
.
There was a problem hiding this comment.
those methods don't return a badrequest, but if they did i would assume so... if a request is bad, is there a reason to think retrying it will help?
if we're returning "badrequest" on something that isn't actually a bad request, then we're doing it wrong, i think.
There was a problem hiding this comment.
(they don't directly return one. of course an api call within them could, but again i'd argue that's a legit fatal error then, unless k8s is returning bad request when it should not)
There was a problem hiding this comment.
I'm just asking if you're confident we're not doing it wrong.
On Wed, Jul 13, 2016 at 5:31 PM, Ben Parees [email protected]
wrote:
In pkg/build/controller/config_controller.go
#9828 (comment):@@ -79,7 +79,7 @@ func (c *BuildConfigController) HandleBuildConfig(bc *buildapi.BuildConfig) erro
if kerrors.IsConflict(err) {
instantiateErr = fmt.Errorf("unable to instantiate Build for BuildConfig %s/%s due to a conflicting update: %v", bc.Namespace, bc.Name, err)
utilruntime.HandleError(instantiateErr)
} else if buildgenerator.IsFatal(err) {} else if buildgenerator.IsFatal(err) || kerrors.IsNotFound(err) || kerrors.IsBadRequest(err) {those methods don't return a badrequest, but if they did i would assume
so... if a request is bad, is there a reason to think retrying it will help?if we're returning "badrequest" on something that isn't actually a bad
request, then we're doing it wrong, i think.—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9828/files/64a7ffb0efa0dc6bf954fe8e43d249281a80c7ab#r70712740,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p0elqKqKUHULUd0Fey3_d5W2cbi9ks5qVVkYgaJpZM4JL2_i
.
There was a problem hiding this comment.
i'm confident the openshift bits aren't, i can't speak to upstream.
|
Lgtm [merge] |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 64a7ffb |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6232/) |
|
flake #8701 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6291/) (Image: devenv-rhel7_4592) |
|
flake #9775 (i think) |
|
Evaluated for origin merge up to 64a7ffb |
fixes #9827