errose28 commented on code in PR #4138:
URL: https://github.com/apache/ozone/pull/4138#discussion_r1063822677
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMStorage.java:
##########
@@ -32,52 +32,178 @@
import static
org.apache.hadoop.ozone.om.OmUpgradeConfig.ConfigStrings.OZONE_OM_INIT_DEFAULT_LAYOUT_VERSION;
/**
- * OMStorage is responsible for management of the StorageDirectories used by
- * the Ozone Manager.
+ * Ozone Manager VERSION file representation.
+ * On top of what is defined in the base Storage class, this class adds
+ * functionality to hold Ozone Manager related data in its VERSION file.
+ * The additional values stored:
+ * - Ozone Manager ID - a UUID that identifies this Ozone Manager.
+ * The value can not be changed once initialized.
+ * - Ozone Manager Node Id - the node id defined for this Ozone manager in the
+ * configuration. The value can not be changed after
+ * it was set.
Review Comment:
I think OM ID was used for non-ratis OMs, but all modern installations using
OM HA would be using only Node ID. Might want to note that here if it is still
true.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -459,6 +459,7 @@ private OzoneManager(OzoneConfiguration conf, StartupOption
startupOption)
this.omNodeDetails = omhaNodeDetails.getLocalNodeDetails();
omStorage = new OMStorage(conf);
+ omStorage.validateOrPersistOmNodeId(omNodeDetails.getNodeId());
Review Comment:
A comment here would be good explaining that we are persisting the node ID
if it is not already there in the constructor to prevent existing clusters from
having to re-init to have this persisted.
Side note: Do we want to do this for new fields added to the version file,
or should we only write them in init and be ok with existing clusters not
having them? Continuing this pattern would cause version file updates to be
split between the init and constructor steps forever.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMStorage.java:
##########
@@ -32,52 +32,178 @@
import static
org.apache.hadoop.ozone.om.OmUpgradeConfig.ConfigStrings.OZONE_OM_INIT_DEFAULT_LAYOUT_VERSION;
/**
- * OMStorage is responsible for management of the StorageDirectories used by
- * the Ozone Manager.
+ * Ozone Manager VERSION file representation.
+ * On top of what is defined in the base Storage class, this class adds
+ * functionality to hold Ozone Manager related data in its VERSION file.
+ * The additional values stored:
+ * - Ozone Manager ID - a UUID that identifies this Ozone Manager.
+ * The value can not be changed once initialized.
+ * - Ozone Manager Node Id - the node id defined for this Ozone manager in the
+ * configuration. The value can not be changed after
+ * it was set.
+ * - Ozone Manager Certificate Serial Id - the serial id of the Ozone Manager's
+ * SSL certificate if one exists.
*/
public class OMStorage extends Storage {
- public static final String STORAGE_DIR = "om";
- public static final String OM_ID = "omUuid";
- public static final String OM_CERT_SERIAL_ID = "omCertSerialId";
+ static final String ERROR_OM_IS_ALREADY_INITIALIZED =
+ "OM is already initialized.";
+ static final String ERROR_UNEXPECTED_OM_NODE_ID_TEMPLATE =
+ "OM NodeId: {} does not match existing nodeId from VERSION file: {}";
Review Comment:
This is being passed to `String#format`, which uses `%` as the encoding, so
the `{}` instances are not replaced. Intellij has a warning for this.
`testValidateOrPersistOmNodeIdThrowsWithDifferentNodeIdAsInFile` makes the same
mistake, and only checks for substring matches, so it passes.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOMStorage.java:
##########
@@ -17,76 +17,312 @@
package org.apache.hadoop.ozone.om;
import java.io.File;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.UUID;
import org.apache.hadoop.hdds.HddsConfigKeys;
-import org.apache.hadoop.hdds.conf.MutableConfigurationSource;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
-import org.apache.ozone.test.GenericTestUtils;
-
-import org.apache.commons.io.FileUtils;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import org.apache.hadoop.ozone.om.upgrade.OMLayoutVersionManager;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.hadoop.ozone.common.Storage.StorageState.INITIALIZED;
+import static
org.apache.hadoop.ozone.om.OMStorage.ERROR_OM_IS_ALREADY_INITIALIZED;
+import static
org.apache.hadoop.ozone.om.OMStorage.ERROR_UNEXPECTED_OM_NODE_ID_TEMPLATE;
+import static org.apache.hadoop.ozone.om.OMStorage.OM_CERT_SERIAL_ID;
+import static org.apache.hadoop.ozone.om.OMStorage.OM_ID;
+import static org.apache.hadoop.ozone.om.OMStorage.OM_NODE_ID;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.fail;
/**
* Testing OMStorage class.
+ * Assumptions tested:
+ * 1. certificate serial ID can be set and unset anytime.
+ * 2. OmId the UUID of the Ozone Manager can be set only when the OMStorage
+ * is not initialized already. Once initialized, setting OmId throws
+ * IOException
+ * 3. OmNodeId:
+ * 3.1. can be set when the storage is not initialized, once initialize,
+ * setting OmNodeId throws IOException
+ * 3.2. verifying the OmNodeId is possible once the storage is initialized,
+ * until it is not initialized, verification throws IOException
+ * 3.3. verifying the OmNodeId does not do anything if the provided value
is
+ * equal to the stored value, throws an IOException otherwise
+ * 4. Configuration parsing:
+ * 4.1. getOmDbDir returns the configured
+ * {@link OMConfigKeys#OZONE_OM_DB_DIRS} value
+ * 4.2. getOmDbDir falls back to {@link HddsConfigKeys#OZONE_METADATA_DIRS}
+ * when {@link OMConfigKeys#OZONE_OM_DB_DIRS} is not set
+ * 4.3. getOmDbDir throws exception if none of the above properties are set
+ * 5. the protected getNodeProperties method properly returns all the keys
+ * that are set properly in the OMStorage object.
*/
public class TestOMStorage {
@Rule
public ExpectedException thrown = ExpectedException.none();
- /**
- * Test {@link OMStorage#getOmDbDir}.
- */
+ @Rule
+ public TemporaryFolder tmpFolder = new TemporaryFolder();
+
+ private static final String OM_ID_STR = new UUID(1L, 1L).toString();
+
@Test
- public void testGetOmDbDir() {
- final File testDir = createTestDir();
+ public void testGetOmDbDir() throws Exception {
+ final File testDir = tmpFolder.newFolder();
final File dbDir = new File(testDir, "omDbDir");
- final File metaDir = new File(testDir, "metaDir"); // should be ignored.
- final MutableConfigurationSource conf = new OzoneConfiguration();
- conf.set(OMConfigKeys.OZONE_OM_DB_DIRS, dbDir.getPath());
- conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, metaDir.getPath());
+ final File metaDir = new File(testDir, "metaDir");
+ OzoneConfiguration conf = confWithHDDSMetaAndOMDBDir(metaDir, dbDir);
+ assertThat(dbDir, equalTo(OMStorage.getOmDbDir(conf)));
+ assertThat(dbDir.exists(), is(true));
+ assertThat(metaDir.exists(), is(false));
+ }
+
+ @Test
+ public void testGetOmDbDirWithFallback() throws Exception {
+ File metaDir = tmpFolder.newFolder();
+ OzoneConfiguration conf = confWithHDDSMetadataDir(metaDir);
+
+ assertThat(metaDir, equalTo(OMStorage.getOmDbDir(conf)));
+ assertThat(metaDir.exists(), is(true));
+ }
+
+ @Test
+ public void testNoOmDbDirConfigured() {
+ thrown.expect(IllegalArgumentException.class);
+ OMStorage.getOmDbDir(new OzoneConfiguration());
+ }
+
+ @Test
+ public void testSetOmIdOnNotInitializedStorage() throws Exception {
+ OMStorage storage = new OMStorage(configWithOMDBDir());
+ assertThat(storage.getState(), is(not(INITIALIZED)));
+
+ String omId = "omId";
try {
- assertEquals(dbDir, OMStorage.getOmDbDir(conf));
- assertTrue(dbDir.exists()); // should have been created.
- } finally {
- FileUtils.deleteQuietly(dbDir);
+ storage.setOmId(omId);
+ } catch (IOException e) {
+ fail("Can not set OmId on a Storage that is not initialized.");
}
+ assertThat(storage.getOmId(), is(omId));
+ assertGetNodeProperties(storage, omId);
}
- /**
- * Test {@link OMStorage#getOmDbDir} with fallback to OZONE_METADATA_DIRS
- * when OZONE_OM_DB_DIRS is undefined.
- */
@Test
- public void testGetOmDbDirWithFallback() {
- final File testDir = createTestDir();
- final File metaDir = new File(testDir, "metaDir");
- final MutableConfigurationSource conf = new OzoneConfiguration();
- conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, metaDir.getPath());
+ public void testSetOmIdOnInitializedStorage() throws Exception {
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFile(conf);
+ thrown.expect(IOException.class);
+ thrown.expectMessage(ERROR_OM_IS_ALREADY_INITIALIZED);
+ OMStorage storage = new OMStorage(conf);
+ storage.setOmId("omId");
+ }
+
+ @Test
+ public void testCertSerialIdOperations() throws Exception {
+ OzoneConfiguration conf = configWithOMDBDir();
+ OMStorage storage = new OMStorage(conf);
+
+ assertThat(storage.getState(), is(not(INITIALIZED)));
+ assertCertOps(storage);
+ storage.initialize();
+ storage.persistCurrentState();
+
+ storage = new OMStorage(conf);
+ assertThat(storage.getState(), is(INITIALIZED));
+ assertCertOps(storage);
+ }
+
+ @Test
+ public void testSetOmNodeIdOnNotInitializedStorage() throws Exception {
+ OMStorage storage = new OMStorage(configWithOMDBDir());
+ assertThat(storage.getState(), is(not(INITIALIZED)));
+
+ String nodeId = "nodeId";
try {
- assertEquals(metaDir, OMStorage.getOmDbDir(conf));
- assertTrue(metaDir.exists()); // should have been created.
- } finally {
- FileUtils.deleteQuietly(metaDir);
+ storage.setOmNodeId(nodeId);
+ } catch (IOException e) {
+ fail("Can not set OmNodeId on a Storage that is not initialized.");
}
+ assertThat(storage.getOmNodeId(), is(nodeId));
+ assertGetNodeProperties(storage, null, nodeId);
}
@Test
- public void testNoOmDbDirConfigured() {
- thrown.expect(IllegalArgumentException.class);
- OMStorage.getOmDbDir(new OzoneConfiguration());
+ public void testSetOMNodeIdOnInitializedStorageWithoutNodeID()
+ throws Exception {
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFile(conf);
+ thrown.expect(IOException.class);
+ thrown.expectMessage(ERROR_OM_IS_ALREADY_INITIALIZED);
+
+ OMStorage storage = new OMStorage(conf);
+ storage.setOmNodeId("nodeId");
}
- public File createTestDir() {
- File dir = new File(GenericTestUtils.getRandomizedTestDir(),
- TestOMStorage.class.getSimpleName());
- dir.mkdirs();
- return dir;
+ @Test
+ public void testSetOMNodeIdOnInitializedStorageWithNodeID() throws Exception
{
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFileWithNodeId(conf, "nodeId");
+ thrown.expect(IOException.class);
+ thrown.expectMessage(ERROR_OM_IS_ALREADY_INITIALIZED);
+
+ OMStorage storage = new OMStorage(conf);
+ storage.setOmNodeId("nodeId");
}
+
+ @Test
+ public void testValidateOrPersistOmNodeIdPersistsNewlySetValue()
+ throws Exception {
+ String nodeId = "nodeId";
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFile(conf);
+
+ OMStorage storage = new OMStorage(conf);
+ assertThat(storage.getState(), is(INITIALIZED));
+ assertThat(storage.getOmNodeId(), is(nullValue()));
+
+ storage.validateOrPersistOmNodeId(nodeId);
+ assertThat(storage.getOmNodeId(), is(nodeId));
+ assertGetNodeProperties(storage, OM_ID_STR, nodeId);
+
+ storage = new OMStorage(conf);
+ assertThat(storage.getOmNodeId(), is(nodeId));
+ assertGetNodeProperties(storage, OM_ID_STR, nodeId);
+ }
+
+ @Test
+ public void testValidateOrPersistOmNodeIdDoesRunWithSameNodeIdAsInFile()
+ throws Exception {
+ String nodeId = "nodeId";
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFileWithNodeId(conf, nodeId);
+
+ OMStorage storage = new OMStorage(conf);
+ assertThat(storage.getState(), is(INITIALIZED));
+ assertThat(storage.getOmNodeId(), is(nodeId));
+ assertGetNodeProperties(storage, OM_ID_STR, nodeId);
+
+ storage.validateOrPersistOmNodeId(nodeId);
+
+ assertThat(storage.getOmNodeId(), is(nodeId));
+ assertGetNodeProperties(storage, OM_ID_STR, nodeId);
+ }
+
+ @Test
+ public void testValidateOrPersistOmNodeIdThrowsWithDifferentNodeIdAsInFile()
+ throws Exception {
+ String nodeId = "nodeId";
+ String newId = "newId";
+ OzoneConfiguration conf = configWithOMDBDir();
+ setupAPersistedVersionFileWithNodeId(conf, nodeId);
+
+ OMStorage storage = new OMStorage(conf);
+ assertThat(storage.getState(), is(INITIALIZED));
+ assertThat(storage.getOmNodeId(), is(nodeId));
+
+ thrown.expect(IOException.class);
+ String expectedMsg =
+ String.format(ERROR_UNEXPECTED_OM_NODE_ID_TEMPLATE, nodeId, newId);
+ thrown.expectMessage(expectedMsg);
+
+ storage.validateOrPersistOmNodeId(newId);
+ }
+
+ private void assertCertOps(OMStorage storage) throws IOException {
+ String certSerialId = "12345";
+ String certSerialId2 = "54321";
+ storage.setOmCertSerialId(certSerialId);
+ assertThat(storage.getOmCertSerialId(), is(certSerialId));
+ assertGetNodeProperties(storage, null, null, certSerialId);
+
+ storage.setOmCertSerialId(certSerialId2);
+ assertThat(storage.getOmCertSerialId(), is(certSerialId2));
+ assertGetNodeProperties(storage, null, null, certSerialId2);
+
+ storage.unsetOmCertSerialId();
+ assertThat(storage.getOmCertSerialId(), is(nullValue()));
+ assertGetNodeProperties(storage, null, null, null);
+ }
+
+ private void assertGetNodeProperties(OMStorage storage, String... values) {
+ Properties p = storage.getNodeProperties();
+ Map<String, String> e = toExpectedPropertyMapping(values);
+
+ if (e.get(OM_ID) != null) {
+ assertThat(p.getProperty(OM_ID), is(e.get(OM_ID)));
+ }
+ if (e.get(OM_NODE_ID) != null) {
+ assertThat(p.get(OM_NODE_ID), is(e.get(OM_NODE_ID)));
+ }
+ if (e.get(OM_CERT_SERIAL_ID) != null) {
+ assertThat(p.get(OM_CERT_SERIAL_ID), is(e.get(OM_CERT_SERIAL_ID)));
+ }
+ }
+
+ private Map<String, String> toExpectedPropertyMapping(String... values) {
+ Map<String, String> ret = new HashMap<>();
+ String[] propNames = new String[]{OM_ID, OM_NODE_ID, OM_CERT_SERIAL_ID};
+ for (int i = 0; i < values.length; i++) {
+ ret.put(propNames[i], values[i]);
+ }
+ return ret;
+ }
+
+ private OMStorage setupAPersistedVersionFile(OzoneConfiguration conf)
Review Comment:
Looks like the return of this method is never used. Should it be removed or
is there a use case for the return value that got missed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
