MIDRC-1292 Enable Nextflow tests in Kind CI + test range param + test PV ops - #609
MIDRC-1292 Enable Nextflow tests in Kind CI + test range param + test PV ops#609paulineribeyre wants to merge 26 commits into
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Integration TestsFailed to Prepare CI environment Please find the Github Action logs here |
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on KindTest summary after running integration tests
Test summary after rerunning failed integration tests
|
Integration Tests on KindTest summary after running integration tests
Test summary after rerunning failed integration tests
|
Integration Tests on KindTest summary after running integration tests
Test summary after rerunning failed integration tests
|
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on Kind
|
Integration Tests on Kind
|
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
2c8c894 to
f9f7151
Compare
Integration Tests on Kind
|
nss10
left a comment
There was a problem hiding this comment.
Some minor suggestions and questions. PR looks good overall.
| else None | ||
| ) | ||
| logger.info(f"S3 {action.upper()} response: {response}") | ||
| logger.debug(f"S3 {action.upper()} response: {response}") |
There was a problem hiding this comment.
Thanks for reducing the clutter in logs. :)
| user: str = "main_account", | ||
| expected_status=200, | ||
| range: str = "", | ||
| ): |
There was a problem hiding this comment.
Can we add a docstring with Args for this function? Rest of the params are self explanatory, but I don't think range is. And a docstring might be handy for future maintanence!
There was a problem hiding this comment.
Also, can you keep expected_status as the last argument for the function, to remain consistent with all the other functions?
| print(f"SKIP_BROKEN: {os.environ.get('SKIP_BROKEN')} => {SKIP_BROKEN}") | ||
|
|
||
|
|
||
| # Test sequential file writes |
There was a problem hiding this comment.
I understand this file is sent to the TES pods to be executed as part of the test, but could you restructure the script to resemble a test suite?
Could each operation be moved into its own function, with all of the functions invoked at the end of the file? If one function fails, the script should continue running the remaining tests while logging which test failed.
I realize the current behavior is essentially binary—all of these tests fail with the S3 CSI driver and pass with S3Files—but organizing the script this way would make it much easier to add new test cases that we want to run on the pods in the future. It would also make the tests more descriptive and ensure that a single failing test does not prevent the remaining tests from running.
There was a problem hiding this comment.
I think it's sufficiently self-explanatory and restructuring the script wouldn't help much
- If the script keeps running if something fails, the TES task will fail to find the output files and will error anyway.
- The ZIP test is dependent on the other 2 cases
- I doubt we'll add many more PV posix tests
- The assertions in the test itself would have to be restructured as well to keep checking other test cases even if a previous assertion fails. I don't think I should spend much more time on this PR if there is no great benefit
| Also a regression test for various persistent volumes issues caused by S3-CSI driver | ||
| limitations. Fixed by switching PVs to S3Files. | ||
| This is written as a single test to keep all the PV-related POSIX issues together, and to | ||
| run fewer TES tasks to speed up testing. |
| expected offset 18154 but got 0" (Victoria's PLP workflow) | ||
| Note: unable to reproduce PDF, FastQ and GZ issues at this time. | ||
| MIDRC-1278 GZ file input: it works when the input is `s3://{s3_path_prefix}/input.txt.gz`. | ||
| We may be able to reproduce the issue by using a presigned URL instead, but that case is |
There was a problem hiding this comment.
This is true. I only noticed the issue when the gz file is received through presigned url.
DM from a thread with Sairam
Can you try using the gz files by uploading it to your S3 work directory temporarily? Once the ticket is resolved, you would be able to work with pre-signed urls directly.
| - Incremental/append writes to files already flushed to S3 | ||
| Note: unable to reproduce issue at this time |
There was a problem hiding this comment.
This was happening while writing to log files. When testing victoria's workflow, the output log file only has the first few lines, but every append after that was silently ignored.
There was a problem hiding this comment.
Were you not seeing any issue when you tested this 👇 in the script
There was a problem hiding this comment.
I did. I couldn't reproduce it initially and I think this comment might be out of date
There was a problem hiding this comment.
Yeah, it is only reproduced while you are in a directory where the PV is mounted. It is possible that you might've tested in a non PV work dir.
For instance, the default workdir for this image quay.io/cdis/gen3-workflow:integration_tests_dicom_image is /utils, which is out of the PV boundary where sequential writes work just fine.
Integration Tests on KindTest summary after running integration tests
Test summary after rerunning failed integration tests
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on Kind
|
Link to JIRA ticket if there is one: https://ctds-planx.atlassian.net/browse/MIDRC-1292
Fixed in uc-cdis/gen3-workflow#169
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes