Fix default network not being set when priority changes #38
Conversation
openshift-ci-robot
added
dco-signoff: yes
saschagrunert
force-pushed
the
network-fix
branch
from
5cdf6a7 to
1998518
Compare
openshift-ci-robot
added
size/XS
If the priorities (sorted by name) of the configured networks change, then the default network needs to be adapted to ensure it is actually being used. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
network-fix
branch
from
1998518 to
db4d13e
Compare
| @@ -334,11 +334,9 @@ func (plugin *cniNetworkPlugin) syncNetworkConfig() error { | |||
| } | |||
|
|
|||
| plugin.Lock() | |||
| defer plugin.Unlock() | |||
| if plugin.defaultNetName == "" { | |||
There was a problem hiding this comment.
doesn't removing this mean default is never set?
There was a problem hiding this comment.
It's now always set, which was my main intention 👇
Lines 337 to 338 in db4d13e
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander, saschagrunert The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@dcbw got a moment to review this? |
|
This is very similar in scope to #31 and the same issues with that one hold with this one. None of the PRs so far correctly handle the case of tearing down a container that was using the previous plugins configuration. That is being addressed with containernetworking/cni#678 upstream and when that happens, ocicni will be updated to handle config caching on DEL. And then finally we can correctly handle changing the network plugin between container ADD/DEL. |
If the priorities (sorted by name) of the configured networks change,
then the default network needs to be adapted to ensure it is actually
being used.
/cc @haircommander @mrunalp
This fix seems a bit critical to me since changing the network priority
on the fly currently does not work in CRI-O. It would be awesome if
we could tag a new release afterwards 😇