Skip to content

HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly #1398

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.

Sign up for GitHub

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

Closed
smengcl wants to merge 7 commits into from
Closed

HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly #1398

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5eaa1ef
HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM
smengcl Aug 30, 2019
5e59dc2
Make testWrongConfiguration and testMultipleOMServiceIds happy.
smengcl Sep 3, 2019
97ed7f4
Initialize this.peerNodes instead to eliminate OzoneManagerRatisServe…
smengcl Sep 4, 2019
c3a37cd
Clean up dead code. Remove assert.
smengcl Sep 4, 2019
fd122ac
Amend unit tests.
smengcl Sep 4, 2019
8b90760
Remove unused import.
smengcl Sep 4, 2019
fe2a9b1
Remove more unused imports.
smengcl Sep 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions ...egration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
import java.util.UUID;
import java.util.concurrent.TimeUnit;

import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_NODES_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY;

/**
* Tests OM related configurations.
*/
Expand Down Expand Up @@ -279,6 +283,61 @@ public void testWrongConfiguration() throws Exception {
}
}

/**
* Test a wrong configuration for OM HA. A configuration with an empty
* node list while service ID is configured should throw an error.
* @throws Exception
*/
@Test
public void testWrongConfigurationNoOMNodes() throws Exception {
String omServiceId = "om-service-test1";
conf.set(OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY, omServiceId);
// Deliberately skip OZONE_OM_NODES_KEY and OZONE_OM_ADDRESS_KEY config

try {
startCluster();
Assert.fail("OM initialization should have failed.");
} catch (OzoneIllegalArgumentException e) {
GenericTestUtils.assertExceptionContains(
"Incorrect configuration. Unable to find OzoneManager" +
" node address for service id " + omServiceId + ". Please" +
" check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY +
", " + OZONE_OM_NODES_KEY + " and " + OZONE_OM_ADDRESS_KEY, e);
}
}

/**
* Test a wrong configuration for OM HA. A configuration with no OM addresses
* while service ID is configured should throw an error.
* @throws Exception
*/
@Test
public void testWrongConfigurationNoOMAddrs() throws Exception {
String omServiceId = "om-service-test1";

String omNode1Id = "omNode1";
String omNode2Id = "omNode2";
String omNode3Id = "omNode3";
String omNodesKeyValue = omNode1Id + "," + omNode2Id + "," + omNode3Id;
String omNodesKey = OmUtils.addKeySuffixes(
OMConfigKeys.OZONE_OM_NODES_KEY, omServiceId);

conf.set(OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY, omServiceId);
conf.set(omNodesKey, omNodesKeyValue);
// Deliberately skip OZONE_OM_ADDRESS_KEY config

try {
startCluster();
Assert.fail("OM initialization should have failed.");
} catch (OzoneIllegalArgumentException e) {
GenericTestUtils.assertExceptionContains(
"Incorrect configuration. Unable to find OzoneManager" +
" node address for service id " + omServiceId + ". Please" +
" check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY +
", " + OZONE_OM_NODES_KEY + " and " + OZONE_OM_ADDRESS_KEY, e);
}
}

/**
* Test multiple OM service configuration.
*/
Expand Down
7 changes: 7 additions & 0 deletions hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_KERBEROS_PRINCIPAL_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_METRICS_SAVE_INTERVAL;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_METRICS_SAVE_INTERVAL_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_NODES_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_NODE_ID_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_RATIS_PORT_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY;
Expand Down Expand Up @@ -614,6 +615,12 @@ private void loadOMHAConfigs(Configuration conf) {
" system with " + OZONE_OM_SERVICE_IDS_KEY + " and " +
OZONE_OM_ADDRESS_KEY;
throw new OzoneIllegalArgumentException(msg);
} else if (!isOMAddressSet && found == 0) {
Copy link
Contributor

@bharatviswa504 bharatviswa504 Sep 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We cannot add this condition here, because think of case
OZONE_OM_SERVICE_IDS_KEY = ns1,ns2
Because after one iteration of nameserviceID if we are not able to find any om node matching with nameservice, we should not throw illegal configuration. We should try out all name services and see if it is matching with any nameservice.

And also, we can eliminate iteration of for loop with OmUtils.emptyAsSingletonNull(omServiceIds)) and similar for nameServiceIds. As for HA this is a must required configuration which needs to be set.

Copy link
Contributor

@bharatviswa504 bharatviswa504 Sep 3, 2019

Choose a reason for hiding this comment

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

And also try out test cases with multiple name services set also. And for this test case, you can use ozone.om.node.id if needed.

Copy link
Contributor Author

@smengcl smengcl Sep 4, 2019

Choose a reason for hiding this comment

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

@bharatviswa504 Thanks for the review. You are right. Now I understand that it makes sense to walk through all configured service ids before declaring failure.

I should probably just put found == 0 check outside the serviceId loop.

String msg = "Incorrect configuration. Unable to find OzoneManager" +
" node address for service id " + serviceId + ". Please" +
" check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY +
", " + OZONE_OM_NODES_KEY + " and " + OZONE_OM_ADDRESS_KEY;
throw new OzoneIllegalArgumentException(msg);
}
}

Expand Down