-
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
avoid repetitive errors on adding fdb entry if it already exists #11788
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on @rajatchopra 's description of how F5 works.
@@ -977,7 +977,12 @@ func (f5 *f5LTM) AddVtep(ipStr string) error { | |||
Name: macAddr, | |||
Endpoint: ipStr, | |||
} | |||
return f5.post(url, payload, nil) | |||
err = f5.post(url, payload, nil) | |||
if err != nil && err.(F5Error).httpStatusCode != HTTP_CONFLICT_CODE { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're sure it'll always be an F5Error type returned from f5.post()? Might be safer to check the cast and always return the error if !err.(F5Error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are constructing this error type.
https://github.com/rajatchopra/origin/blob/f5_fdb/pkg/router/f5/f5.go#L407
The field httpStatusCode is always filled up either with nil value '0' if the request failed without obtaining the http status code or with the actual status code here:
https://github.com/rajatchopra/origin/blob/f5_fdb/pkg/router/f5/f5.go#L430
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool.
Example error that this code tries to bypass:
|
LGTM |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11152/) (Image: devenv-rhel7_5320) |
Evaluated for origin merge up to dd78339 |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to dd78339 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11152/) (Base Commit: 6bf8b33) |
cc @openshift/networking