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

implement preserve.replicas when restaging files #234

Conversation

cookie33
Copy link
Contributor

Implement fix for: #233

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Yep, looks good. We (iRODS team) will discuss how we want to approach adding an automated test for this (as @trel described in #233 (comment)). In the meantime, feel free to take a swing at it yourself. ;)

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

I went ahead and wrote a test for this and may have discovered an issue with the fix. Please confirm that suggested fix works for your use case.

storage_tiering.cpp Outdated Show resolved Hide resolved
@cookie33
Copy link
Contributor Author

We have to think about it. It works as follows for a restage:

  • get replica for the resource where to stage from. fi: 7
$ ils -l
/igor/home/rods:
  rods              7 eudatPnfs      1931831 2023-12-11.13:44 & test_20231208_60.txt
  • get group name by replica number. But this might be an other number. 8 if a previous restage has been done and the file has been removed from the upper tier again.
$ imeta ls -d test_20231208_60.txt
AVUs defined for dataObj /igor/home/rods/test_20231208_60.txt:
attribute: irods::access_time
value: 1702298856
units:
----
attribute: irods::storage_tiering::group
value: eudat
units: 8

And in this stage it fails because the replica and group version do not match.
See if there is an other function to get the group.

@cookie33
Copy link
Contributor Author

Changed one query:

-            boost::format("SELECT META_DATA_ATTR_VALUE WHERE DATA_NAME = '%s' AND COLL_NAME = '%s' AND META_DATA_ATTR_NAME = '%s' AND META_DATA_ATTR_UNITS = '%s'")
+            boost::format("SELECT META_DATA_ATTR_VALUE WHERE DATA_NAME = '%s' AND COLL_NAME = '%s' AND META_DATA_ATTR_NAME = '%s' AND META_DATA_ATTR_UNITS >= '%s'")

This makes sure that the function get_group_name_by_replica_number will work even if the replica has a lower number than the unit in irods::storage_tiering::group

@alanking
Copy link
Contributor

I left a comment in #232 regarding the query update: #232 (comment)

@cookie33 cookie33 marked this pull request as draft December 13, 2023 07:07
@cookie33
Copy link
Contributor Author

I made it a draft pull request.
I will make some tests in bash and when that is OK I will convert it back to a real pull request again.
The test will be similar like the automated tests from the irods consortium itself.

@korydraughn
Copy link
Collaborator

packaging/test_plugin_unified_storage_tiering.py contains all of our tests. All tests are expected to be implemented using Python.

We can also translate your bash tests into python if that helps.

@cookie33
Copy link
Contributor Author

cookie33 commented Dec 13, 2023

I tried running the python tests on my system. But the first thing it does is stop the iRODS server :(

I will try to get it running in a similar fashion as the iRODS tests. This will make it easier to incorporate.
After that I will update the official test_plugin_unified_storage_tiering.py with my updates after testing as much as possible.

But it will take some time to get some proper tests together.
So please bear with me.

@korydraughn
Copy link
Collaborator

That's correct. The run_tests.py script restarts the server before launching the tests to make sure the environment is ready.

Tests which depend on the delay server (like this project) normally restart the server too.

We're happy to help you cross the finish line :-)

@korydraughn
Copy link
Collaborator

Forgot to mention that you can use the iRODS Development Environment to run the tests easier. See https://github.com/irods/irods_development_environment.

With that, you can use the plugin builder and plugin test runners to build and test.

@cookie33
Copy link
Contributor Author

cookie33 commented Dec 14, 2023

Updated test_plugin_unified_storage_tiering_test .py in the class TestStorageTieringPluginPreserveReplica

Renamed one test to test_put because it did not do a get.
Added 4 other tests where preserve_replicas is set to true.
With some tests it now also checks if the replication number is the one which is expected.

tested:

$ time python3 run_tests.py --run_specific_test test_plugin_unified_storage_tiering > /tmp/test.txt
irods.test.test_plugin_unified_storage_tiering_test.TestStorageTieringPluginPreserveReplica.test_put ... ok
irods.test.test_plugin_unified_storage_tiering_test.TestStorageTieringPluginPreserveReplica.test_put_and_get_01 ... ok
irods.test.test_plugin_unified_storage_tiering_test.TestStorageTieringPluginPreserveReplica.test_put_and_get_02 ... ok
irods.test.test_plugin_unified_storage_tiering_test.TestStorageTieringPluginPreserveReplica.test_put_and_get_03 ... ok
irods.test.test_plugin_unified_storage_tiering_test.TestStorageTieringPluginPreserveReplica.test_put_and_get_04 ... ok

----------------------------------------------------------------------
Ran 5 tests in 308.318s

OK

real    5m8.492s
user    0m4.896s
sys     0m1.739s

@cookie33 cookie33 marked this pull request as ready for review December 14, 2023 13:36
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Thanks for writing these tests! I think that the first test you wrote covers the ground and we probably don't need the other tests. I could be wrong, so please let me know if I misunderstood anything.

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved
storage_tiering.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Looking very good. Saw one trailing space that I'm guessing was not on purpose.

I think we are almost ready to squash this. @cookie33 - Are you comfortable squashing this down to 1 or 2 commits? I would suggest 1 commit for #232 and 1 commit for #233 with commit messages like this:

[#232] My super helpful commit title

And maybe a description with the what and why, if you're into it.

If you're busy, we can swizzle the commits (your authorship will be retained). Up to you!

storage_tiering.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Commits swizzled locally. Build succeeded. Running tests. Once these have passed, I'll put this in.

Great work, @cookie33!

@alanking alanking force-pushed the update_migrate_object_to_minimum_restage_tier_to_include_preserve_replicas branch from 2daa727 to 7fd7b7e Compare December 19, 2023 18:38
@alanking
Copy link
Contributor

Confirmed that tests are passing. Added #s. Merging.

@alanking alanking merged commit 511cba8 into irods:main Dec 19, 2023
1 check failed
@cookie33 cookie33 deleted the update_migrate_object_to_minimum_restage_tier_to_include_preserve_replicas branch December 20, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants