HADOOP-18542. Keep MSI tenant ID and client ID optional #4262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Hi,
Thanks for the PR. Requesting you to kindly check if the JIRA (https://issues.apache.org/jira/browse/HADOOP-17725) used on the PR is correct. Since, the JIRA is about improving error messages which has been taken by #3041.
Also, the PR is missing on the tests, requesting you to kindly add tests around the code-change and also share the test-results of tests run (as shared by sadikovi in #3041). How to run tests: https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/site/markdown/testing_azure.md (please check https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/site/markdown/testing_azure.md#generating-test-run-configurations-and-test-triggers-over-various-config-combinations).
Regards
|
Hi @pranavsaxena-microsoft, I've tried to follow this section of the documentation that you mentioned, but it's outdated:
I followed the rest of the documentation by copying
Then I ran So it doesn't run the tests, same for |
|
Hi @CLevasseur , the testing section requires you to have latest trunk commits into your branch (as the testing section has been updated for the trunk). Hence requesting you to kindly back-merge apache:trunk into your branch. Thanks. |
|
I have pulled the latest changes. It now fails when running the tests. Note that it also fails when I run the tests from the trunk branch of the Also, it looks like those tests create a lot of containers in the storage account, is there an easy way to clean those ? Tests Output (Both apache/hadoop:trunk and CLevasseur/hadoop:trunk give the same errors) NonHNS-SharedKey: And here are the detailed errors: Any idea why those tests are failing ? HNS is disabled on that storage account, so I can't run the configurations |
CLevasseur
changed the title
CLevasseur
changed the title
|
I have created this JIRA to track this bug: https://issues.apache.org/jira/browse/HADOOP-18542 |
|
ITestAzureBlobFileSystemCheckAccess.testCheckAccessForAccountWithoutNS looks related...what is the full stack? it may be a different exception came back from the azure call, or it actually worked. other ones are not expected. are you working with a hierarchical store here? as i think those tests are failures from that detail, not your change (essentially: etags aren't being preserved across rename). do the same test suite against trunk and see if the tests still fail, if so, create a new jira (or maybe there is one already). it shouldn't block this pr |
|
The exception for
No, HNS is disabled on Azure's side, and in the account settings as well:
The result is the same, here are the logs I'm getting: I'll create another JIRA to fix those |
|
Here is the JIRA to track those integration test failures: https://issues.apache.org/jira/browse/HADOOP-18545 |
|
Hi, ITestAbfsTerasort.test_110_teragen, ITestAbfsJobThroughManifestCommitter.test_0420_validateJob, ITestAbfsManifestCommitProtocol failures are expected as of now. ITestAzureBlobFileSystemCheckAccess.testCheckAccessForAccountWithoutNS failure is not expected. As your changes are around OAuth, you would need an HNS account for the testing. Thanks. |
|
Hi, sorry for the delay, it took me a while to set everything up. Unfortunately I am still getting issues when running the integration tests on the It looks like permission issues, although the roles look correct to me. I followed the doc asking for: The credentials of each of these 4 account was set in the configuration file. Here is the output of running the HNS-OAuth test suite, can you tell me if those failures are expected, and if not, what I might miss ? |
|
that test_120_terasort(org.apache.hadoop.fs.azurebfs.commit.ITestAbfsTerasort failure is happening because yarn wants the owner of a file as determined from getFileStatus to match the yarn user (here: the username of the person running the test) Look at IdentityTransformer and its configuration keys to see what you need to set for a mapping |
CLevasseur
force-pushed
the
HADOOP-17725-trunk
branch
4 times, most recently
from
8410223 to
50a67fe
Compare
|
I have had a look at IdentityTransformer (we're talking about those configuration keys, right ?)but I am not sure what those fields are for and how to set them, I have tried the following snippet but it failed with similar errors. <property>
<name>fs.azure.identity.transformer.domain.name</name>
<value>$superuser</value>
</property>
<property>
<name>fs.azure.identity.transformer.service.principal.substitution.list</name>
<value>*</value>
</property>I have added a unit-test to check that we can now instantiate an MsiTokenProvider without setting the client and tenant IDs in the configuration. The integration tests that are failing look unrelated to my changes (see test run in my previous comment). |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@pranavsaxena-microsoft thoughts? would you be able to download this PR and test locally? |
|
the test which is a definite regression is ...that is a test which came in with the mandatoryPassword code. you will have to edit CONFIG_KEYS to remove the keys which are now optional. as to the others, I don't think they are related, more that you are testing through a non-HNS a/c and too much of the test code has drifted to being HNS-only. push up a patch for the CONFIG_KEYS change and I will download and test; if it works for me I'll be happy |
|
I just reran the whole test suite on a fresh install, and this time everything passed, without changing the code 🤷 There might have been some build cache still using a broken version, it's kind of a mystery to me to be honest. Here is the test output, feel free to run it yourself: |
|
@steveloughran @saxenapranav can you have a look please ? |
saxenapranav
approved these changes
Feb 28, 2023
CLevasseur
force-pushed
the
HADOOP-17725-trunk
branch
from
6501eae to
d6b23cc
Compare
CLevasseur
force-pushed
the
HADOOP-17725-trunk
branch
4 times, most recently
from
7abf3df to
ae1f329
Compare
CLevasseur
force-pushed
the
HADOOP-17725-trunk
branch
from
ae1f329 to
a602b81
Compare
|
I've rebased the branch onto the latest trunk, you should be good to run the integration tests |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
:::: AGGREGATED TEST RESULT :::: ============================================================
|
Thanks @CLevasseur The failures reported are known and we have a PR already out that fixes those tests. I see there are some PR checks failing. Might be intermittent, you can make a small commit to re-trigger that. |
...tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAccountConfiguration.java
Outdated
Show resolved
Hide resolved
anujmodi2021
approved these changes
Jul 24, 2024
There was a problem hiding this comment.
+1
Added some nits that can be used to re-trigger Yetus checks on PR.
@steveloughran, post yetus checks, this is good to merge.
Thanks
|
🎊 +1 overall
This message was automatically generated. |
anujmodi2021
approved these changes
Jul 24, 2024
|
👋 Are there remaining blockers to merge this one ? |
|
merged; will get into 3.4.1 @CLevasseur when the next RC comes out, please validate it! |
steveloughran
pushed a commit
that referenced
this pull request
Aug 21, 2024
Contributed by Carl Levasseur
KeeProMise
pushed a commit
to KeeProMise/hadoop
that referenced
this pull request
Sep 9, 2024
Contributed by Carl Levasseur
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
Contributed by Carl Levasseur
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
This PR is the same as #3788 but rebased onto a more recent version of the trunk branch.
This make tenant and client ID optional when getting an Azure token from the Azure Metadata Service since they can be inferred from the VM the calls is made from.
For some reason the integration tests were failing in @virajjasani 's version (couldn't get a useful error message) but once I rebased onto
trunkit worked.I ran the integration tests from an azure VM so that Azure's metadata service can infer the tenant and client ID (which is why they are optional in
MsiTokenProvider)How was this patch tested?
I ran the integration tests:
Let me know if something's missing, this is the first PR I do in this repo.