Add proper error message to OVS health check #17890
Conversation
openshift-ci-robot
added
the
size/M
|
PTAL @openshift/networking |
pkg/network/node/healthcheck.go
Outdated
| if !healthFn() { | ||
| return false, fmt.Errorf("OVS health check failed") | ||
| if err := healthFn(); err != nil { | ||
| return false, fmt.Errorf("OVS health check failed, %v", err) |
There was a problem hiding this comment.
How about: "OVS health check failed: %v"
There was a problem hiding this comment.
you're right, i'll change it
pkg/network/node/sdn_controller.go
Outdated
| @@ -78,12 +79,12 @@ func (plugin *OsdnNode) alreadySetUp(localSubnetGatewayCIDR string, clusterNetwo | |||
| } | |||
| } | |||
| if !found { | |||
| return false | |||
| return errors.New("Local subnet gateway CIDR not found") | |||
There was a problem hiding this comment.
Errors are usually lowercase (as @pravisankar keeps reminding me)
Same for the following errors.
There was a problem hiding this comment.
Do you mean that they should start with a lowercase letter like this? "local subnet gateway CIDR not found"
openshift-merge-robot
added
the
needs-rebase
The healtFn and alreadySetup functions were returning a boolean and they should return an error with information in case of failures. Fixes: bz#1504011
imcsk8
force-pushed
the
healthcheck-error-message
branch
from
6972988 to
fd3af25
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imcsk8, knobunc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
| @@ -96,7 +97,7 @@ func deleteLocalSubnetRoute(device, localSubnetCIDR string) { | |||
| }) | |||
|
|
|||
| if err != nil { | |||
| glog.Errorf("Error removing %s route from dev %s: %v; if the route appears later it will not be deleted.", localSubnetCIDR, device, err) | |||
| glog.Errorf("error removing %s route from dev %s: %v; if the route appears later it will not be deleted.", localSubnetCIDR, device, err) | |||
There was a problem hiding this comment.
Our convention is user returned errors start with lower case and log messages start with upper case. In this case 'Error removing...' is fine as this is a log msg.
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue. |
The healtFn and alreadySetup functions were returning a boolean
and they should return an error with information in case of
failures.
Fixes: bz#1504011