Skip to content
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

WIP: add size options for volume ls #43548

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

digging up an old branch I started 3 years ago, and which can be used for the "volumes" dashboard in Docker Desktop.

Still to do;

  • add tests
  • add api changelog
  • update swagger
  • more (?)

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@tianon
Copy link
Member

tianon commented Apr 29, 2022

To be clear before my other comments, I 100% want this very badly - the number of times I've wondered "how big is this volume?" is pretty embarrassing. 😂

My biggest concern is that calculating the size of a volume is a really expensive operation (I've got volumes in the 100s of GiB which take a non-trivial amount of time to run du -hsx on), so I think we need to be really careful that we pipe through context all the way down such that users hitting Ctrl-C after they realize this is expensive doesn't keep thrashing their disk. 😬

(For example, the prune endpoints don't do this consistently, for somewhat understandable reasons, and it's definitely bitten me more than once; docker system prune, "oh crap, Ctrl-C, do thing, docker system prune again, "already in progress")

@thaJeztah
Copy link
Member Author

Yes! Definitely need to look at all of those.

The TL;DR why I pushed this was that I was chatting with @rumpl that I had a branch somewhere in a pretty early state; @rumpl worked on this for Docker Desktop (for the volume dashboard), so I said; let me just rebase and push that branch (in all its untested glory 😁)

All that said; I think this would be a pretty useful feature to have (likely to be "optional" with a --size flag, unless we're considering some caching)

@cpuguy83
Copy link
Member

Why on List instead of Inspect?
Doing it on List is very expensive.

@thaJeztah
Copy link
Member Author

@cpuguy83 yes, it's definitely gonna be heavy.

The reason to add it to volume list was that that's where it's currently (~) used in Docker Desktop's Dashboard; I'm not 100% sure how it currently handles the size calculation (could be either through docker system df (which would be even worse), or some external code that traverses the filesystem (also ugly); perhaps @rumpl recalls how it's currently implemented.

Adding this to docker volume inspect could be another option; the downside of that would be that using that for the dashboard would potentially mean that the dashboard itself (or the backend code for it) would potentially hammer the API with individual requests for each volume; which could potentially be even more heavy? (if no maximum concurrency is set).

Not sure what'd be best for this;

  • perhaps some caching (and an option to "force" recalculating)? (depends on how accurate we want the information to be)
  • add it to docker volume inspect (but perhaps add some concurrency limit on the daemon side to prevent it handling too many concurrent calculations?)
  • at least we must be sure that concurrent requests for the same volume reuse the same calculation (I think some work for that is already in place, but may not be in use in this PR yet)

@cpuguy83
Copy link
Member

Come to think of it, adding filters for docker system df to just get the resources you want may actually be better since it already has support for de-duplication (of df requests).

@thaJeztah
Copy link
Member Author

Yes, that was implemented in #42559 (with initially the intent to have some convenience endpoints per object type; see #42560)

I wonder if that would fully solve the use-case, as I don't think we'd have all the options that <object> list provides (thinking of filtering, and possibly some fields not included) - need to check though (w.r.t. the fields at least)

Need to look what the most reasonable approach is; the backend uses
functional options, whereas other backends use an option-struct :/

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants