HDDS-12596. OM fs snapshot max limit is not enforced

[peterxcli]

What changes were proposed in this pull request?

//ozone-default.xml
<property>
  <name>ozone.om.fs.snapshot.max.limit</name>
  <value>1000</value>
  <tag>OZONE, OM, MANAGEMENT</tag>
  <description>
    The maximum number of filesystem snapshot allowed in an Ozone Manager.
  </description>
</property>

The value of the above property is passed to RDBStore but it is never used.

We should verify & limit the number of snapshots created in OMSnapshotCreateRequest.

• Get non-deleted rows count directly from NumSnapshotActive metrics.
  • It would be prepared on the OM node restart, and be inc/dec on TestOMSnapshotCreateRequest/TestOMSnapshotDeleteRequest. So it would reflect the real snapshotTable row count.
    

    ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java

    Lines 1949 to 1980 in a4d878e

    	/**
    	* Iterate the Snapshot table, check the status
    	* for every snapshot and update OMMetrics.
    	*/
    	private void updateActiveSnapshotMetrics()
    	throws IOException {
    	long activeGauge = 0;
    	long deletedGauge = 0;
    	try (TableIterator<String, ? extends
    	KeyValue<String, SnapshotInfo>> keyIter =
    	metadataManager.getSnapshotInfoTable().iterator()) {
    	while (keyIter.hasNext()) {
    	SnapshotInfo info = keyIter.next().getValue();
    	SnapshotInfo.SnapshotStatus snapshotStatus =
    	info.getSnapshotStatus();
    	if (snapshotStatus == SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE) {
    	activeGauge++;
    	} else if (snapshotStatus ==
    	SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED) {
    	deletedGauge++;
    	}
    	}
    	}
    	metrics.setNumSnapshotActive(activeGauge);
    	metrics.setNumSnapshotDeleted(deletedGauge);
    	}

What is the link to the Apache JIRA

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

How was this patch tested?

CI:
https://github.com/peterxcli/ozone/actions/runs/14679627514
https://github.com/peterxcli/ozone/actions/runs/14683601913

[peterxcli]

The value of the above property is passed to RDBStore but it is never used.

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

[peterxcli]

@szetszwo @SaketaChalamchala please take a look if you have time. Thanks!

[hemantk-12]

Thanks @peterxcli for working on this.

[hemantk-12]

@SaketaChalamchala can you please take a look?

[peterxcli]

@hemantk-12 Do you know how to sync the changes in interface-client proto file to proto.lock?

[adoroszlai]

Do you know how to sync the changes in interface-client proto file to proto.lock?

You shouldn't. proto.lock captures the previous release's state, so it's updated only before releasing Ozone. It is used as a reference to check for incompatible changes.

[peterxcli]

Do you know how to sync the changes in interface-client proto file to proto.lock?

You shouldn't. proto.lock captures the previous release's state, so it's updated only before releasing Ozone. It is used as a reference to check for incompatible changes.

Got it. Thanks @adoroszlai!

[hemantk-12]

Source code looks good. Left some comments on tests. Please take a look.

[swamirishi]

@peterxcli thanks for working on the patch. I have a few suggestions for making this implementation correct in case of concurrent ratis requests.

[swamirishi]

Make this method synchronized and have a parameter in memory which keeps the number of snapshots creation in flight in memory. On hindsight I believe we should not put this check inside validateAndUpdateCache since this can impact ratis replays. @hemantk-12
The in memory count should be increased once the pre execute passes inside a synchronized block and should be decremented once the snapshot gets added on to the snapshot chain in validateAndUpdate cache. The total number of snapshots at any time would be:
totalSnapshots = snapshotChainManager.size() + inmemoryCounter

[swamirishi]

To add more details:
Ratis replay would not be deterministic across various om nodes if the config is not matching across all nodes. We should only look at leader. Since validateAndUpdateCache runs on all nodes, this could lead to an inconsistent state.

[swamirishi]

Rename this function addSnapshotLimitCheck() or something

[hemantk-12]

This approach has an issue. preExecute runs only on a single node (the leader node, if I recall correctly), whereas validateAndUpdateCache runs on all nodes. If we decrement in validateAndUpdateCache, other nodes will keep decrementing without a corresponding increment, causing the value to go negative. Additionally, if leadership changes, it could result in exceeding the allowed limit.

IMO, we can simplify this by relying on the SnapshotChain count, which will act as a soft limit initially and eventually become a hard limit. I doubt that applyingTransaction is slower than the write request rate to the extent that it would create a backlog of snapshot creation requests.

[swamirishi]

@hemantk-12 We can ignore the inflight counter on the followers. We would be just using this counter on the leader. We don't have to be worried about the value in the followers. On leader switch this number can be reset to 0 again.

[hemantk-12]

Yes, we can ignore the inflight counter on the followers and start from 0, but if there is a leadership change while preExecute has completed and validateAndUpdateCache has not. When a new leader (former follower) tries to apply the transactions (basically executing validateAndUpdateCache), inflight counter will go negative.

[hemantk-12]

@swamirishi and I were discussing this, and one way to fix it is to implement notifyLeaderReady() and notifyNotLeader() similar to SCMStateMachine for OzoneManager and reset the inflight counter on leadership change.

cc: @swamirishi @SaketaChalamchala

[hemantk-12]

Thanks, @peterxcli for addressing all the review comments.
Overall changes look good to me.

[peterxcli]

agreed this section is prone to race conditions.
Semaphore would be a more appropriate solution, but it will be require a larger rewrite.

Agreed, this section is prone to race conditions.
A semaphore would be a more appropriate solution, but it would require a larger rewrite.

@jojochuang I have refactored that section by putting it into a synchronized block. I think it now has the same effect as a Semaphore:

Changed to use atomicReference to handle the exception and place the increment logic within the AtomicReference#updateAndGet block

[hemantk-12]

Thanks @peterxcli for the improvement.

LGTM+1.

[hemantk-12]

@swamirishi can you please take another look?

[jojochuang]

BTW ozone.om.fs.snapshot.max.limit of 1000 is probably too low. We have users at a few thousand snapshots already and will scale up even further. It looks like 1000 is not a problem. We should raise it a lot higher. That's a separate task.

EDIT: opened https://issues.apache.org/jira/browse/HDDS-12948 to track this task

[jojochuang]

It looks like snapshotLimitCheck() can still be prone to race conditions. But it should be fine in this case. The race is adding currentSnapshotNum to inFlightSnapshotCount. But the result is not saved so it doesn't cause data loss or anything. It's a guardrail and it's okay even if it's off by a few.

[peterxcli]

Thanks @hemantk-12, @swamirishi, @jojochuang and @SaketaChalamchala for the review, and @adoroszlai for the clarification of my proto.lock question!

[adoroszlai]

@jojochuang @peterxcli This caused build failure on master due to other recent changes around OM metadata classes. The compile error can be fixed by simply adding the required import, but the test is failing after that. Thus I had to revert this change, sorry.

As a sanity check, I recommend running local build and checkstyle/pmd before merging PRs.

gh pr checkout 8157
git merge --no-commit origin/master
mvn -DskipRecon -DskipShade -DskipTests clean verify
hadoop-ozone/dev-support/checks/checkstyle.sh
hadoop-ozone/dev-support/checks/pmd.sh

(Note: check exit code of pmd, Maven always shows success.)

When there are related changes on master, I also recommend merging it into the PR branch to trigger complete CI.

[swamirishi]

@peterxcli Can't we call this inside inflightSnapshotCount.updateAndGet() function that would make this threadsafe and consistent.

[swamirishi]

I was off the previous week so couldn't review this PR in time. Can you please make this change in a follow up jira.

[peterxcli]

Sure and thanks for the review! Created HDDS-12952 for this.