-
Notifications
You must be signed in to change notification settings - Fork 501
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-5094. [FSO] Fail OM startup when turn on prefix layout with old buckets #2151
Conversation
@rakeshadr One Idea here is to persist format layout in bucket metadata and use that. And also iterate the buckets(Even now we iterate buckets during startup) and set layout version of the buckets to older format layout during startup if the format is not set. (I think this approach has been discussed during initial meetings) |
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 have one comment the approach/solution taken to solve this issue.
Thanks @bharatviswa504 for bringing the point. I think, I haven't explain it clearly about the purpose of this patch. I will try to add more details here: Yes, I have persisted the FSO layout in The purpose of this patch is to validate and fail during OM startup till I support and implement all the cases incrementally(phase by phase), then will remove this check. This means that, now in phase-1 the new feature is allowed only in a fresh new cluster. Later, I agree to support this case in follow-up tasks, existing old key(s) should be visible in prefix based OM layout. |
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, the quick fix @rakeshadr.
-
This patch seems to disable the usage of this feature in any existing cluster, and it can be used only in new cluster. It seems to be a quite huge limitation and I would suggest to mention in at least in the doc: if somebody creates at least one bucket this value can not be changed anymore.
-
Can you please confirm if this check works well when the metadata is empty? I may be wrong (I didn't test) but I think if I create buckets with existing code (no metadata) and update to this branch and set the layout to 'simple' the cluster couldn't be started. Fix me if I am wrong.
-
This validation won't work if somebody enables only the prefix-layout without the fs path support. What is the expected behavior in this case? Do we need any validation to avoid such case?
@@ -1115,6 +1118,9 @@ public void start() throws IOException { | |||
getOMMetadataLayout(); | |||
|
|||
metadataManager.start(configuration); | |||
|
|||
validatesBucketLayoutMismatches(); |
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.
Can it be checked before starting the ratis/RPC servers? Seems to be more safe IMHO...
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.
Can it be checked before starting the ratis/RPC servers? Seems to be more safe IMHO...
Thanks @elek for the comment. IMHO to maintain the existing order of starting of all the services as it is and don't like to impact existing OM startup behavior due to this temporary validation logic, by default the feature is disabled and there is no impact to the existing users/clusters. Like I said the contributors(dev team) will be actively working to support older[simple] buckets in prefix layout and will remove this validation logic later.
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.
impact existing OM startup behavior due to this temporary validation logic
Thanks the answer @rakeshadr. Can you please help me to understand how can moving this line upper would affect OM start behavior?
As far as I understand this check is only applied when OzoneManagerRatisUtils.isBucketFSOptimized
is true. I just asked if this new (!) check would be more safe to do before the ratis initialization as additional actions may happen before this safety check stops the clusters.
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.
@elek Yes, its validating the bucket metadata only when OzoneManagerRatisUtils.isBucketFSOptimized
is true. IIUC, you are suggesting to do the modification like below in OzoneManager#start() function, right? .
I have tried this but my test fail as I could see validation logic reads bucketMetadata successfully only if I keep it after metadataManager.start(configuration);
function.
The validation logic reads bucket table data using iterator = metadataManager.getBucketTable().iterator();
,
getOMMetadataLayout();
validatesBucketLayoutMismatches();
// Start Ratis services
if (omRatisServer != null) {
omRatisServer.start();
}
metadataManager.start(configuration);
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.
@elek I hope you agree to move the metadataManager.start(configuration)
call upwards before the ratis server start ?
OzoneManager#start()
LOG.info(buildRpcServerStartMessage("OzoneManager RPC server",
omRpcAddress));
metadataManager.start(configuration);
getOMMetadataLayout();
validatesBucketLayoutMismatches();
// Start Ratis services
if (omRatisServer != null) {
omRatisServer.start();
}
startSecretManagerIfNecessary();
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.
@elek I've modified the startup steps in latest (2nd) commit -
- Validate Invalid Configuration(layout PREFIX and ozone.om.enable.filesystem.paths=false) at the beginning
- Moved
validatesBucketLayoutMismatches
before ratis server.
Hope this looks fine to you. Thanks!
Sure, I will update the wiki page and apache docs. "The feature is disabled by default and can be used only in new cluster".
Your previous question has the answer - "it can be used only in new cluster". A new cluster will not have any volumes. Simple answer is, during cluster startup the validation logic will check all the existing buckets, if any and then will fail the OM startup if there are any older buckets[simple layout].
Please refer isFSOptimizedBucket logic. Here it checks both the properties and FSO requires both to be set(ozone.om.metadata.layout=PREFIX and ozone.om.enable.filesystem.paths=true). |
Thanks @elek for the review comments. |
Thank You @rakeshadr for the clear explanation and offline discussion. I am fine with this, if we are planning to address them in furthere Jiras. |
Fix me if I am wrong, but the current check doesn't handle case when bucketMetadata is empty (old data). For example if I set layout prefix to + public static boolean isFSOptimizedBucket(
+ Map<String, String> bucketMetadata) {
+ // layout 'PREFIX' represents optimized FS path
+ boolean metadataLayoutEnabled =
+ org.apache.commons.lang3.StringUtils.equalsIgnoreCase(
+ OMConfigKeys.OZONE_OM_METADATA_LAYOUT_PREFIX,
+ bucketMetadata
+ .get(OMConfigKeys.OZONE_OM_METADATA_LAYOUT));
+
+ boolean fsEnabled =
+ Boolean.parseBoolean(bucketMetadata
+ .get(OMConfigKeys.OZONE_OM_ENABLE_FILESYSTEM_PATHS));
+
+ return metadataLayoutEnabled && fsEnabled;
+ }
Thanks, will check it. For your information: during my test I was able to create buckets with |
I can confirm that I can create buckets where PREFIX is enabled but
If it's a supported case we need to improve the validation of this patch. If it's not, we should open a a new issue for validation and deny this mis-configuration as it can cause problems. |
Thanks for the confirmation @elek with a test case. Instead of failing with an error, presently OM proceeds silently with default metadata layout by making isBucketFSOptimized flag to false. I agree to fail the startup and make it visible to everyone. I will include this check along with this OM start up validation patch. |
I'm adding test scenarios explicitly with config values to understand it better, please feel free to add if I missed anything. Thanks! Scenario-1) Created a bucket with older cluster. Here bucketMetadata is empty. Test Step-1) Stops OM server and updated configs ozone.om.metadata.layout=PREFIX and ozone.om.enable.filesystem.paths=true. Test Step-2) Starts OM, validate isFSOptimizedBucket and metadataLayoutEnabled will be false. Test Result: OM startup will fail. Scenario-2) Created a bucket with older cluster. Here bucketMetadata is empty. Test Step-1) Stops OM server and updated configs ozone.om.metadata.layout=SIMPLE and ozone.om.enable.filesystem.paths=true. Test Step-2) Starts OM Test Result: OM won't validate the bucketmetadata layout as the cluster level layout is SIMPLE. It will start successfully with prefix feature disabled. Please add you expectations, thanks @elek |
It might be better to throw an exception in case of misconfiguration. the
Even better: it would be great to use just the |
I think it's better to always validate, because we have this scenario, too: Scenarion-3) Created a bucket with the new cluster and layout=PREFIX. bucketMetadata is filled Test Step-1) Stops OM server and updated configs ozone.om.metadata.layout=SIMPLE and ozone.om.enable.filesystem.paths=true. Test Step-2) Starts OM Test Result: OM should validate the layouts and fail as SIMPLE and PREFIX couldn't be handled in the same system (today). |
BTW, can we test it without using |
Thanks @elek for the clear information, IMHO to throw exception in case of invalid configuration and fail OM startup. I've updated the PR with this behavior. |
Thanks @elek for pointing out this case. I have added this case in the latest commit. |
Thanks a lot @elek for the suggestion. Meantime, I've modified existing unit test case by covering the cases that we discussed in this PR but I am still using FYI, I'm also having future plans to create |
Hi @elek, I've updated the PR based on your comment. Can you please review it again, thanks! |
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, @rakeshadr the update. Validation logic looks good to me. 👍
I have a few -- very small -- nit comments (and I assume that some unit test changes supposed to be part of an integration test related patch not this one)
I have ran this unit test multiple times(20 times) locally in my env and I could see that each run took only <=5secs. Hope this make sense to you!
It seems you have a powerful machine. On CI server it's:
2021-04-26T03:50:12.2809634Z [INFO] Running org.apache.hadoop.ozone.om.TestOMStartupWithLayout
2021-04-26T03:50:45.3975735Z [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 33.113 s - in org.apache.hadoop.ozone.om.TestOMStartupWithLayout
But I think moving the check to the beginning is helped, and I didn't consider that in most of the cases the MiniOzoneCluster
is not required to be started (as it should be failing) So let's keep it as is for now...
I'm also having future plans to create MiniOMCluster by mocking other services except OM, especially for the PREFIX/SIMPLE based integration-tests
👍 It sounds like a great idea. I like it.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
...egration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java
Outdated
Show resolved
Hide resolved
...ne/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemWithFSO.java
Outdated
Show resolved
Hide resolved
...ntegration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileInterfacesWithFSO.java
Outdated
Show resolved
Hide resolved
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 again @elek for the review comments. Please find my reply and I will update the patch soon.
…ne/om/OzoneManager.java Co-authored-by: Elek, Márton <elek@users.noreply.github.com>
Marked this as addressed based on @bharatviswa504's reply - comment link here |
Marked this as addressed based on @bharatviswa504's reply - #2151 (comment)
@elek I've addressed your comments. Can you please review it again, thanks! |
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.
+1, thanks, (the continuous) update and patience.
Thanks a lot @elek, @bharatviswa504, @mukul1987 for the detailed reviews. After several CI runs, finally got a clean build report. I'm merging it to the branch. |
What changes were proposed in this pull request?
Start an existing OM using new PREFIX configuration layout format, which(OM) already contains old buckets.
Scenario:
Start cluster with configs,
OZONE-SITE.XML_ozone.om.enable.filesystem.paths=true
OZONE-SITE.XML_ozone.om.metadata.layout=simple
create /vol1/bucket1/dir1/dir2/key1
Stop OM
Update config OZONE-SITE.XML_ozone.om.metadata.layout=prefix
Start OM.
Output: Failed to start OM as there is a bucket with old layout format.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5094
How was this patch tested?
Added unit test case.