Skip to content

HDDS-14570. SCM should finalize all versions with one Ratis request #9715

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

Merged
sodonnel merged 4 commits into from
Mar 19, 2026
Merged

HDDS-14570. SCM should finalize all versions with one Ratis request #9715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44c9358
HDDS-14570. SCM should finalize all versions with one Ratis request
Feb 5, 2026
c501d9e
Refactor test to finalize all versions in a single call
Feb 6, 2026
25991d7
Skip finalize if the feature is already finalized
Mar 5, 2026
b1a344e
Change from and to version to just target version
Mar 6, 2026
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
7 changes: 7 additions & 0 deletions ...p-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/BasicUpgradeFinalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ private static boolean isFinalized(Status status) {
public abstract void finalizeLayoutFeature(LayoutFeature lf, T context)
throws UpgradeException;

public void finalizeLayoutFeatures(Iterable<LayoutFeature> features, T context)
throws UpgradeException {
for (LayoutFeature lf : features) {
finalizeLayoutFeature(lf, context);
}
}

protected void finalizeLayoutFeature(LayoutFeature lf, Optional<?
extends UpgradeAction> action, Storage storage)
throws UpgradeException {
Expand Down
4 changes: 1 addition & 3 deletions ...ork/src/main/java/org/apache/hadoop/ozone/upgrade/DefaultUpgradeFinalizationExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ public void execute(T component, BasicUpgradeFinalizer<T, ?> finalizer)
protected void finalizeFeatures(T component,
BasicUpgradeFinalizer<T, ?> finalizer, Iterable<LayoutFeature> lfs)
throws UpgradeException {
for (LayoutFeature lf: lfs) {
finalizer.finalizeLayoutFeature(lf, component);
}
finalizer.finalizeLayoutFeatures(lfs, component);
}
}
2 changes: 1 addition & 1 deletion ...scm/src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/FinalizationStateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public interface FinalizationStateManager {
void removeFinalizingMark() throws IOException;

@Replicate
void finalizeLayoutFeature(Integer layoutVersion)
void finalizeLayoutFeatures(Integer toLayoutVersion)
throws IOException;

/**
Expand Down
19 changes: 14 additions & 5 deletions ...src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/FinalizationStateManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,11 @@ public void addFinalizingMark() throws IOException {
}

@Override
public void finalizeLayoutFeature(Integer layoutVersion) throws IOException {
finalizeLayoutFeatureLocal(layoutVersion);
public void finalizeLayoutFeatures(Integer toVersion) throws IOException {
int startLayoutVersion = versionManager.getMetadataLayoutVersion() + 1;
Copy link
Copy Markdown
Contributor

@dombizita dombizita Mar 19, 2026

Choose a reason for hiding this comment

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

Overall the changes make sense to me, only this line is a little hard to track back. Where does this come from?

Copy link
Copy Markdown
Contributor Author

@sodonnel sodonnel Mar 19, 2026

Choose a reason for hiding this comment

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

The current metaLayoutVersion reported by the versionManager is the last version that was finalized. So when we get a finalizeLayoutFeatures request we have to finalize all features from "current + 1" to the toVersion passed. That is why we get MetaLayoutVersion + 1.

In a future change, we have some logic to apply to the toVersion - eg if the toVersion is greater than our softwareLayoutVersion it means this software is an unexpected version and it should abort. But as the current implementation doesn't do that, we are keeping it as it was for now, and will add that when its all simplified.

Otherwise we could just drop the toVersion parameter and just finalize everything that isn't already finalized.

Does that answer your question?

Copy link
Copy Markdown
Contributor

@errose28 errose28 Mar 19, 2026

Choose a reason for hiding this comment

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

The toVersion parameter will be used checked against our software version in a future change to make sure all machines are running the same software version before proceeding with finalization. We can just use versionManager.unfinalizedFeatures to actually iterate the features and send them in for finalization though.

Copy link
Copy Markdown
Contributor Author

@sodonnel sodonnel Mar 19, 2026

Choose a reason for hiding this comment

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

I'll tidy this up in the next PR. Probably if I take away the use of toVersion, I will get checkstyle or PMD warnings about unused parameters ...

for (int version = startLayoutVersion; version <= toVersion; version++) {
finalizeLayoutFeatureLocal(version);
}
}

/**
Expand All @@ -115,9 +118,15 @@ private void finalizeLayoutFeatureLocal(Integer layoutVersion)
// version. This is updated in the replicated finalization steps.
// Layout version will be written to the DB as well so followers can
// finalize from a snapshot.
HDDSLayoutFeature feature =
(HDDSLayoutFeature)versionManager.getFeature(layoutVersion);
upgradeFinalizer.replicatedFinalizationSteps(feature, upgradeContext);
if (versionManager.getMetadataLayoutVersion() >= layoutVersion) {
LOG.warn("Attempting to finalize layout feature for layout version {}, but " +
"current metadata layout version is {}. Skipping finalization for this layout version.",
layoutVersion, versionManager.getMetadataLayoutVersion());
} else {
HDDSLayoutFeature feature =
(HDDSLayoutFeature) versionManager.getFeature(layoutVersion);
upgradeFinalizer.replicatedFinalizationSteps(feature, upgradeContext);
}
} finally {
checkpointLock.writeLock().unlock();
}
Expand Down
29 changes: 20 additions & 9 deletions ...rver-scm/src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/SCMUpgradeFinalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,33 @@ public void preFinalizeUpgrade(SCMUpgradeFinalizationContext context)

@Override
public void finalizeLayoutFeature(LayoutFeature lf,
SCMUpgradeFinalizationContext context) throws UpgradeException {
// Run upgrade actions, update VERSION file, and update layout version in
// DB.
SCMUpgradeFinalizationContext context) throws UpgradeException {
throw new UnsupportedOperationException("FinalizeLayoutFeature is not supported in this implemementation. " +
"Please use finalizeLayoutFeatures instead.");
}

@Override
public void finalizeLayoutFeatures(Iterable<LayoutFeature> features, SCMUpgradeFinalizationContext context)
throws UpgradeException {
int lastLv = -1;
for (LayoutFeature lf : features) {
lastLv = lf.layoutVersion();
Copy link
Copy Markdown

@yandrey321 yandrey321 Feb 12, 2026

Choose a reason for hiding this comment

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

should it be inside if (-1 != lf.layoutVersion()) check?

Copy link
Copy Markdown
Contributor Author

@sodonnel sodonnel Mar 6, 2026

Choose a reason for hiding this comment

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

It was intentionally like this, as we want to store the first and last value in the iterator. However I have refactored this a bit in the latest version as we don't actually need the firstLv to be passed through.

}
try {
context.getFinalizationStateManager()
.finalizeLayoutFeature(lf.layoutVersion());
if (lastLv > -1) {
// If there are no feature to finalize we just skip this and the finalize operation is a noop.
context.getFinalizationStateManager().finalizeLayoutFeatures(lastLv);
} else {
LOG.info("No layout features to finalize.");
}
} catch (IOException ex) {
throw new UpgradeException(ex,
UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED);
throw new UpgradeException(ex, UpgradeException.ResultCodes.LAYOUT_FEATURE_FINALIZATION_FAILED);
}
}

/**
* Run on each SCM (leader and follower) when a layout feature is being
* finalized to run its finalization actions, update the VERSION file, and
* move the state of its in memory datanodes to healthy readonly.
* finalized to run its finalization actions, update the VERSION file.
*
* @param lf The layout feature that is being finalized.
* @param context Supplier of objects needed to run the steps.
Expand Down
32 changes: 8 additions & 24 deletions ...hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/upgrade/TestScmFinalization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,33 +114,17 @@ public void testUpgradeStateToCheckpointMapping() throws Exception {
.build();
stateManager.setUpgradeContext(context);

assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.FINALIZATION_REQUIRED);
assertCurrentCheckpoint(scmContext, stateManager, FinalizationCheckpoint.FINALIZATION_REQUIRED);
stateManager.addFinalizingMark();
assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.FINALIZATION_STARTED);

for (HDDSLayoutFeature feature: HDDSLayoutFeature.values()) {
// Cannot finalize initial version since we are already there.
if (!feature.equals(HDDSLayoutFeature.INITIAL_VERSION)) {
stateManager.finalizeLayoutFeature(feature.layoutVersion());
if (versionManager.needsFinalization()) {
assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.FINALIZATION_STARTED);
} else {
assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.MLV_EQUALS_SLV);
}
}
}
// Make sure we reached this checkpoint if we finished finalizing all
// layout features.
assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.MLV_EQUALS_SLV);
assertCurrentCheckpoint(scmContext, stateManager, FinalizationCheckpoint.FINALIZATION_STARTED);

HDDSLayoutFeature[] finalizationFeatures = HDDSLayoutFeature.values();
HDDSLayoutFeature finalVersion = finalizationFeatures[finalizationFeatures.length - 1];
assertCurrentCheckpoint(scmContext, stateManager, FinalizationCheckpoint.FINALIZATION_STARTED);
stateManager.finalizeLayoutFeatures(finalVersion.layoutVersion());
assertCurrentCheckpoint(scmContext, stateManager, FinalizationCheckpoint.MLV_EQUALS_SLV);
stateManager.removeFinalizingMark();
assertCurrentCheckpoint(scmContext, stateManager,
FinalizationCheckpoint.FINALIZATION_COMPLETE);
assertCurrentCheckpoint(scmContext, stateManager, FinalizationCheckpoint.FINALIZATION_COMPLETE);
}

private void assertCurrentCheckpoint(SCMContext context,
Expand Down