errose28 commented on code in PR #11800:
URL: https://github.com/apache/ozone/pull/11800#discussion_r4426606357
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -711,6 +711,9 @@ private void initializeSystemManagers(OzoneConfiguration
conf,
}
versionManager = new ScmVersionManager(scmStorageConfig, this);
+ // 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.
+ versionManager.finalizeIfRequired(scmMetadataStore.getMetaTable(), false);
Review Comment:
I think we should move this to the top of `start` so that upgrade actions
are not run on a partially constructed SCM instance.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java:
##########
@@ -46,14 +46,25 @@ public void validateDBVersion(Table<String, String>
finalizationStore) throws IO
}
}
- public void finalizeFromSnapshotIfRequired(Table<String, String>
finalizationStore) throws IOException {
+ public void finalizeIfRequired(Table<String, String> finalizationStore,
boolean fromSnapshot)
+ throws IOException {
ComponentVersion apparentVersionInNewDB =
getApparentVersionInDB(finalizationStore);
if (apparentVersionInNewDB != null && !isAllowed(apparentVersionInNewDB)) {
- LOG.info("New snapshot received with higher apparent version {}.
Attempting to finalize to that version.",
- apparentVersionInNewDB);
+ if (fromSnapshot) {
+ LOG.info("New snapshot received with higher apparent version {}.
Attempting to finalize to that version.",
+ apparentVersionInNewDB);
Review Comment:
I think we can remove this log line and the boolean parameter. There are
lots of messages already indicating whether a component is starting up or
installing a snapshot which can provide context for what this method logs.
IMO this version of the method better captures what do in each case.
```java
public void finalizeFromDBIfRequired(Table<String, String>
finalizationStore)
throws IOException {
ComponentVersion apparentVersionInNewDB =
getApparentVersionInDB(finalizationStore);
if (apparentVersionInNewDB == null) {
LOG.info("Component has apparent version {} but no apparent version
was found in the DB. That is expected if" +
"this component has never been finalized to a newer version.",
getApparentVersion());
} else if (!isAllowed(apparentVersionInNewDB)) {
// The DB is ahead of the VERSION file. This can happen if the leader
finalized and this follower received the
// update through a snapshot. It can also happen if the component was
finalizing from a snapshot, failed
// partway through, and restarted.
if (apparentVersionInNewDB != getSoftwareVersion()) {
LOG.warn("Received apparent version {} in DB which does not match
the current apparent version {} or" +
"software version {}. User may have incorrectly run finalization
before all components were upgraded." +
"Finalizing this component to its software version.",
apparentVersionInNewDB, getApparentVersion(),
getSoftwareVersion());
} else {
LOG.info("Version file has lower apparent version ({}) than DB ({}).
Finalizing to the DB version.",
getApparentVersion(), apparentVersionInNewDB);
}
finalizeUpgrade();
} else if (!apparentVersionInNewDB.equals(getApparentVersion())) {
// The DB is behind the VERSION file. This should not happen:
finalization writes the VERSION file inside
// applyTransaction, so the Ratis log entry was committed and should
be replayed on restart, which would
// update the DB. This is a defensive branch to catch any potential
bugs.
LOG.warn("DB has lower apparent version ({}) than VERSION file ({}).
Updating DB to match VERSION file.",
apparentVersionInNewDB, getApparentVersion());
finalizationStore.put(APPARENT_VERSION_KEY,
String.valueOf(getApparentVersion().serialize()));
}
}
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/upgrade/TestScmStartupInvalidApparentVersion.java:
##########
@@ -59,8 +68,91 @@ 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);
+ 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.
+ */
+ @Test
+ public void testStartupUpdatesDBWhenVersionFileAheadOfDB() throws Exception {
+ // Create an SCM whose VERSION file is already at SOFTWARE_VERSION
(simulates the crash scenario where the
+ // VERSION file was written during finalization but the DB was not yet
flushed).
Review Comment:
This is a defensive check against potential bugs and is never expected to
happen. The comments should clearly call this out. Right now they are ambiguous
as to whether this is an expected situation or not.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/upgrade/ScmUpgradeAction.java:
##########
@@ -22,7 +22,9 @@
/**
* Storage Container Manager Upgrade Action interface. An upgrade action is an
operation that
- * needs to be executed during finalization.
+ * needs to be executed during finalization. It may also run while the SCM is
being constructed, if its DB
+ * records a higher apparent version than its VERSION file, before the
managers created after the version
+ * manager exist.
Review Comment:
If startup finalization is moved to `StorageContainerManager#start` we don't
need to add this disclaimer about the upgrade action parameter being partially
initialized.
--
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]
