-
Notifications
You must be signed in to change notification settings - Fork 546
[networks]: add prune command #914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
aa9d622 to
fd0e2af
Compare
…check - Implement server-side prune() in NetworksService with atomic operations - Only prune networks in .running state (preserves non-running networks) - Add comprehensive test coverage (5 tests covering all edge cases) - Follow existing patterns (similar to VolumesService.prune()) - Add networkPrune XPC route and client method This merges the architectural improvements from PR apple#914 with the correct logic from PR apple#906, addressing jlogan's request to combine the best of both implementations.
|
Is doing prune in the server needless complexity for I'm asking this as a design question, not as an explicit criticism of the approach. Our principal concern for these prune operations is consistency – if we run some arbitrary collection of operations on a set of related resources, we don't want to leave the system in an inconsistent state. Principally I think that's dangling references; as an example, after a prune we should never have a case where a container refers to a non-existent network. The network delete operation is already consistent on its own – a naive but functional implementation of prune would just to try deleting each network in turn regardless of what refers to it. Ideally there'd be a distinct EBUSY-like error so one could differentiate those (which could be silently ignored) versus other conditions. I think the first, client-side-only, PR precomputed the set of unreferenced networks up front which would reduce the number of trips to the server that would return EBUSY – a simple and nice optimization. Is there something that a server-side implementation of prune can do that can't be done by combining existing operations in ContainerClient? The guiding principle here is to keep the server side as slender and simple as possible, so that it's easier to reliably maintain. It's probably worthwhile looking at what we need/want to do for other resources. For the image prune stuff there was a prune() operation for imagerefs all the way down in containerization, but we extracted that part out to container based on the same principle – the ImageStore only needs to ensure consistent pruning of content blobs, while pruning imagerefs can be dealt with in container. |
Sources/Services/ContainerAPIService/Networks/NetworksService.swift
Outdated
Show resolved
Hide resolved
|
@jglogan I agree that a client-side approach would align better with the existing design pattern. Looking at
A client-side
This keeps the server minimal and consistent with how image prune works. The The main benefit of server-side would be atomicity via |
This may have been where the confusion first occurred, as a single design pattern does not seem to be used consistently. I took reference from the
There is nothing on the server-side implementation that can't be done by combining operations in the
Below is a list of the
Is there any reason why any of these resources would EVER need to use a server-side approach (besides image prune with the content blobs)? Regarding the |
This. I've blocked out some time today to look at this (and the error handling bit I mentioned) from a broader perspective, and then will follow up here and we can discuss how to move forward. |
|
OK, here are my thoughts after a little reflection:
The principal disadvantage that I see right now is performance. Our APIs operate on a single resource at once today, each one locking the container collection. This isn't really a new issue though: @saehejkang and @suhasramanand What do you think? |
This makes sense to me. Just to be clear, this is for get, create, delete. operations for a single resource?
This also makes sense to me. Again, to be clear, this is for operations like
|
Yes. Let's get
Some reasons we might need a server-side approach:
There's nothing stopping us from that, or for moving compound operations like prune into the API server. The principal motivation for moving in the current direction (keeping things simple, and as you pointed out, consistent) for now is to start cleaning up the client code and get it in a good state for developers. Once that's done we can let time and experience guide how we enhance the client API and the underlying API server. The thing that gets under my skin at present is that our client (the APIs and the core types) is dispersed across ContainerClient and Services. The dependencies between targets within the project are messier than they need to be. I want to change this so that ContainerClient contains all of the SDK material, such that you or I can look at the docc just for ContainerClient and find what we need to code against container. There's a lot more developer documentation that could be done to support this. We still lack docs for our extension mechanisms (the plugin system in particular), for example. |
I would like to spearhead this initiative and complete it to fruition. Once
I completely agree that cleanup is important and keeping things simple/consistent is important for quality. It is all coming full circle, as I remember asking a question in the discussions about how the SDK/ContainerClient works. I am sure that working on these updates will help me wrap my head around everything and I can work on adding some docs, hopefully in the near future. Awesome points above and thank you for the all the explanations about the design/ your thought process!
How do we want to proceed with |
|
@saehejkang Consider yourself signed up for these...I'll watch for an update on this PR, we can merge it and yep, go ahead and move to the next! Thank you. |
f9961dc to
3dbc333
Compare
| } | ||
|
|
||
| let networksToPrune = allNetworks.filter { network in | ||
| network.id != ClientNetwork.defaultNetworkName && !networksInUse.contains(network.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I purposefully left the check for if a network is running because the issue did not call for it. Furthermore, if the network is not being used by any containers, I feel should it not be pruned, no matter the state?
Type of Change
Motivation and Context
Closes #893
Testing