From 5eaa1efd06fd756cba0ed5241680c7fddc34eb1f Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Fri, 30 Aug 2019 15:56:24 -0700 Subject: [PATCH 1/7] HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly --- .../om/TestOzoneManagerConfiguration.java | 59 +++++++++++++++++++ .../apache/hadoop/ozone/om/OzoneManager.java | 7 +++ 2 files changed, 66 insertions(+) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java index 77f0dfc2305a0..323cb454393d4 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java @@ -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. */ @@ -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. */ diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 48b095cfdb274..800e6f92d7a8e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -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; @@ -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 (found == 0) { + 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); } } From 5e59dc2344d365443e33bcf835065b6d89fe377c Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Tue, 3 Sep 2019 14:04:40 -0700 Subject: [PATCH 2/7] Make testWrongConfiguration and testMultipleOMServiceIds happy. --- .../src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 800e6f92d7a8e..df36e3e751e90 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -615,7 +615,7 @@ private void loadOMHAConfigs(Configuration conf) { " system with " + OZONE_OM_SERVICE_IDS_KEY + " and " + OZONE_OM_ADDRESS_KEY; throw new OzoneIllegalArgumentException(msg); - } else if (found == 0) { + } else if (!isOMAddressSet && found == 0) { String msg = "Incorrect configuration. Unable to find OzoneManager" + " node address for service id " + serviceId + ". Please" + " check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY + From 97ed7f4c30ef7abc89c989e930b3cedabcb11c26 Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Wed, 4 Sep 2019 14:33:52 -0700 Subject: [PATCH 3/7] Initialize this.peerNodes instead to eliminate OzoneManagerRatisServer#newOMRatisServer NPE. --- .../apache/hadoop/ozone/om/OzoneManager.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index df36e3e751e90..cd97eb8b06cb1 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -615,20 +615,27 @@ 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) { - 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); } } - +/* + if (found == 0) { + String msg = "Incorrect configuration. Unable to perform" + + " self-discovery for current OzoneManager node. Please" + + " check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY + + ", " + OZONE_OM_NODES_KEY + " and " + OZONE_OM_ADDRESS_KEY; + throw new OzoneIllegalArgumentException(msg); + } +*/ if (!isOMAddressSet) { // No OM address is set. Fallback to default InetSocketAddress omAddress = OmUtils.getOmAddress(conf); int ratisPort = conf.getInt(OZONE_OM_RATIS_PORT_KEY, OZONE_OM_RATIS_PORT_DEFAULT); + // HDDS-2064: this.peerNodes would be null at this point because + // it is not initialized, we want to initialize it as an empty list + // to prevent NPE in OzoneManagerRatisServer#newOMRatisServer. + assert(this.peerNodes == null); + this.peerNodes = new ArrayList<>(); LOG.info("Configuration either no {} set. Falling back to the default " + "OM address {}", OZONE_OM_ADDRESS_KEY, omAddress); From c3a37cdf97e3bc00122f56afd1932a71254b58df Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Wed, 4 Sep 2019 14:34:41 -0700 Subject: [PATCH 4/7] Clean up dead code. Remove assert. --- .../java/org/apache/hadoop/ozone/om/OzoneManager.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index cd97eb8b06cb1..f7d15ff917fb9 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -617,15 +617,7 @@ private void loadOMHAConfigs(Configuration conf) { throw new OzoneIllegalArgumentException(msg); } } -/* - if (found == 0) { - String msg = "Incorrect configuration. Unable to perform" + - " self-discovery for current OzoneManager node. Please" + - " check and reconfigure: " + OZONE_OM_SERVICE_IDS_KEY + - ", " + OZONE_OM_NODES_KEY + " and " + OZONE_OM_ADDRESS_KEY; - throw new OzoneIllegalArgumentException(msg); - } -*/ + if (!isOMAddressSet) { // No OM address is set. Fallback to default InetSocketAddress omAddress = OmUtils.getOmAddress(conf); @@ -634,7 +626,6 @@ private void loadOMHAConfigs(Configuration conf) { // HDDS-2064: this.peerNodes would be null at this point because // it is not initialized, we want to initialize it as an empty list // to prevent NPE in OzoneManagerRatisServer#newOMRatisServer. - assert(this.peerNodes == null); this.peerNodes = new ArrayList<>(); LOG.info("Configuration either no {} set. Falling back to the default " + From fd122acd75c330b5b0399012ee4f121a44ceaee1 Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Wed, 4 Sep 2019 14:49:40 -0700 Subject: [PATCH 5/7] Amend unit tests. --- .../om/TestOzoneManagerConfiguration.java | 36 ++++++------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java index 323cb454393d4..f5b5d052b577e 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java @@ -284,35 +284,27 @@ 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. + * A configuration with an empty node list while service ID is configured + * Should successfully start with no NPEs. * @throws Exception */ @Test - public void testWrongConfigurationNoOMNodes() throws Exception { + public void testNoOMNodes() 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); - } + // Should successfully start with no NPEs + startCluster(); } /** - * Test a wrong configuration for OM HA. A configuration with no OM addresses - * while service ID is configured should throw an error. + * A configuration with no OM addresses while service ID is configured. + * Should successfully start with no NPEs. * @throws Exception */ @Test - public void testWrongConfigurationNoOMAddrs() throws Exception { + public void testNoOMAddrs() throws Exception { String omServiceId = "om-service-test1"; String omNode1Id = "omNode1"; @@ -326,16 +318,8 @@ public void testWrongConfigurationNoOMAddrs() throws Exception { 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); - } + // Should successfully start with no NPEs + startCluster(); } /** From 8b90760efaae3c0c07e6a7586aaeef177131db36 Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Wed, 4 Sep 2019 14:51:07 -0700 Subject: [PATCH 6/7] Remove unused import. --- .../src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index f7d15ff917fb9..f7d1f114343e2 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -205,7 +205,6 @@ 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; From fe2a9b1018eca47a3c7f065f3e52d8646b5e3b00 Mon Sep 17 00:00:00 2001 From: Siyao Meng Date: Thu, 5 Sep 2019 10:03:08 -0700 Subject: [PATCH 7/7] Remove more unused imports. --- .../apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java index f5b5d052b577e..f3aa83672ed9c 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerConfiguration.java @@ -42,10 +42,6 @@ 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. */