bridge: Enable disabling bridge interface #997
Conversation
|
/cc @EdDev @AlonaKaplan |
ormergi
force-pushed
the
bridge-cont-iface-state
branch
2 times, most recently
from
7b93279 to
a12ba15
Compare
|
Changes: fix linter issues |
ormergi
force-pushed
the
bridge-cont-iface-state
branch
from
a12ba15 to
2dccace
Compare
The new `disableContainerInterface` parameter is added to the bridge plugin to enable setting the container interface state down. When the parameter is enabled, the container interface (veth peer that is placed at the container ns) remain down (i.e: disabled). The bridge and host peer interfaces state are not affected by the parameter. Since IPAM logic involve various configurations including waiting for addresses to be realized and setting the interface state UP, the new parameter cannot work with IPAM. In case both IPAM and DisableContainerInterface parameters are set, the bridge plugin will raise an error. Signed-off-by: Or Mergi <[email protected]>
ormergi
force-pushed
the
bridge-cont-iface-state
branch
from
2dccace to
7e131a0
Compare
|
Changes: add failing test when both IPAM and disable-interface parameters are set |
mlguerrero12
left a comment
mlguerrero12
left a comment
There was a problem hiding this comment.
have you checked if there is something to do for the check command? Does it verify that the link is up?
| return fmt.Errorf("bridge port in error state: %s", hostVeth.Attrs().OperState) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
from line 581 to here, nothing should be run (except hostVeth initialization) when DisableContainerInterface is set, so I think we could encapsulate this in a function to improve readability. We could simply call that function when DisableContainerInterface is false.
Or multiple functions, something like:
if !DisableContainerInterface {
if layer3 {
setupl3()
} else {
l2link up function or code
}
checkBridgePortSTate()
}
WDYT?
There was a problem hiding this comment.
I have posted a PR for small refactoring around the places this PR change #998
Can we please address what you suggest on a follow up PR?
Hi @mlguerrero12 thanks for the review 😀 , from what I saw the check cmd does not check the interface state at all. |
Hah. Oops :-). Well, that would be good to fix, but it could be in a separate PR 😬 |
| return fmt.Errorf("bridge port in error state: %s", hostVeth.Attrs().OperState) | ||
| } | ||
| } | ||
| } |
|
Hey @squeed please take another look, let me know if there is anything else we should do 🙂 |
|
Updating docs PR containernetworking/cni.dev#137 |
|
Thanks for your contribution! |
OCPBUGS-29888: cherry-pick containernetworking/plugins#997
OCPBUGS-30045: [4.15] cherry-pick containernetworking/plugins#997
OCPBUGS-46121: [4.14] cherry-pick containernetworking/plugins#997
5 participants
The new
disableContainerInterfaceparameter is added to the bridge plugin toenable setting the container interface state down.
When the parameter is enabled, the container interface (veth peer that is placed
at the container ns) remain down (i.e: disabled).
The bridge and host peer interfaces state are not affected by the parameter.
Notes for reviewers:
to be realized and setting the interface state UP, the new parameter cannot work
with IPAM.
In case both IPAM and DisableContainerInterface parameters are set, the bridge
plugin will raise an error.
When the new parameter is set, there will be no IP to report, thus it cannot work with older versions.
In newer versions it works and an empty IP addresses slice will return.
Related Issue: #951