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

Build/test hook improvements, fixes for test suite, and cherry-picked 4-2-stable changes (main) #190

Merged
merged 10 commits into from
Apr 18, 2022

Conversation

alanking
Copy link
Contributor

These tests do NOT pass presently. I suspect there is a bug in the delay server as the tests do pass in 4-2-stable. If you diff this against 4-2-stable, you will see that the only difference of any possible consequence is the fetching of configuration values, and I don't think that is related to the issues I am seeing. I could be wrong, of course. :) Leaving as draft until we can figure it out.

@alanking
Copy link
Contributor Author

Okay, we have confirmed that the issue is in the delay server and I am working on a fix for this now: irods/irods#6323

I'd still like to see the tests pass before I merge this but I will mark as ready for review.

@alanking alanking marked this pull request as ready for review April 13, 2022 18:50
Copy link
Member

@trel trel left a comment

Choose a reason for hiding this comment

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

test mode is what sets up the rule engine plugins?

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

Are you referring to test_mode=True in the restart calls? This is restarting the server in test mode which is a special mode in 4.3.0 to handle the differences in logging between the past and the future. Any test relying on log messages needs to use test mode when starting the server. I think we always restart the server in test mode in the main repo for the test suite.

@trel
Copy link
Member

trel commented Apr 13, 2022

okay, so then... where did the extra REPs get to? just big minus?

@alanking
Copy link
Contributor Author

The extra REPs are only applicable in policy-composed storage tiering. The unified storage tiering REP contains all of these rule engine plugins in the same shared object so there is no need to install or configure them separately. Many of the diffs from the test file are a result of literally copying the file from 4-2-stable and pasting into main.

@trel
Copy link
Member

trel commented Apr 13, 2022

ooooh, right.

@alanking alanking marked this pull request as draft April 14, 2022 16:58
@alanking alanking force-pushed the 6020.m branch 2 times, most recently from c78978a to 7001f7f Compare April 18, 2022 15:42
@alanking
Copy link
Contributor Author

I created #193 to address my remaining concerns, but this set of changes has produced success in getting the tests to pass in conjunction with irods/irods#6325.

@alanking alanking marked this pull request as ready for review April 18, 2022 15:50
@trel
Copy link
Member

trel commented Apr 18, 2022

these look good to me.

@alanking
Copy link
Contributor Author

alanking commented Apr 18, 2022

Should we go ahead and get this in, then? I'll ping @korydraughn in case he has thoughts.

@trel
Copy link
Member

trel commented Apr 18, 2022

I vote green, #, and push em.

@korydraughn
Copy link
Collaborator

Same here. If it's green, let's go on and merge it.

- Ported test suite from 4-2-stable which tests unified plugin
- Fix local import syntax
- Add test_mode=True to restart calls
- Change delay server sleep time setting to new key
Due to some changes in how scheduling data migrations works, the unified
storage tiering REP needed to be updated in order to work. The following
changes were made:

 - "policy" was changed to "policy_to_invoke" as expected in the cpp
default REP implementation of irods_policy_enqueue_rule
 - "payload" was changed to "parameters" as expected in the cpp default
REP implementation of irods_policy_enqueue_rule
 - "delay_conditions" was moved inside of the "parameters" JSON struct
as expected in the cpp default REP implementation of
irods_policy_enqueue_rule

This causes everything to align as expected in the new world of
policy-composed rule invocation.
The DOUBLE execution frequency directive is known to have issues. As
this is an example invocation, we should not be using the DOUBLE
directive in the execution frequency hint. Additionally, as this is only
going to be used in testing and demonstrations/training, the amount of
time between retries has been reduced from 1 hour to 60 seconds.
This wraps all of the tests with try-except blocks in order to guarantee
that the artifacts generated by the tests get cleaned up even in the
event of failure.

Also adds a function called invoke_storage_tiering_rule() to capture the
example rule invocation used in the tests. This will insulate the tests
from changes to how the test storage tiering invocation might work in
the future.
When testing the object limit parameter with a value set to a number
greater than the maximum number of SQL rows, the test asserts that the
very last object does not tier out. This implicitly relies on the order
of the rule IDs coming out of the database. The test should instead
simply assert that any one of the object remains in the lower tier.
@alanking
Copy link
Contributor Author

Okey dokey. Thanks

@alanking alanking merged commit 879c31c into irods:main Apr 18, 2022
@alanking alanking deleted the 6020.m branch April 18, 2022 19:26
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.

3 participants