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

Add option to run CI on all kernel flavors, plus other improvements #417

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

brenns10
Copy link
Contributor

This branch adds three improvements related to testing different kernel flavors:

  1. Allows locally running tests against all kernel versions of a given flavor by python setup.py test -K -f flavor. This is just a convenience that I wanted.
  2. Adds a "test-all-kernel-flavors" option similar to the "test-all-python-versions" option. Its main difference is that we don't test all flavors on push to the main branch (whereas we do test all Python versions for every push to main). I think that makes sense, because pushes to main can be rather frequent, and testing each flavor triples the test burden (which is of course multiplicative with all the Python versions). The "test-all-kernel-flavors" option is active after each vmtest build, or when the label is applied to a PR, or for a manual run on the Github Actions UI.
  3. Finally, to avoid filling up the disk space with downloaded kernels, I was forced to add some Github Actions specific logic to the testing, in order to cleanup downloaded kernels. We delete kernels after we test them, and we also set a size limit on the queue used to communicate downloaded kernels back to the main test thread. This ensures that at a certain point, the download thread pauses until a message is removed from the queue, and thus we never have more than a finite limit of kernels on-disk. This is important because of course the download speed is frequently faster than running the tests themselves.

I tested part 3 locally and verified that when GITHUB_ACTIONS=true, my build/vmtest directory never grew beyond ~2.5 GiB for a standard -K test, and it was emptied of kernels by the end of the run. I didn't want to do the -K -F locally, but I'd assume the result is about the same. For a local -K test with GITHUB_ACTIONS=false, the build/vmtest directory reached 7.5 GiB quickly and never decreased.

Test runs in Github Actions:

  1. A test run which includes the changes of Fix test failures on SLOB / tiny #416: https://github.com/brenns10/drgn/actions/runs/9998854777
  2. A test run which does not include the changes of Fix test failures on SLOB / tiny #416 and thus is expected to fail: https://github.com/brenns10/drgn/actions/runs/9998964897

This makes it simple to run all tests for a specific flavor. That's
especially useful if submitting a pull request, where CI will test the
default flavor, but you want to double check some other flavor first.

Signed-off-by: Stephen Brennan <[email protected]>
Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you! One minor comment below.

# risk of filling up the limited disk space is Github Actions. Set a limit
# of no more than 5 files which can be downloaded ahead of time. This is a
# magic number with no testing, but should work.
maxsize = 5 if IN_GITHUB_ACTIONS else 0
Copy link
Owner

Choose a reason for hiding this comment

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

This would be cleaner as an argument passed to download_in_thread(). Other than that, this is a really clever solution.

Copy link
Contributor Author

@brenns10 brenns10 Jul 18, 2024

Choose a reason for hiding this comment

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

Thanks! Would you like vmtest/__main__.py to pass this parameter based on GITHUB_ACTIONS environment variable? There are two callers, but of course setup.py is the only one used in CI. But in case we switch over to vmtest/__main__.py we may want the logic in both places.
(edit: I'll leave vmtest/__main__.py alone for now)

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch, yeah, just copy the logic in both places. I have a dream of getting off setuptools entirely, and vmtest/__main__.py will replace this if that ever happens.

The Github Actions disk space can get filled up if we test all kernel
flavors. So, delete kernels after testing when running in Github
Actions. However, if the download speed is far greater than the speed of
tests, then there is also the possibility that the disk space will be
filled up by the download thread, before the tests will run and delete
the older kernels. So we also need to ensure the download thread can't
get too far ahead of the tests, which we implement by setting a max size
on the download queue.

Signed-off-by: Stephen Brennan <[email protected]>
@osandov osandov added the test-all-kernel-flavors Run tests on all kernel flavors label Jul 19, 2024
@osandov
Copy link
Owner

osandov commented Jul 19, 2024

I was hoping that adding the label to this PR would do a test run, but maybe since it's not in the main branch yet, it didn't. Oh well, this looks good!

@osandov osandov merged commit 46da1ca into osandov:main Jul 19, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-all-kernel-flavors Run tests on all kernel flavors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants