-
Notifications
You must be signed in to change notification settings - Fork 55
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
If the IP is on the lo device already don't fail #205
If the IP is on the lo device already don't fail #205
Conversation
9d9eecd
to
f99e218
Compare
f99e218
to
ef71908
Compare
great catch, I think we may keep that the same and instead, on the caller, just ignore the error and log, but not stop the loadbalancer creation, since the tunnels may have different errors on different OS I think this approach is more resilient cloud-provider-kind/pkg/loadbalancer/server.go Lines 120 to 126 in defae81
replacing |
Signed-off-by: Dave Protasowski <[email protected]>
pkg/loadbalancer/tunnel.go
Outdated
if err != nil { | ||
return err | ||
klog.V(4).Infof("error adding IP to local interface: %v - %s", err, output) |
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.
wrap the error here, I think this tunnel
is ok to return the error ... I find better if we swallow it here by logging it at normal level, as the tunnel is an addon for certain environments it should not block the loadbalancer creation but it should be easy for the user to find the error in the logs
cloud-provider-kind/pkg/loadbalancer/server.go
Lines 120 to 126 in defae81
if s.tunnelManager != nil { | |
klog.V(2).Infof("updating loadbalancer tunnels on userspace") | |
err = s.tunnelManager.setupTunnels(loadBalancerName(clusterName, service)) | |
if err != nil { | |
return nil, err | |
} | |
} |
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.
pushed - take a look at the edits
/approve one change only for final lgtm #205 (comment) Thanks |
@dprotaso: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/lgtm Thanks |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, dprotaso The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This might be the fix for #189 or #192