errose28 commented on code in PR #11800:
URL: https://github.com/apache/ozone/pull/11800#discussion_r4461591136
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/upgrade/TestScmStartupInvalidApparentVersion.java:
##########
Review Comment:
We should rename this class since it now testing valid DB version cases.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/upgrade/TestScmStartupInvalidApparentVersion.java:
##########
@@ -59,8 +68,97 @@ public void
testStartupFailsWhenApparentVersionBeyondLastKnownComponentVersion()
assertStartupFailsWithComponentVersionMessage(SOFTWARE_VERSION.serialize()
+ 1);
}
+ /**
+ * An SCM that stops after replacing its DB with a finalized checkpoint, but
before updating its VERSION file, cannot
+ * replay the finalization from the Raft log, so it should finalize up to
the version in its DB on restart.
+ */
+ @Test
+ public void testStartupFinalizesToApparentVersionInDB() throws Exception {
+ OzoneConfiguration conf = createScmStorage(INITIAL_VERSION.serialize());
+ StorageContainerManager scm = HddsTestUtils.getScm(conf, new
SCMConfigurator());
+ try {
+ // Without a version in the DB, startup leaves the SCM pre-finalized.
+
assertThat(scm.getScmMetadataStore().getMetaTable().get(APPARENT_VERSION_KEY)).isNull();
+
assertThat(scm.getVersionManager().getApparentVersion()).isEqualTo(INITIAL_VERSION);
+
+ // The DB is ahead of the VERSION file, as the interrupted install
leaves it. The SCM finalizes to the
+ // software version
+ scm = restartWithDBVersion(scm, SOFTWARE_VERSION);
+ // finalizeFromDBIfRequired is called by scm.start(), but calling that
here makes the tests slow and we would
+ // need more setup for safemode etc. As this test is unit testing
finalizeFromDBIfRequired we call it directly
+ // to keep the tests simple.
+
scm.getVersionManager().finalizeFromDBIfRequired(scm.getScmMetadataStore().getMetaTable());
+ assertVersions(scm, SOFTWARE_VERSION, SOFTWARE_VERSION);
+ assertThat(scm.getVersionManager().needsFinalization()).isFalse();
+ } finally {
+ scm.stop();
+ scm.join();
+ }
+ }
+
+ /**
+ * The VERSION file is ahead of the DB. This can happen if finalization
wrote the VERSION file (inside Ratis
+ * applyTransaction) but the process crashed before the Ratis transaction
buffer was flushed to RocksDB.
+ * Ratis log replay should correct this, but finalizeFromSnapshotIfRequired
also updates the DB as a safety net.
Review Comment:
This and line 114 have a stale method name in the comment.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -1536,6 +1536,11 @@ public CertificateStore getCertificateStore() {
*/
@Override
public void start() throws IOException {
+ // If the SCM stopped during a snapshot install after the DB was replaced,
the VERSION file is behind the DB, and
+ // the finalization included in the snapshot is not in the Raft log to be
replayed.
+ // The metadata store may have been closed by a prior stop(); start() is
idempotent and reopens it if needed.
+ scmMetadataStore.start(configuration);
Review Comment:
`TestStorageContainerManagerHA` has a bug. We should not add a workaround
for this in prod code. This is Claude's alternative to review:
---
The test now passes with all three SCMs actually started, and with the
DB-reopen line removed from `StorageContainerManager.start()`. It takes about 5
s longer, because the old version only passed with one SCM silently failing to
start.
| Version | Runs (test time)
|
| ------------------------------------- | ----------------------------------
|
| Before (current PR branch) | 18.70 s, 18.61 s, 18.53 s, 18.72 s
|
| After (test fix, reopen line removed) | 24.16 s, 24.08 s, 23.93 s, 24.09 s
|
**Why the old test was faster:** it only ever brought up two of the three
SCMs.
- The cluster's SCM list comes from a `HashMap`, so `get(0)` is always
scmNode-2. That SCM had never been started; the running one is scmNode-1.
- The old test stopped scmNode-2, called `start()` again on the
still-running scmNode-1, and started scmNode-3.
- Starting scmNode-2 again threw `IllegalStateException: CLOSED -> STARTING`
from its already-closed Ratis server. The test only catches `IOException`, so
that thread died unnoticed and the test passed.
- In the fixed run, scmNode-2 and scmNode-3 join the leader at the same
time. The second join waits about 5 s behind the first Ratis reconfiguration.
**The fix (test only, plus removing the reopen line):**
- It leaves the running SCM alone and starts only the SCMs that were never
started, all at once. It counts any exception as a failure and asserts there
were none.
- It drops the retry loop. That loop restarted stopped instances, which
can't work. Replacing them with new instances doesn't work in one JVM either:
an SCM's RPC ports are bound in its constructor, and stopping an SCM that never
started doesn't release them.
**Two other ways I tried that didn't work:**
- **Replace every SCM with a new instance:** this hit `BindException`,
because of those unreleased ports.
- **Replace only the running SCM with a new instance, starting all three
together:** this hung until the 5-minute timeout. SCMs 2 and 3 each block in
`addSCM` waiting on the other, whose port is bound but not yet serving. I only
saw this in the test. In production the gap between creating an SCM and
starting it is much shorter, but the same hang might be possible.
A separate flaw I left alone: the leader-count loop `break`s at the first
leader, so it only checks that there is at least one leader, not exactly one.
```diff
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
index 679118e798..ae4bd0d96a 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
@@ -1538,8 +1538,6 @@ public CertificateStore getCertificateStore() {
public void start() throws IOException {
// If the SCM stopped during a snapshot install after the DB was
replaced, the VERSION file is behind the DB, and
// the finalization included in the snapshot is not in the Raft log to
be replayed.
- // The metadata store may have been closed by a prior stop(); start()
is idempotent and reopens it if needed.
- scmMetadataStore.start(configuration);
versionManager.finalizeFromDBIfRequired(scmMetadataStore.getMetaTable());
if (LOG.isInfoEnabled()) {
LOG.info(buildRpcServerStartMessage(
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManagerHA.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManagerHA.java
index 9ded335c96..288d7807ee 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManagerHA.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManagerHA.java
@@ -22,6 +22,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.CountDownLatch;
@@ -146,48 +147,30 @@ public void testSCMLeadershipMetric() throws
IOException, InterruptedException {
// in order to initialize and bootstrap SCM instances only
cluster = haMiniClusterBuilder.build();
- List<StorageContainerManager> storageContainerManagersList =
cluster.getStorageContainerManagersList();
-
- // stop the single SCM instance in order to imitate further
simultaneous start of SCMs
- storageContainerManagersList.get(0).stop();
- storageContainerManagersList.get(0).join();
-
- // WHEN (imitate simultaneous start of the SCMs)
- int retryCount = 0;
- while (true) {
- CountDownLatch scmInstancesCounter = new
CountDownLatch(scmInstancesCount);
- AtomicInteger failedSCMs = new AtomicInteger();
- for (StorageContainerManager scm : storageContainerManagersList) {
- new Thread(() -> {
- try {
- scm.start();
- } catch (IOException e) {
- failedSCMs.incrementAndGet();
- } finally {
- scmInstancesCounter.countDown();
- }
- }).start();
- }
- scmInstancesCounter.await();
- if (failedSCMs.get() == 0) {
- break;
- } else {
- for (StorageContainerManager scm : storageContainerManagersList) {
- scm.stop();
- scm.join();
- LOG.info("Stopping StorageContainerManager server at {}",
- scm.getClientRpcAddress());
+ // WHEN (imitate simultaneous start of the SCMs that were bootstrapped
but not started). A stopped SCM cannot be
+ // started again, so the running SCM is left as is.
+ List<StorageContainerManager> inactiveSCMs = new ArrayList<>();
+ cluster.getInactiveSCM().forEachRemaining(inactiveSCMs::add);
+ CountDownLatch scmInstancesCounter = new
CountDownLatch(inactiveSCMs.size());
+ AtomicInteger failedSCMs = new AtomicInteger();
+ for (StorageContainerManager scm : inactiveSCMs) {
+ new Thread(() -> {
+ try {
+ scm.start();
+ } catch (Exception e) {
+ LOG.error("Failed to start SCM {}", scm.getSCMNodeId(), e);
+ failedSCMs.incrementAndGet();
+ } finally {
+ scmInstancesCounter.countDown();
}
- ++retryCount;
- LOG.info("SCMs port conflicts, retried {} times",
- retryCount);
- failedSCMs.set(0);
- }
+ }).start();
}
+ scmInstancesCounter.await();
+ assertEquals(0, failedSCMs.get());
// THEN expect only one SCM node (leader) will have
'scmha_metrics_scmha_leader_state' metric set to 1
int leaderCount = 0;
- for (StorageContainerManager scm : storageContainerManagersList) {
+ for (StorageContainerManager scm :
cluster.getStorageContainerManagersList()) {
if (scm.getScmHAMetrics() != null &&
scm.getScmHAMetrics().getSCMHAMetricsInfoLeaderState() == 1) {
leaderCount++;
break;
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java:
##########
@@ -46,14 +46,32 @@ public void validateDBVersion(Table<String, String>
finalizationStore) throws IO
}
}
- public void finalizeFromSnapshotIfRequired(Table<String, String>
finalizationStore) throws IOException {
+ public void finalizeFromDBIfRequired(Table<String, String> finalizationStore)
Review Comment:
I need some more time to work through the cases in this method. Will post
more on this area in a follow-up review.
--
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]
