Allow macvlan endpoint to start with parent down#49630
Allow macvlan endpoint to start with parent down#49630akerouanton merged 1 commit intomoby:masterfrom
Conversation
When a macvlan's parent interface is down it's not possible to send NA messages. So, ignore the error. Signed-off-by: Rob Murray <rob.murray@docker.com>
vvoland
left a comment
There was a problem hiding this comment.
SGTM, but would appreciate a second review from @akerouanton
| return true | ||
| } | ||
| log.G(ctx).WithField("", maxWait).Warn("No route for neighbour advertisement") | ||
| return false |
There was a problem hiding this comment.
If a container is attached to multiple macvlan interfaces whose parent is down when the container starts, the current implementation will incur 200ms of delay per down parent.
It's good to not barf out, but I'm wondering if we should have a mechanism to skip this polling altogether, while still trying to send gratuituous ARPs/NAs 🤔
There was a problem hiding this comment.
We can possibly remove the polling completely - as the comment for waitForMcastRoute says ...
// In CI, the NA send failed with "write ip ::1->ff02::1: sendmsg: network is unreachable".
// That error has not been seen since addition of the check that the veth's parent bridge port
// is forwarding, so that may have been the issue. But, in case it's a timing problem that's
// only less-likely because of delay caused by that check, make sure the route exists.
In CI the failures weren't predictable, just flaky. So, if we do remove it, perhaps the flakes will come back at a lower rate. So, particularly for a patch release, I don't think it's worth the risk of removing it - we may not see test failures before it ships.
(For the use-case described in the issue, a 200ms delay in startup of a standby server probably isn't a big issue. I think it's probably quite unusual to start a container with lots of 'down' macvlan interfaces. We can investigate further if it is an issue though.)
There was a problem hiding this comment.
So, particularly for a patch release, I don't think it's worth the risk of removing it
Agree. We can probably remove this polling, but that's no big deal anyway.
| apiClient := testEnv.APIClient() | ||
|
|
||
| const tap = "dummytap0" | ||
| res := icmd.RunCommand("ip", "tuntap", "add", "mode", "tap", tap) |
There was a problem hiding this comment.
Genuine question: why a tap instead of a dummy (like other tests)?
There was a problem hiding this comment.
Without the fix, the test passes with a dummy interface - I didn't investigate why.
The interfaces end up with different flags though ...
25: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 92:83:68:73:fd:f3 brd ff:ff:ff:ff:ff:ff
27: tap0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default qlen 1000
link/ether 4e:2a:e8:1f:95:48 brd ff:ff:ff:ff:ff:ff
- What I did
When macvlan's parent interface is down it's not possible to send NA messages - so, it's not possible to start a container with an endpoint in that network. Allow that, to make it possible to start a standby-container in a high availability setup where the interface is down until it's needed.
- How I did it
Ignore
ENETUNREACHfrom an NA send if no multicast route is found.- How to verify it
New integration test. Without the fix, it fails with ...
- Human readable description for the release notes
- Allow container startup when an endpoint is attached to a macvlan network where the parent interface is down.