Add network namespace pool support

[aaronlehmann]

This adds netNSPoolSize pool options which allow setting a target network namespace pool size. buildkitd will create this number of network namespaces at startup (without blocking). When a container execution finishes, the network namespace gets returned to the pool. If the pool goes above the target size, there is a grace period to allow network namespaces to be reused, and if this passes without reuse, the extra namespaces will be released.

[aaronlehmann]

I was also wondering if there should be some kind of orderly shutdown where we release the network namespaces in the pool when buildkitd shuts down (in addition to cleaning up on restart). It doesn't look like there's a mechanism right now to block the shutdown on cleanup activities like this, and I think in some situations it could block shutdown for a long time, so I'm not sure if it makes sense or not.

[coryb]

lgtm, should probably add the new config option to the example buildkitd.toml: https://github.com/moby/buildkit/blob/master/docs/buildkitd.toml.md

[aaronlehmann]

should probably add the new config option to the example buildkitd.toml

Good point, added.

[tonistiigi]

I was also wondering if there should be some kind of orderly shutdown where we release the network namespaces in the pool when buildkitd shuts down (in addition to cleaning up on restart).

Yes, if buildkitd is shut down gracefully then it should not leave any netns mounted. The cleanup on boot is only for the case where the daemon got killed with sigkill and no cleanup was possible. We can create a cleanup function that gets called on defer that would release the items available in the pool and ideally way for all the other mounted ones to be released as well and detect if any of these paths is misbehaving(assuming everything is correct the code path that asked for the netns also cleans up after itself but we can double check).

[tonistiigi]

With the current implementation isn't it more like a cni-pool-size?

[aaronlehmann]

Renamed.

[tonistiigi]

I'm not sure if this is quite correct. You do actualSize > pool.targetSize comparison when deciding to add a cleanup func and also inside the cleanup but I don't think there is a guarantee that these conditions are the same when these places get called for a specific ns.

At least it seems safer if we skip this check end just always call cleanUpToDefaultSize() that is a no-op if available is not greater than size.

[aaronlehmann]

The namespaces are reused in FIFO order, so I think this is correct as written. If we were not above the target size at put time, it will be some other namespace that brings us the target size, and that's the one we'll want to release if it hasn't been reused after 5 minutes.

But thinking about this some more, I don't think we want the FIFO order. It means we'll cycle through the namespaces and it will be hard to shrink back to the target size if there's constant activity. LIFO seems better. So I've rewritten this to use LIFO and align more closely to your suggestion.

[aaronlehmann]

Yes, if buildkitd is shut down gracefully then it should not leave any netns mounted. The cleanup on boot is only for the case where the daemon got killed with sigkill and no cleanup was possible. We can create a cleanup function that gets called on defer that would release the items available in the pool

I couldn't find a place to put this defer, so I tried a different approach where cniProvider will launch a cleanup when the main application context is cancelled. This will not block graceful shutdown (which has pros and cons). Feel free to point me in a different direction.

[aaronlehmann]

I couldn't find a place to put this defer, so I tried a different approach where cniProvider will launch a cleanup when the main application context is cancelled. This will not block graceful shutdown (which has pros and cons). Feel free to point me in a different direction.

Hmm, this doesn't seem to work well in practice because buildkitd exits before cniProvider has a chance to do much cleanup. I could create a blocking Close method on cniProvider, but I'm not sure where to call this. Any suggestions?

[tonistiigi]

Yeah, it looks like this requires a bit bigger change. I think we need to pass it through from WorkerController -> Worker -> NetProviders and add blocking cleanup method for each one. There might be something else in this call path as well that we have missed and might need cleanup.

A quicker patch would be to let the grpc shut down and then just do a hardcoded cleanup, maybe just looking for mountpoints under the state folder and unmount them directory without too much cni specifics. But I think the first solution is better.

[aaronlehmann]

Updated to create these Close methods and close the Controller with a defer in main.

[tonistiigi]

isn't this supposed to be && runtime.GOOS != "windows" . If not then I think the comment is confusing.