Cleanup interfaces properly when vxlan plumbing fails

[sanimej]

Related to #1765

There have been many reports of container creation failing with "Error creating vxlan: file exists" error. This happens very randomly (but typically after lot of stack deploy/rm) without a specific sequence to recreate the issue.

I haven't been able to recreate it either. But with a forced error I can hit this issue. When setting up the overlay network we create a separate namespace for the overlay network, create a vxlan device and move it into the overlay namespace and configure it (interface name for ex). If something goes wrong here vxlan interface wasn't getting cleaned up properly. If that network is delated, another network could get its vni id. Since an orphaned interface exists with that vni id, new vxlan device can't be created.

Changes in this PR does proper cleanup of the vxlan interface and the overlay namespace on any failure so that subsequent attempts or another network which gets the same vxlan id can succeed.

Signed-off-by: Santhosh Manohar [email protected]

[mavenugo]

This makes sense for initSubnetSandbox. How does the restore path work ?
I have to spend some time analyzing that as well to confirm if this change is complete.

[mavenugo]

@sanimej Reviewing the restore path, I think if restoreSubnetSandbox fails for any reason, I think we will end up in the same issue.
And the current fix will not help, because your changes currently performs deleteInterface for vxlan device only if AddInterface fails. But if the restoreSubnetSandbox fails, then the failure will happen in createVxlan call and there is no recovery for that.

Do you think it is valid ?

[sanimej]

@mavenugo The way restore works is fundamentally different. For the restore case we do handle something similar, but at the network level; https://github.com/docker/libnetwork/blob/master/drivers/overlay/overlay.go#L99

Going strictly by the code, yes, restoreSubnetSandbox can also fail. But all the recent issues reported for this error are in swarm mode (ie: not the restore code path). For the restore case we have to try with a forced error and verify things end to end. It belongs in a separate PR.

[mavenugo]

I agree. Thanks.

[fcrisciani]

not super familiar with this part of the code, but the fix looks reasonable to me

[joshsleeper]

polite bump for @mavenugo~

Any particular reason this has been sitting [seemingly] approved but unmerged for so long?
I'm running into this exact issue today with the absolute latest versions of everything.

[fcrisciani]

@selansen can you take over this patch?

[selansen]

Will do that.

[ctelfer]

Reviewed this, and it looks mostly ok to me. But I am a bit nervous about the "re-once"ing. (i.e. assigning a new sync.Once instance. I get what the rationale, but am worried there is a race there. Need to think it through a bit more.

[ctelfer]

I think there is a race here. Assume that 2 goroutines are trying to join the network at the same time. The first one gets in and hits the "once" and starts spinning things up but fails. On the way out (but not completely out) it resets the "once". Now the second goroutine joins the sandbox and succeeds, going through the once and back out again before the first goroutine returns from initSubnetSandbox(). it assigns a 'nil' to the subnet.initErr field because it succeeded. Then the first goroutine completes and assigns its error to subnet.initErr. Subsequent goroutines will all get the wrong initErr as a result.

This is probably not likely to happen because there aren't blocking calls between reinitializing the "once" and setting the initErr field. But one should never rely on things like this.

[ctelfer]

Having thought through this and reviewed sync.Once for loopholes and the like, I think the options are either to accept as is, or use a pattern like:

        var err error
        s.Lock()
        if !s.initialized {
                if err = n.initSubnetSandbox(s, restore); err == nil {
                        s.initialized = true
                }
        }
        s.Unlock()
        return err

I don't see any way of re-oncing that is clean.

[fcrisciani]

carried by #2146