bridge: Extract some netlink operations code to file#998
bridge: Extract some netlink operations code to file#998ormergi wants to merge 1 commit intocontainernetworking:mainfrom
Conversation
ormergi
force-pushed
the
bridge-ref-netlink-op
branch
from
2f3559d to
34e5f3e
Compare
|
/cc @EdDev @AlonaKaplan |
pkg/link/opstate.go
Outdated
| } | ||
|
|
||
| if idx == len(retries)-1 { | ||
| return nil, fmt.Errorf("timeout waiting for %q state up", linkName) |
There was a problem hiding this comment.
nit: please say what state the link is currently in.
pkg/link/opstate.go
Outdated
| func SetUp(linkName string) error { | ||
| link, err := netlink.LinkByName(linkName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to retrieve link: %v", err) |
There was a problem hiding this comment.
nit: please use %w to wrap errors everywhere.
There was a problem hiding this comment.
I didn't saw error being unwrapped in the bridge plugin code, also we didnt wrapped them before this change 🙂
Why is it necessary to wrap them here?
There was a problem hiding this comment.
because all of this code was written before %w was a thing :-).
squeed
left a comment
squeed
left a comment
There was a problem hiding this comment.
A few small nits, then this looks good!
ormergi
force-pushed
the
bridge-ref-netlink-op
branch
from
34e5f3e to
ad9910e
Compare
|
@squeed thanks for the review 🙂 , please take another look. |
|
|
||
| link, err = netlink.LinkByName(linkName) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
let's be consistent and return here fmt.Errorf("failed to retrieve link: %v", err) as in SetUp
| } else { | ||
| // If layer 2 we still need to set the container veth to up | ||
| if err := netns.Do(func(_ ns.NetNS) error { | ||
| link, err := netlink.LinkByName(args.IfName) |
There was a problem hiding this comment.
Why do you want a function for this? I understand the one for retries which includes some logic with the for loop but this is simple and not used anywhere else.
There was a problem hiding this comment.
My intent was to improve readability, even though there is no much of logic its less code to read, and may reduce some cognitive load (I dont need to know we need to fetch the link in order to bring it down).
Also, with #997 changes, it makes the code read better:
...
else if !disableContIface {
link.SetUp(ifaceName)
}
...
WDYT?
There was a problem hiding this comment.
no, I think this is simple enough. If you want to improve readability, then abstract all the logic inside if layer3 branch, then in your other pr, you can do
if !disableContIface {
if layer {
layer3func()
} else {
thiscode
}
WaitOperStateUp()
}
This is a small refactoring around netlink operations logic. Setting link state up and waiting for link oper state code is moved to link package. Signed-off-by: Or Mergi <[email protected]>
ormergi
force-pushed
the
bridge-ref-netlink-op
branch
from
ad9910e to
4720e51
Compare
3 participants
This is a small refactoring around netlink operations logic.
Setting link state up and waiting for link oper state code is moved to link package.