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

Iss34 check avail cache size for download #321

Merged

Conversation

prestonsn
Copy link
Contributor

@prestonsn prestonsn commented Jun 10, 2022

Pull-request for issue #34. Addresses missing check for available disk space prior to installing package(s). This PR only provides a cache directory space check, and does not check for available disk space at package install paths.

  • Adds tracking of package download sizes.
  • Reports download size of package(s) to the user prior to accepting an install.
  • Reports download size of package(s) through json formatting when enabled.
  • Adds sufficient error handling for statfs syscall.
  • Adds tests to ensure accuracy of reported download package size(s).
  • Adds test to intentionaly trigger an out of disk space error.
  • Fix Copyright year in file headers.

An install will now appear as follows:

root [ /build ]# ./bin/tdnf install htop gdb vim

Installing:
vim                    x86_64           8.2.4925-1.ph4         photon-updates     3.44M           1.53M
gdb                    x86_64           10.1-2.ph4             photon-updates    10.54M           4.21M
htop                   x86_64           3.1.2-1.ph4            photon-updates   383.54k         163.31k

Total installed size:  14.36M
Total download size:   5.89M
Is this ok [y/N]: y

Downloading:
vim                                    1600344 100%
gdb                                    4410176 100%
htop                                    167227 100%
Testing transaction
Running transaction
Installing/Updating: htop-3.1.2-1.ph4.x86_64
Installing/Updating: gdb-10.1-2.ph4.x86_64
Installing/Updating: vim-8.2.4925-1.ph4.x86_64

Complete!

@vmwclabot
Copy link
Member

@prestonsn, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link
Member

@prestonsn, your company's legal contact has approved your signed contributor license agreement. It will also be reviewed by VMware, but the merge can proceed.

@vmwclabot
Copy link
Member

@prestonsn, VMware has approved your signed contributor license agreement.

client/packageutils.c Outdated Show resolved Hide resolved
common/utils.c Outdated Show resolved Hide resolved
tools/cli/lib/installcmd.c Outdated Show resolved Hide resolved
tools/cli/lib/installcmd.c Outdated Show resolved Hide resolved
client/api.c Outdated Show resolved Hide resolved
client/packageutils.c Outdated Show resolved Hide resolved
Copy link
Contributor

@tapakund tapakund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments.

client/api.c Outdated Show resolved Hide resolved
client/packageutils.c Show resolved Hide resolved
@sshedi
Copy link
Contributor

sshedi commented Jun 14, 2022

I have two more things.

  1. Can we somehow create a test for negative scenario? Where available size is less than download size? It might be difficult but it's good to have it.
  2. Do we need to add + 5 or 10% to total download size ? rpms are zstd compressed, after extraction size will be more. So we might be able to download but not extract and install the rpm because space won't be available. Consider linux rpm for example,
➜ $ ls
boot  etc  lib  linux-5.10.78-6.ph4.x86_64.rpm  var
[Tue 14.Jun.2022 10:51:39] sshedi@ ~/work/photon-dev/t (dev)➤
➜ $ du *
14M	boot
4.0K	etc
23M	lib
29M	linux-5.10.78-6.ph4.x86_64.rpm
4.0K	var

rpm is 29M, after extraction it's ~37M.

Both these are my queries, I'm okay with not having these things.

Changes LGTM.

@oliverkurth
Copy link
Contributor

  1. Do we need to add + 5 or 10% to total download size ? rpms are zstd compressed, after extraction size will be more. So we might be able to download but not extract and install the rpm because space won't be available. Consider linux rpm for example,

rpm is 29M, after extraction it's ~37M.

Shreenidhi, we don't have to guess, we know the exact install size from the metadata. The problem is that the package may be installed across multiple filesystems, and we don't know how the sizes of installed files (AFAIK, maybe we can finds out).

However, this is not a common case, so maybe we can make the check optional, or even detect it automatically.

@prestonsn
Copy link
Contributor Author

prestonsn commented Jun 14, 2022

  1. Can we somehow create a test for negative scenario? Where available size is less than download size? It might be difficult but it's good to have it.

I'm currently working on adding some functionality to the test code to mount a tmpfs filesystem with intentionally limited diskspace. Then supply its mounted path as the cache directory path to tdnf. This should trigger the test case as you mentioned.

Copy link
Contributor

@oliverkurth oliverkurth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add the package size to the json info - should be trivial.

client/defines.h Outdated Show resolved Hide resolved
client/packageutils.c Show resolved Hide resolved
include/tdnferror.h Show resolved Hide resolved
pytests/tests/test_cache.py Show resolved Hide resolved
pytests/tests/test_cache.py Show resolved Hide resolved
@prestonsn
Copy link
Contributor Author

Please also add the package size to the json info - should be trivial.

Added in latest commit: e9c6d07

@prestonsn
Copy link
Contributor Author

Latest commit 9560534 adds a test to intentionally trigger an out-of-diskspace error. It uses the new script pytests/mount-small-cache.in to mount the small directory. If this mount fails, then the test is skipped.

@oliverkurth
Copy link
Contributor

oliverkurth commented Jul 8, 2022

This looks good to me. Nice work! Please squash the commits together into a smaller number of commits that make sense on their own. fb9b995 could be worth its own PR, but should be left as a separate commit at least. Squashing all other commits into a single one is fine too.

I am wondering about corner cases:

  • if the file is already downloaded to cache, would this check for remaining space even though download isn't needed?
  • similar, if we do tdnf install ./foo-1.2.3.rpm or tdnf install file:///some/path/foo-1.2.3.rpm we don't download, so no need to check for the size. I don't think this is addressed.

This could be fixed in a separate PR, but if not addressed we should create an issue.

@prestonsn prestonsn force-pushed the iss34-CheckAvailCacheSizeForDownload branch from c32c6fb to 0435d23 Compare July 11, 2022 22:10
@prestonsn
Copy link
Contributor Author

This looks good to me. Nice work! Please squash the commits together into a smaller number of commits that make sense on their own. fb9b995 could be worth its own PR, but should be left as a separate commit at least. Squashing all other commits into a single one is fine too.

Thank you for the advice! Everything has been squashed into 3 commits, and those mentioned changes have been merged in their own PR #333.

I am wondering about corner cases:

* if the file is already downloaded to cache, would this check for remaining space even though download isn't needed?

* similar, if we do `tdnf install ./foo-1.2.3.rpm` or `tdnf install file:///some/path/foo-1.2.3.rpm` we don't download, so no need to check for the size. I don't think this is addressed.

This could be fixed in a separate PR, but if not addressed we should create an issue.

Good point, I've opened issue #334 and added some more details there.

@oliverkurth oliverkurth merged commit 89b6b7b into vmware:dev Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants