HDDS-13572. SCMSnapshotProvider should not auto-create Ratis directory by echonesis · Pull Request #9689 · apache/ozone

echonesis · 2026-01-29T08:12:31Z

What changes were proposed in this pull request?

This PR prevents SCMSnapshotProvider from auto-creating Ratis directory when it is not configured.

Add validation to check Ratis directory existence before snapshot operations
Throw clear error when Ratis directory is missing instead of silently creating it

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13572

How was this patch tested?

GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/21465081605

szetszwo

@echonesis , thanks a lot for working on this!

Both the ratisStorageDir and ratisSnapshotDir should only be created only if either --init or --bootstrap option is set in StorageContainerManagerStarter. Without those options, the directories should already exist. If not, we should fail the command.

Please see also the comments inlined.

szetszwo · 2026-02-02T18:01:44Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

  @VisibleForTesting
-  protected SCMSnapshotProvider newScmSnapshotProvider(StorageContainerManager storageContainerManager) {
-    return new SCMSnapshotProvider(storageContainerManager.getConfiguration(),
+  protected SCMSnapshotProvider newScmSnapshotProvider(
+      StorageContainerManager storageContainerManager) {


This method is not used in test. Let's remove @ VisibleForTesting

szetszwo · 2026-02-02T18:06:38Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMSnapshotProvider.java

  public SCMSnapshotProvider(ConfigurationSource conf,
      List<SCMNodeDetails> peerNodes,
      CertificateClient scmCertificateClient) {


This method is used only in TestSCMSnapshotProvider, let's remove it and change the test.

szetszwo · 2026-02-02T18:07:21Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

+    File snapshotDirFile = new File(snapshotDir);
+
+    SCMSnapshotProvider.StartupOption startupOption;
+    if (snapshotDirFile.exists()) {


Let's use

if (Files.isDirectory(Paths.get(snapshotDir))) {

szetszwo · 2026-02-02T18:26:48Z

hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test-repair-tools.sh

+wait_for_om_leader
+
+echo "Deleting test keys to create tombstones"
+docker exec "${om_container}" bash -c "kinit -kt /etc/security/keytabs/testuser.keytab testuser/om@EXAMPLE.COM && ozone fs -rm -R -skipTrash ofs://omservice/vol1/bucket1"
+echo "Test keys deleted"
+
+echo "Restarting OM after key deletion to flush tombstones to sst files"
+docker restart "${om_container}"
+wait_for_om_leader


This change seems unrelated? If yes, please revert it.

These changes are related - the stricter Ratis directory checks affect startup timing.
The wait_for_om_leader ensures proper ordering after container restarts.

szetszwo · 2026-02-02T18:27:03Z

hadoop-ozone/dist/src/main/smoketest/repair/om-compact.robot

+SST Size Should Be Reduced
+    [Arguments]    ${original_size}
+    ${current_size} =    Get OM DB SST Files Size
+    Should Be True    ${current_size} < ${original_size}
+    ...    OM DB size should be reduced after compaction. Before: ${original_size}, After: ${current_size}
+
 *** Test Cases ***
 Testing OM DB Size Reduction After Compaction
-    # Test keys are already created and flushed
-    # Delete keys to create tombstones that need compaction
-    Delete Test Keys
-
+    # Test keys are already created, deleted and flushed by the test script
    ${size_before_compaction} =    Get OM DB SST Files Size

    Compact OM DB Column Family    fileTable
    Compact OM DB Column Family    deletedTable
    Compact OM DB Column Family    deletedDirectoryTable
-
-    ${size_after_compaction} =    Get OM DB SST Files Size

-    Should Be True    ${size_after_compaction} < ${size_before_compaction}
-    ...    OM DB size should be reduced after compaction. Before: ${size_before_compaction}, After: ${size_after_compaction}
+    # Compaction is asynchronous, poll until size is reduced
+    Wait Until Keyword Succeeds    60sec    5sec    SST Size Should Be Reduced    ${size_before_compaction}


This change seems unrelated? If yes, please revert it.

Same as #9689 (comment)
This ensures proper ordering after container restarts.

szetszwo · 2026-02-02T18:37:24Z

...hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java

+
+    // Create Ratis snapshot directory during initialization
+    // This ensures the directory exists before SCM starts normally
+    String snapshotDir = SCMHAUtils.getSCMRatisSnapshotDirectory(conf);


snapshotDir should be created only for scmInit or scmBootstrap. So, it should not be created here.

echonesis · 2026-02-03T02:39:22Z

@echonesis , thanks a lot for working on this!

Both the ratisStorageDir and ratisSnapshotDir should only be created only if either --init or --bootstrap option is set in StorageContainerManagerStarter. Without those options, the directories should already exist. If not, we should fail the command.

Please see also the comments inlined.

Thanks @szetszwo
I agree with this principle. However, there's an upgrade scenario I'd like to clarify:
In StorageContainerManager constructor (L376-378), when an old cluster (without Ratis/HA) starts normally and detects !isSCMHAEnabled(), it calls initializeRatis() to auto-enable Ratis. This also needs to create the directories.
Should this upgrade path:

Continue to auto-create directories (current behavior), or
Require users to run --init first before upgrading?

szetszwo · 2026-02-03T18:32:44Z

... there's an upgrade scenario I'd like to clarify ...

Thanks for pointing out the upgrade scenario. The initializeRatis(..) does not auto-create directories. When the directories are created for upgrade?

We probably should have --upgrade option. Any "auto-create directories" behavior is error prone and leads to data loss.

echonesis · 2026-02-04T04:45:25Z

... there's an upgrade scenario I'd like to clarify ...

Thanks for pointing out the upgrade scenario. The initializeRatis(..) does not auto-create directories. When the directories are created for upgrade?

We probably should have --upgrade option. Any "auto-create directories" behavior is error prone and leads to data loss.

In initializeRatis() (StorageContainerManager.java L1354-L1358), I added explicit creation of snapshotDir:

String snapshotDir = SCMHAUtils.getSCMRatisSnapshotDirectory(conf);
HddsUtils.createDir(snapshotDir);

For ratisStorageDir, it's auto-created by Ratis internally when SCMRatisServerImpl.initialize() calls RaftServer.build(). The upgrade scenario (L376-378) calls initializeRatis() during normal startup when !isSCMHAEnabled(), which triggers both directory creations.

I agree that adding an --upgrade option would be a cleaner approach.

szetszwo · 2026-02-04T20:25:00Z

In initializeRatis() (StorageContainerManager.java L1354-L1358), I added explicit creation of snapshotDir:

Sorry, I mean to ask -- When is the snapshotDir created for upgrade before this change?

echonesis · 2026-02-05T02:08:57Z

In initializeRatis() (StorageContainerManager.java L1354-L1358), I added explicit creation of snapshotDir:

Sorry, I mean to ask -- When is the snapshotDir created for upgrade before this change?

Before this change, both directories were auto-created in SCMSnapshotProvider constructor (L71, L74-75) via HddsUtils.createDir(). So for upgrade, when SCMHAManagerImpl creates SCMSnapshotProvider, it auto-creates both directories there.

szetszwo · 2026-02-05T02:18:00Z

Before this change, both directories were auto-created in SCMSnapshotProvider constructor ...

@echonesis , thanks for the info. For now, let's add the auto-create only for upgrade case but not inside initializeRatis(..) since initializeRatis(..) is also used in the non-upgrade cases. Would it work?

echonesis · 2026-02-05T03:14:45Z

Before this change, both directories were auto-created in SCMSnapshotProvider constructor ...

@echonesis , thanks for the info. For now, let's add the auto-create only for upgrade case but not inside initializeRatis(..) since initializeRatis(..) is also used in the non-upgrade cases. Would it work?

Yes, that works. I'll move the auto-create logic to the upgrade case instead of initializeRatis().
One clarification: should we still plan for a --upgrade option in a follow-up JIRA to make this explicit, or is auto-create in the upgrade path acceptable as a long-term solution?

HDDS-13572. SCMSnapshotProvider should not auto-create Ratis directory

04b252e

szetszwo reviewed Feb 2, 2026

View reviewed changes

Conversation

echonesis commented Jan 29, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echonesis commented Feb 3, 2026

Uh oh!

szetszwo commented Feb 3, 2026

Uh oh!

echonesis commented Feb 4, 2026

Uh oh!

szetszwo commented Feb 4, 2026

Uh oh!

echonesis commented Feb 5, 2026

Uh oh!

szetszwo commented Feb 5, 2026

Uh oh!

echonesis commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants