Add tests for data objects stuck in locked status, prevent nullptr dereference (4-2-stable) #2097
Conversation
|
Should we wait to review this? |
|
This can be reviewed now as it is nearly identical to #2094, just reserving the right to do some unannounced force-pushing :p |
|
Will review as soon as i get an opening. |
|
irods/externals#181 is required along with the current set of changes in order to build the plugin. Will start running tests soon. Marking as ready for review since I do not expect to make any more dramatic changes outside of review comments. |
|
When I tried to run tests for this, it seemed to be infinitely stuck and a lot of error messages were coming out of MinIO. Will need to investigate. Everything leading up to running the tests (including running the test hook) seems fine now. |
|
A couple of things happened, preventing the tests from passing here:
So... everything passes now, yay |
|
The only things that should be different at this point are the changes to the test hook to chown the keypair file (which is different because of python2) and the latest commit which addresses #2100 (which needs its commit message updated and/or @JustinKyleJames should make that change in a separate PR). So, it's still nearly identical save for those two things. |
There was a problem hiding this comment.
Changes look good. Will await outcome with @JustinKyleJames regarding commit ownership before approving this PR.
Commit looks good and I think I am fine with the ownership being as is. |
|
Thanks! Squashed and added coauthorship to that last commit. |
This bumps the libs3 dependency declared in CMakeLists so that packages can be built. Also fixes the build hook to properly install any custom externals packages supplied to the script.
Adds tests for itouch and istream into the S3 resource plugin. This tests to make sure that the open/write/close APIs are appropriately abstracted for dealing with different types of storage, as exposed through these two iCommands. These tests are minimal and likely will require expansion in the future.
Adds a test which makes the S3 secret key invalid and attempts an iput. This ensures that data objects do not get stuck in a locked status even when the S3 backend can no longer be reached by the S3 resource plugin. The same test was created for irepl and icp as well.
This checks the irods::error returned by the s3_file_write_operation function and returns early if something went wrong. If the program is allowed to continue after a failure in this function, it can lead to a nullptr dereference later on. This is a minimal change required to prevent a nullptr dereference from inside the S3 resource plugin (which causes the servicing agent to crash). There are other places that need to prevent this kind of situation from happening, but this is the one instance I know about.
Co-authored-by: Justin James <[email protected]>
|
Merge when ready. |
Addresses #2092
Addresses #2095
Addresses irods/irods#6154
Addresses irods/irods#6479
Addresses irods/irods#6880
Companion PR: irods/irods#6905
Companion PR: irods/externals#181
Cherry-picked from #2094
Cherry-picked from #2093
Leaving in draft for now because building the plugin doesn't quite work. Might need to do something in irods/externals first...