[Storage] Smoke tests for Storage mounting #1445
Conversation
…iments into storage_mountsmoketest
# Conflicts: # sky/utils/schemas.py
| f'sky launch -y -c {name} examples/using_file_mounts.yaml', | ||
| f'sky logs {name} 1 --status', # Ensure the job succeeded. | ||
| ], | ||
| test_commands, | ||
| f'sky down -y {name}', | ||
| timeout=20 * 60, # 20 mins | ||
| ) | ||
| run_one_test(test) |
There was a problem hiding this comment.
Is there a way to clear the folders/files creased in storage_setup_commands?
There was a problem hiding this comment.
I added cleanup commands in d7afd8d, but that causes transient test failures because a cleanup at the end of a test might delete the files in use by another test. I tried to use pytest_sessionfinish hooks but that does not work with the parallelization we use in pytest-xdist - each process runs just one test and terminates, so the cleanup code is called n times instead of just once (see issue).
To avoid adding more complexity to our testing code, I decided to exclude cleanup for now (since these are tiny files anyway). lmk if you think we should still implement something
There was a problem hiding this comment.
Sounds good! It seems pretty difficult to implement cleanup. I have another idea if you have time; instead of creating a bunch of files and directories, can we somehow move these commands and use import tempfile to generate temporary files and directories on local in that case?
tests/test_smoke.py
Outdated
| yaml_str = pathlib.Path( | ||
| 'tests/test_yamls/test_storage_mounting.yaml').read_text() | ||
| storage_name = f'sky-test-{int(time.time())}' | ||
| yaml_str = yaml_str.replace('random_bucket_name', storage_name) |
There was a problem hiding this comment.
The design choice to replace random_bucket_name with another name stands out a bit. Running the yaml file standalone will def not work as random_bucket_name is most likely taken, so it probably better to make it a jinja file. Why can't we make test_storage_mounting.yaml a jinja file instead and fill in the placeholder variable?
There was a problem hiding this comment.
Done for test_storage_mounting! I didn't do jinja for test_spot_storage because it uses a YAML in the examples folder, and it might be confusing for users to see jinja templates in the examples.
| # Mounting private buckets in MOUNT mode | ||
| /mount_private_mount: | ||
| name: random_bucket_name | ||
| source: ~/tmp-workdir |
There was a problem hiding this comment.
Just curious, since the same source is specified twice to the same bucket, there is redundant uploading done? Should we allow for users to do this? (assert unique?)
There was a problem hiding this comment.
Yeah, it would we uploaded twice... we should do some optimizations for this in storage.
There was a problem hiding this comment.
Sg, ig lets keep it separate from this PR then
|
Ready for another look @michaelzhiluo! Tested
|
| f'sky launch -y -c {name} examples/using_file_mounts.yaml', | ||
| f'sky logs {name} 1 --status', # Ensure the job succeeded. | ||
| ], | ||
| test_commands, | ||
| f'sky down -y {name}', | ||
| timeout=20 * 60, # 20 mins | ||
| ) | ||
| run_one_test(test) |
There was a problem hiding this comment.
Sounds good! It seems pretty difficult to implement cleanup. I have another idea if you have time; instead of creating a bunch of files and directories, can we somehow move these commands and use import tempfile to generate temporary files and directories on local in that case?
| # Mounting private buckets in MOUNT mode | ||
| /mount_private_mount: | ||
| name: random_bucket_name | ||
| source: ~/tmp-workdir |
There was a problem hiding this comment.
Sg, ig lets keep it separate from this PR then
That's a great point @michaelzhiluo - I looked into it and unfortunately If everything else looks good, should we merge this? |
Our current smoke tests for Storage only test the
Storageclass. This PR adds smoke tests for the mounting functionality to make sure our mounting backends (goofys/s3fs/gcsfuse) are also functioning correctly.Tested:
pytest tests/test_smoke.py::test_storage_mounts tests/test_smoke.py::test_file_mounts