-
Notifications
You must be signed in to change notification settings - Fork 370
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
remove config mismatched port during creation #2582
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2582 +/- ##
===========================================
+ Coverage 42.18% 65.43% +23.24%
===========================================
Files 153 287 +134
Lines 18491 29653 +11162
===========================================
+ Hits 7801 19402 +11601
+ Misses 9991 8493 -1498
- Partials 699 1758 +1059
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 overall, thanks
if interfaceConfig.OFPort != 0 { | ||
return interfaceConfig.OFPort, nil | ||
if err := c.ovsBridgeClient.DeletePort(interfaceConfig.PortUUID); err != nil { | ||
return 0, fmt.Errorf("IPSec tunnel interface config doesn't match cached one, fail to delete stale IPSec tunnel port %s: %v", interfaceConfig.InterfaceName, 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.
The first sencence can be a separate log indicating a mismatch is detected and it's going to delete the stale port.
klog.InfoS("IPSec tunnel interface config doesn't match the cached one, deleting the stale IPSec tunnel port", "node", nodeName, "interface", interfaceConfig.InterfaceName)
if err := c.ovsBridgeClient.DeletePort(interfaceConfig.PortUUID); err != nil {
return 0, fmt.Errorf("failed to delete stale IPSec tunnel port %s: %v", interfaceConfig.InterfaceName, 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.
done
} | ||
} else { | ||
} | ||
if !ok || hasRemoved { |
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.
nit: you can rename ok
to exists
and set it to false
after deleting the stale port, then only need to check exists
here.
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.
done
Signed-off-by: Lan Luo <[email protected]>
Hi @tnqn Could you help to take a look again? last time I addressed a comment but no review yet. thanks! |
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
/test-all |
Signed-off-by: Lan Luo [email protected]