-
Notifications
You must be signed in to change notification settings - Fork 509
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
HDDS-8450. Dedicated acceptance test suite for s3a #6458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these tests @adoroszlai
generate_report "acceptance" "${ALL_RESULT_DIR}" "${XUNIT_RESULT_DIR}" | ||
|
||
if [[ "${OZONE_ACCEPTANCE_SUITE:-}" != "s3a" ]]; then | ||
generate_report "acceptance" "${ALL_RESULT_DIR}" "${XUNIT_RESULT_DIR}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment or rename this method to clarify that this method only applies to tests run with Robot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalized s3a
-specific condition to check acceptance test type (robot
or maven
). We may be adding more Maven-based tests in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the overall job passes, there is still a failure because generate_report
is being called (not sure why this doesn't fail the s3a job):
Robot framework is not installed, the reports cannot be generated (sudo pip install robotframework).
...
Error: Process completed with exit code 1.
Looks like OZONE_ACCEPTANCE_TEST_TYPE
is not exported in acceptance.sh
and not visible in this subshell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, will this also be a problem if devs run test-all.sh
manually to run all the tests locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out the problem with OZONE_ACCEPTANCE_TEST_TYPE
.
will this also be a problem if devs run
test-all.sh
manually to run all the tests locally?
When running test-all.sh
directly, installing Robot is the developer's responsibility (it was already required before this change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, my question is more generally: what is the expected way to run all acceptance tests locally and manually? Ideally running test-all.sh
(or maybeacceptance.sh
) should kick off all the acceptance tests locally. After this change, running either of these will only run the robot tests, unless the user knows which variables to export to their shell before running.
Maybe that's ok for now and we can handle that in a follow up task, since there's probably not many people running all the tests this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the expected way to run all acceptance tests locally and manually?
...
After this change, running either of these will only run the robot tests
Actually, they will also try to run s3a
tests, but that part will fail, since Hadoop sources will be missing.
We can avoid that by further separating the two kinds of tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to handle this in a follow up task if you want. It works fine in CI and I don't think many people are running the tests this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly changed s3a-test.sh
to not fail, rather skip the test if HADOOP_AWS_DIR
is missing. This way current behavior is retained when running acceptance.sh
or test-all.sh
without any test filters.
… auth-keys.xml inline
@ivandika3 Could you please review, too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the S3A acceptance test integration. Mostly LGTM.
Not terribly familiar with S3A, but I left some comments.
pushd "${HADOOP_AWS_DIR}" | ||
|
||
# S3A contract tests are enabled by presence of `auth-keys.xml`. | ||
# https://hadoop.apache.org/docs/3.3.6/hadoop-aws/tools/hadoop-aws/testing.html#Setting_up_the_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I can't seem to access this link.
Do you mean r3.3.6 instead of 3.3.6? (i.e. https://hadoop.apache.org/docs/r3.3.6/hadoop-aws/tools/hadoop-aws/testing.html#Setting_up_the_tests)
<property> | ||
<name>fs.s3a.path.style.access</name> | ||
<value>true</value> | ||
</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to also test for virtual host style pattern to uncover some AWS Java SDK problems with it (e.g. HDDS-7035) since our S3G implementation is built on top of path style pattern assumption (i.e. S3G needs to use VirtualHostStyleFilter
to convert virtual host style -> path style).
This can be addressed in future patches if needed.
@adoroszlai @errose28 I have a question regarding S3A in general. In what case does using S3A protocol more beneficial rather than using the OFS protocol? My understanding is that S3A aims to provide HDFS semantic to non hadoop-compatible storage (like using S3Guard when AWS S3 was still eventually consistent), whereas Ozone already provides OFS as a direct implementation of HDFS filesystem. From the performance point of view, it seems using s3a adds more network overhead since the s3a client send request to S3G, and the S3G to OM whereas OFS will directly connect to OM. Edit: One advantage I can think of is that the S3A client (just like any other S3 client) does not need to configure for OM services (or multiple OM services if federation OMs are supported in the future). Thanks in advance. |
Another case may be if client already has jars required for S3A, but classpath cannot be changed to add Ozone FS jar. Not sure if that's a common situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the patch and the clarification. LGTM +1.
Thanks @errose28 for the review. Patch is updated, please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @adoroszlai. Looks like there is still one issue remaining and then this should be good to go.
Thanks @errose28, @ivandika3 for the review. |
(cherry picked from commit a523fd9)
(cherry picked from commit a523fd9)
What changes were proposed in this pull request?
Run some S3A contract tests as part of acceptance check. Tests are executed via Maven against Docker Compose-based Ozone cluster, using Hadoop 3.3.6 source. JUnit test results and output for failed tests are included in the artifact, similar to integration check.
Limitations:
only works in unsecure env. for nowhttps://issues.apache.org/jira/browse/HDDS-8450
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/8484286662
Also tried some failing tests locally to verify behavior of the check.