You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2025/03/25 15:03:56 UTC
[PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
peterxcli opened a new pull request, #8157:
URL: https://github.com/apache/ozone/pull/8157
## What changes were proposed in this pull request?
```xml
//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.
https://github.com/apache/ozone/blob/a4d878ef7ad0bd8697fddffa689b8159ca22df1f/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L1949-L1980
## 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/14060484307
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2057633471
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -172,6 +175,9 @@ public final class OmSnapshotManager implements AutoCloseable {
// Soft limit of the snapshot cache size.
private final int softCacheSize;
+ private int fsSnapshotMaxLimit;
+ private final AtomicInteger inFlightSnapshotCount = new AtomicInteger(0);
Review Comment:
`inFlightSnapshotCount` is the count of Ratis transactions that have not been applied yet.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2829494108
> 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`:
```java
synchronized (inFlightSnapshotCount) {
int currentCount = inFlightSnapshotCount.get();
if (currentSnapshotNum + currentCount >= fsSnapshotMaxLimit) {
throw new OMException(
String.format("Snapshot limit of %d reached. Cannot create more snapshots. " +
"Current snapshots: %d, In-flight creations: %d",
fsSnapshotMaxLimit, currentSnapshotNum, currentCount) +
" If you have already deleted some snapshots, " +
"please wait for the background service to complete the cleanup.",
OMException.ResultCodes.TOO_MANY_SNAPSHOTS);
}
inFlightSnapshotCount.set(currentCount + 1);
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #8157:
URL: https://github.com/apache/ozone/pull/8157
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2071917560
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -859,6 +869,42 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2039921272
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
@swamirishi and I were discussing this, and one way to fix it is to implement `notifyLeaderReady()` and `notifyNotLeader()` similar to [SCMStateMachine](https://github.com/apache/ozone/blob/7dfd8c1a7889b5947c87f81d1c4d24a30bf629f6/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java#L372) for OzoneManager and reset the `inflight` counter on leadership change.
cc: @swamirishi @SaketaChalamchala
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2057633471
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -172,6 +175,9 @@ public final class OmSnapshotManager implements AutoCloseable {
// Soft limit of the snapshot cache size.
private final int softCacheSize;
+ private int fsSnapshotMaxLimit;
+ private final AtomicInteger inFlightSnapshotCount = new AtomicInteger(0);
Review Comment:
`inFlightSnapshotCount` is the count of snapshot transactions in Ratis log that have not been applied yet.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2071924670
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -859,6 +869,42 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
Review Comment:
Sure and thanks for the review. Created [HDDS-12952](https://issues.apache.org/jira/browse/HDDS-12952) for this.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -859,6 +869,42 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
Review Comment:
Sure and thanks for the review! Created [HDDS-12952](https://issues.apache.org/jira/browse/HDDS-12952) for this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2760255574
@hemantk-12 Do you know how to sync the changes in `interface-client` proto file to `proto.lock`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2027380316
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
With `inFlightSnapshotCount` approach, it should be moved just before the request is submitted to Ratis at line 125. Because if the name or permission check fails, it will never reach `validateAndUpdateCache` to decrement the `inFlightSnapshotCount`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2804800091
> which can be detected by implementing
> NotifyLeaderChanged(I am not really sure if we need this, maybe it could be helpful resetting the counters in the followers
@swamirishi I think `notifyLeaderReady` is enough now. If we need more logic about follower/leader in future we can then migrate the reset logic to `notifyLeaderChanged`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2036075294
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -856,23 +858,31 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
- public void validateSnapshotLimit() throws IOException {
+ /*
+ * Check snapshot limit
+ * Note: This method increments the snapshot in-flight counter
+ */
+ public synchronized void snapshotLimitCheck() throws IOException {
OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
SnapshotChainManager snapshotChainManager =
omMetadataManager.getSnapshotChainManager();
int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
- if (currentSnapshotNum >= fsSnapshotMaxLimit) {
+ int inFlightCount = inFlightSnapshotCount.get();
Review Comment:
@peterxcli We should do this entire validation inside inFlightCount.update() block
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2035945168
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
No, currently you are just deleting entries directly from the SnapshotInfoTable and chain. Ideally, we should issue a delete snapshot (like [this](https://github.com/apache/ozone/blob/master/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshot.java#L1820)) for all the snapshots, and wait for `SnapshotInfoTable` to be empty.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2803988575
@hemantk-12 @swamirishi
Just came up one question: Does `notifyStatusChanged` happen before any request applied on new leader node and also executed in serial?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2022076658
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -228,6 +231,52 @@ public void testCloseOnEviction() throws IOException,
GenericTestUtils.waitFor(() -> {
return logCapture.getOutput().contains(msg);
}, 100, 30_000);
+
+ // clean up all snapshot
+ snapshotChainManager.deleteSnapshot(second);
+ snapshotChainManager.deleteSnapshot(first);
+ assertEquals(snapshotChainManager.getGlobalSnapshotChain().size(), 0);
+ }
+
+ @Test
+ public void testValidateSnapshotLimit() throws IOException {
+ Table<String, OmVolumeArgs> volumeTable = mock(Table.class);
+ Table<String, OmBucketInfo> bucketTable = mock(Table.class);
+ Table<String, SnapshotInfo> snapshotInfoTable = mock(Table.class);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), VOLUME_TABLE, volumeTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), BUCKET_TABLE, bucketTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), SNAPSHOT_INFO_TABLE, snapshotInfoTable);
+
+ om.getOmSnapshotManager().setFsSnapshotMaxLimit(2);
Review Comment:
Yes, but the validation function should read the value from config every time it's called. Do we want it to be that?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021902454
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
Rename this function addSnapshotLimitCheck() or something
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2054783740
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +858,40 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
+ int oldCount = inFlightSnapshotCount.get();
+ int newCount = inFlightSnapshotCount.updateAndGet(count -> {
Review Comment:
Since we are using two steps, get and update, we may throw an exception for a few snapshot requests due to the case condition between get and update. It is not a big deal, but we can avoid it by putting it inside a synchronized block on `inFlightSnapshotCount` object.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2016283073
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -165,6 +165,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
> > 2. Should we compare it against total snapshots? I'm thinking that if there is an issue in the SnapshotDeletionService, we may accumulate a lot of snapshots without knowing that there is a problem in the snapshot purge unless someone has good dashboard and alarms in place.
>
> Maybe use the size of globalSnapshotChain map in SnapshotChainManager?
@SaketaChalamchala Do you have any thoughts on this? Thanks
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2035945168
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
No, currently you are just deleting entries directly from the SnapshotInfoTable and chain. Ideally, we should issue a delete snapshot for all the snapshots, and wait for `SnapshotInfoTable` to be empty.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021892047
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().validateSnapshotLimit();
Review Comment:
yeah this validation should move to the validateAndUpdateCacheMethod
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2012627385
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -165,6 +165,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
1. We can't rely on this metric. If a bootstrapped node becomes the leader, it may not have the correct active snapshot count. We should use SnapshotInfoTable size.
2. Should we compare it against total snapshots? I'm thinking that if there is an issue in the SnapshotDeletionService, we may accumulate a lot of snapshots without knowing that there is a problem in the snapshot purge unless someone has good dashboard and alarms in place.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2059642586
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +858,40 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
+ int oldCount = inFlightSnapshotCount.get();
+ int newCount = inFlightSnapshotCount.updateAndGet(count -> {
Review Comment:
@peterxcli Why can't we directly have an AtomicReference<OMException> outside the block? This can be just set to the exception that needs to be thrown if at all it needs to be thrown
with a null check it can be checked outside of the getAndUpdate
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2059643655
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +858,40 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
+ int oldCount = inFlightSnapshotCount.get();
+ int newCount = inFlightSnapshotCount.updateAndGet(count -> {
Review Comment:
We wouldn't need the oldCount and newCount with the above proposed change
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2036114063
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2019107488
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -988,6 +989,9 @@ private void instantiateServices(boolean withNewSnapshot) throws IOException {
fsSnapshotEnabled = configuration.getBoolean(
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY,
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_DEFAULT);
+ fsSnapshotMaxLimit = configuration.getInt(
Review Comment:
1. Should this config be moved to [OmSnapshotManager](https://github.com/apache/ozone/blob/dc4789772168efe312f7832b6bb3bc3e7bc40d1b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L109)? If agree, we can just move `validateSnapshotLimit` to `OmSnapshotManager` as well.
1. Please remove this config from [OmMetadataManagerImpl](https://github.com/apache/ozone/blob/86d2027e328cd66c3319b355f3438842440ffa70/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java#L592) and [DBStoreBuilder](https://github.com/apache/ozone/blob/eb96ff489209ad9dab10a3cded277ae5ccd7cb4a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java#L101).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -99,11 +99,23 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
snapshotPath = snapshotInfo.getSnapshotPath();
}
+ private void validateSnapshotLimit(OzoneManager ozoneManager) throws IOException {
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ OMMetrics omMetrics = ozoneManager.getMetrics();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
I would prefer SnapshotChain over metric. Please use SnapshotChain.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2020029794
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -988,6 +989,9 @@ private void instantiateServices(boolean withNewSnapshot) throws IOException {
fsSnapshotEnabled = configuration.getBoolean(
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY,
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_DEFAULT);
+ fsSnapshotMaxLimit = configuration.getInt(
Review Comment:
Yes, you can do it as part of [HDDS-12690](https://issues.apache.org/jira/browse/HDDS-12690).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021927113
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().validateSnapshotLimit();
Review Comment:
Based on https://github.com/apache/ozone/pull/8157#discussion_r2021899830 let's leave it here and make other changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2039504201
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
@hemantk-12 What's your thoughts on this. Thanks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2751581499
> The value of the above property is passed to RDBStore but it is never used.
https://issues.apache.org/jira/browse/HDDS-12690
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2012770082
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -165,6 +165,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
> Maybe use the size of globalSnapshotChain map in SnapshotChainManager?
Yes, this works too.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2032149324
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
I agree with @hemantk-12. We need not reject a snapshot create because of inFlight Snapshot create requests. inFlight snapshot creates may not always result in a snapshot being created and added to the chain.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2032318977
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
Reverted the change.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2039913359
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
I guess it is OK to directly delete it from the test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2798227577
> @peterxcli Had a discussion with @hemantk-12 we should go ahead with enforcing hard limits on the number of snapshots. For that the number of snapshot creates inflight would have to be considered. This counter should be only relevant on leader. So on every leader switch this counter needs to be reset, which can be detected by implementing
As a side note we can actually implement a notifier similar to what SCM has to notify status of OM has changed, maybe resuing the same interface is also a good idea. What do you think @hemantk-12 ?
https://github.com/apache/ozone/blob/0e109d19eaaf82f8c5220d506c4b3d3a2ab03899/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMService.java#L27-L31
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2017775944
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -147,6 +147,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
OzoneManagerProtocolProtos.UserInfo userInfo = getOmRequest().getUserInfo();
String key = snapshotInfo.getTableKey();
try {
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
+ throw new OMException(
+ String.format("Snapshot limit of %d reached. Cannot create more snapshots.", maxSnapshots),
+ OMException.ResultCodes.INVALID_SNAPSHOT_ERROR);
Review Comment:
1. Can you please create a new ResultCode `TOO_MANY_SNAPSHOTS`?
2. We should move this check to preExecute() similar to [buckets](https://github.com/apache/ozone/blob/770d3e62144267d2970f19ad29916186bec2b085/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java#L97).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2836104534
@swamirishi can you please take another look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2060809943
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
We should move this after all other validations, before line 125. Otherwise, inflightCount will never be decreased if snapshot name validation or permission check fails.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2014569797
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotCreateRequest.java:
##########
@@ -264,6 +264,39 @@ public void testEntryExists() throws Exception {
assertEquals(2, getOmMetrics().getNumSnapshotCreates());
}
+ @Test
+ public void testSnapshotLimit() throws Exception {
+ when(getOzoneManager().isAdmin(any())).thenReturn(true);
+
+ when(getOzoneManager().getFsSnapshotMaxLimit()).thenReturn(1);
+
+ String key1 = getTableKey(getVolumeName(), getBucketName(), snapshotName1);
+
+ OMRequest omRequest =
+ createSnapshotRequest(getVolumeName(), getBucketName(), snapshotName1);
+ OMSnapshotCreateRequest omSnapshotCreateRequest = doPreExecute(omRequest);
+
+ assertNull(getOmMetadataManager().getSnapshotInfoTable().get(key1));
+ omSnapshotCreateRequest.validateAndUpdateCache(getOzoneManager(), 1);
+
+ assertNotNull(getOmMetadataManager().getSnapshotInfoTable().get(key1));
+
+ // Should fail as snapshot limit is 1
+ omRequest = createSnapshotRequest(getVolumeName(), getBucketName(), snapshotName2);
+ omSnapshotCreateRequest = doPreExecute(omRequest);
+ OMClientResponse omClientResponse =
+ omSnapshotCreateRequest.validateAndUpdateCache(getOzoneManager(), 2);
+
+ OMResponse omResponse = omClientResponse.getOMResponse();
+ assertNotNull(omResponse.getCreateSnapshotResponse());
+ assertEquals(OzoneManagerProtocolProtos.Status.INVALID_SNAPSHOT_ERROR,
+ omResponse.getStatus());
+
+ assertEquals(1, getOmMetrics().getNumSnapshotCreateFails());
+ assertEquals(1, getOmMetrics().getNumSnapshotActive());
+ assertEquals(2, getOmMetrics().getNumSnapshotCreates());
Review Comment:
Could you also expand the test to delete a snapshot and create one again?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2024030779
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -228,6 +231,52 @@ public void testCloseOnEviction() throws IOException,
GenericTestUtils.waitFor(() -> {
return logCapture.getOutput().contains(msg);
}, 100, 30_000);
+
+ // clean up all snapshot
+ snapshotChainManager.deleteSnapshot(second);
+ snapshotChainManager.deleteSnapshot(first);
+ assertEquals(snapshotChainManager.getGlobalSnapshotChain().size(), 0);
+ }
+
+ @Test
+ public void testValidateSnapshotLimit() throws IOException {
+ Table<String, OmVolumeArgs> volumeTable = mock(Table.class);
+ Table<String, OmBucketInfo> bucketTable = mock(Table.class);
+ Table<String, SnapshotInfo> snapshotInfoTable = mock(Table.class);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), VOLUME_TABLE, volumeTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), BUCKET_TABLE, bucketTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), SNAPSHOT_INFO_TABLE, snapshotInfoTable);
+
+ om.getOmSnapshotManager().setFsSnapshotMaxLimit(2);
Review Comment:
Nah, I don't want that. I was thinking if we could load a new config every time a test runs.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2036071493
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
@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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2798223689
@peterxcli Had a discussion with @hemantk-12 we should go ahead with enforcing hard limits on the number of snapshots. For that the number of snapshot creates inflight would have to be considered. This counter should be only relevant on leader. So on every leader switch this counter needs to be reset, which can be detected by implementing
NotifyLeaderChanged
https://github.com/apache/ratis/blob/c81902a094344d953f2fc5f5d7ed08dc7c171cdf/ratis-server-api/src/main/java/org/apache/ratis/statemachine/StateMachine.java#L215-L218 for ozoneManagerStateMachine
https://github.com/apache/ozone/blob/dc9952e446940b82b277a7e8c9d02239a8417a06/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java#L92
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2039503041
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
If we want to purge the snapshot through `ObjectStore#deleteSnapsot`, we have to set the mock `volumeTable` and `bucketTable` for each `OmSnapshot` to make the deleted keys scanning of `SnapshotDeletionService` work.
https://github.com/apache/ozone/blob/3cb97fecdd167a99eb86193c480220bc31f15a35/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java#L192-L199
And I had looked through the behaviours in `TestOmSnapshotManager`, it seems they all do direct operation on snapshotChain or the table. So I think it's ok to do cleanup via direct public function calling on them, and also make the cleanup logic explicit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2751584624
@szetszwo @SaketaChalamchala please take a look if you have time. Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021559024
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -228,6 +231,52 @@ public void testCloseOnEviction() throws IOException,
GenericTestUtils.waitFor(() -> {
return logCapture.getOutput().contains(msg);
}, 100, 30_000);
+
+ // clean up all snapshot
+ snapshotChainManager.deleteSnapshot(second);
+ snapshotChainManager.deleteSnapshot(first);
+ assertEquals(snapshotChainManager.getGlobalSnapshotChain().size(), 0);
+ }
+
+ @Test
+ public void testValidateSnapshotLimit() throws IOException {
+ Table<String, OmVolumeArgs> volumeTable = mock(Table.class);
+ Table<String, OmBucketInfo> bucketTable = mock(Table.class);
+ Table<String, SnapshotInfo> snapshotInfoTable = mock(Table.class);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), VOLUME_TABLE, volumeTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), BUCKET_TABLE, bucketTable);
+ HddsWhiteboxTestUtils.setInternalState(
+ om.getMetadataManager(), SNAPSHOT_INFO_TABLE, snapshotInfoTable);
+
+ om.getOmSnapshotManager().setFsSnapshotMaxLimit(2);
Review Comment:
Is it possible to set it in `OzoneConfig` rather than this way?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java:
##########
@@ -272,11 +273,16 @@ public void testEntryExist() throws Exception {
private OMSnapshotDeleteRequest doPreExecute(
OMRequest originalRequest) throws Exception {
+ return doPreExecute(originalRequest, getOzoneManager());
+ }
+
+ public static OMSnapshotDeleteRequest doPreExecute(
Review Comment:
Why do we need this change?
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -228,6 +231,52 @@ public void testCloseOnEviction() throws IOException,
GenericTestUtils.waitFor(() -> {
return logCapture.getOutput().contains(msg);
}, 100, 30_000);
+
+ // clean up all snapshot
+ snapshotChainManager.deleteSnapshot(second);
Review Comment:
Why do we need to remove these snapshots from the chain? Shouldn't `setupData` take care of it? If not, I'll suggest to move this code to `@BeforeEach` and `@AfterEach`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -352,6 +402,9 @@ public void testGetSnapshotInfo() throws IOException {
ome = assertThrows(OMException.class,
() -> om.getOmSnapshotManager().getSnapshot(s2.getSnapshotId()));
assertEquals(OMException.ResultCodes.FILE_NOT_FOUND, ome.getResult());
+
+ // clean up
+ snapshotChainManager.deleteSnapshot(s1);
Review Comment:
Same as previous.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2020374862
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -99,11 +99,23 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
snapshotPath = snapshotInfo.getSnapshotPath();
}
+ private void validateSnapshotLimit(OzoneManager ozoneManager) throws IOException {
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ OMMetrics omMetrics = ozoneManager.getMetrics();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
Done. PTAL.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021899830
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
Make this method synchronized and have a parameter in memory which keeps the number of snapshots creation in flight in memory. In hindsight 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
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2752263985
@SaketaChalamchala can you please take a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2027380316
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
With `inFlightSnapshotCount` approach, it should be moved just before the request is submitted to Ratis at line 125.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2845707887
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2846551538
@jojochuang @peterxcli This caused [build failure](https://github.com/apache/ozone/actions/runs/14790150556/job/41525587482#step:16:24) 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2028360157
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
@hemantk-12 @swamirishi Would you like me to revert the "inflight count" changes?
If you have any other idea on this, please also let me know, Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2044313178
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerHASnapshot.java:
##########
@@ -345,6 +345,29 @@ public void testKeyAndSnapshotDeletionService() throws IOException, InterruptedE
checkSnapshotIsPurgedFromDB(omFollower, tableKey);
}
+ @Test
+ public void testSnapshotInFlightCount() throws Exception {
+ // snapshot inflight count should be reset to 0 when leader changes
+
+ // first do some snapshot creations
+ String snapshotName1 = UUID.randomUUID().toString();
+ store.createSnapshot(volumeName, bucketName, snapshotName1);
+
+ // then shutdown the leader
+ OzoneManager omLeader = cluster.getOMLeader();
+ cluster.shutdownOzoneManager(omLeader);
+
+ // wait for the new leader to be elected
+ cluster.waitForLeaderOM();
+
+ // check the inflight count on the new leader is 0
+ OzoneManager newLeader = cluster.getOMLeader();
+ assertEquals(0, newLeader.getOmSnapshotManager().getInFlightSnapshotCount());
Review Comment:
I have check that if I comment out the reset logic, this test would failed with:
```java
ERROR] Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 112.5 s <<< FAILURE! -- in org.apache.hadoop.ozone.om.snapshot.TestOzoneManagerHASnapshot
[ERROR] org.apache.hadoop.ozone.om.snapshot.TestOzoneManagerHASnapshot.testSnapshotInFlightCount -- Time elapsed: 1.501 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <0> but was: <-24>
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:531)
at org.apache.hadoop.ozone.om.snapshot.TestOzoneManagerHASnapshot.testSnapshotInFlightCount(TestOzoneManagerHASnapshot.java:365)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2804269820
Ok I think `StartupLogEntry` provide this guarantee.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2057045464
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +858,40 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
+ int oldCount = inFlightSnapshotCount.get();
+ int newCount = inFlightSnapshotCount.updateAndGet(count -> {
Review Comment:
agreed this section is prone to race conditions.
Semaphore would be a more appropriate solution, but it will be require a larger rewrite.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -172,6 +175,9 @@ public final class OmSnapshotManager implements AutoCloseable {
// Soft limit of the snapshot cache size.
private final int softCacheSize;
+ private int fsSnapshotMaxLimit;
+ private final AtomicInteger inFlightSnapshotCount = new AtomicInteger(0);
Review Comment:
I'm new to this. What does inFlightSnapshotCount count?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2060809943
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
We should move this after all other validations. Otherwise, inflightCount will never be decreased if snapshot name validation or permission check fails.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2071916413
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -859,6 +869,42 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
Review Comment:
Can't we call this inside inflightSnapshotCount.updateAndGet() function that would make this threadsafe and consistent.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -859,6 +869,42 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
Review Comment:
@peterxcli Can't we call this inside inflightSnapshotCount.updateAndGet() function that would make this threadsafe and consistent.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2846491732
Thanks @hemantk-12, @swamirishi, @jojochuang and @SaketaChalamchala for the review, and @adoroszlai for the clarification of my `proto.lock` question!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2760274933
> > 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!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021899830
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
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
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2027433198
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
Should we revert the "inflight count" change? Maybe we can make it to become a real hard limit after migrating to pre-ratis execution?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2032001762
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
I'm OK with keeping the SnapshotChain-based approach and using it as an eventual limit rather than increasing the complexity with `inFlightCount` approach.
Even if `preExecute` runs only on a leader node and the count is decremented on the leader node, the issue persists in case of Ratis leadership change and log replay.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2012748531
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -165,6 +165,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
> 1\. We can't rely on this metric. If a bootstrapped node becomes the leader, it may not have the correct active snapshot count. We should use SnapshotInfoTable size.
I think it still have consistent value, which is guarantee by ratis.
> 2\. Should we compare it against total snapshots? I'm thinking that if there is an issue in the SnapshotDeletionService, we may accumulate a lot of snapshots without knowing that there is a problem in the snapshot purge unless someone has good dashboard and alarms in place.
Maybe use the size of globalSnapshotChain map in SnapshotChainManager?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021835809
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().validateSnapshotLimit();
Review Comment:
I discussed with @swamirish, it makes more sense to do this check in `validateAndUpdateCache` rather than `preExecute`. Limiting BucketCreation reason is different from SnapshotCreation. Since `validateAndUpdateCache` gives a consistence view, we should do it there. Sorry for the confusion.
cc: @swamirishi
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021835809
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().validateSnapshotLimit();
Review Comment:
I discussed with @swamirish, it makes more sense to do this check in `validateAndUpdateCache` rather than `preExecute`. Limiting BucketCreation reason is different from SnapshotCreation. Since `validateAndUpdateCache` gives a consistence view, we should use it. Sorry for the confusion.
cc: @swamirishi
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2021901789
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +856,25 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void validateSnapshotLimit() throws IOException {
Review Comment:
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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2027380316
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
With `inFlightSnapshotCount` approach, it should be moved just before the request is submitted to Ratis at line 125. Because if the name or permission check fail, it will never reach to `validateAndUpdateCache` to decrement it,
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2027459415
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -104,6 +104,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) {
@RequireSnapshotFeatureState(true)
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ // verify snapshot limit
+ ozoneManager.getOmSnapshotManager().snapshotLimitCheck();
Review Comment:
I agree that `inFlightCount` gives a real hard limit, but it comes with race condition problems as mentioned in [this](https://github.com/apache/ozone/pull/8157#discussion_r2027392804) comment.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2059642586
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -849,6 +858,40 @@ private void validateSnapshotsExistAndActive(final String volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ public void snapshotLimitCheck() throws IOException, OMException {
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager();
+ SnapshotChainManager snapshotChainManager = omMetadataManager.getSnapshotChainManager();
+ int currentSnapshotNum = snapshotChainManager.getGlobalSnapshotChain().size();
+ int oldCount = inFlightSnapshotCount.get();
+ int newCount = inFlightSnapshotCount.updateAndGet(count -> {
Review Comment:
Why can't we directly have an AtomicReference<OMException> outside the block? This can be just set to the exception that needs to be thrown if at all it needs to be thrown
with a null check it can be checked outside of the getAndUpdate
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2034051022
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
nit: we should delete snapshots and wait for purge.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2034301324
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
Do you mean calling `SnapshotDiffCleanupService#run`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2019674462
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -988,6 +989,9 @@ private void instantiateServices(boolean withNewSnapshot) throws IOException {
fsSnapshotEnabled = configuration.getBoolean(
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_KEY,
OMConfigKeys.OZONE_FILESYSTEM_SNAPSHOT_ENABLED_DEFAULT);
+ fsSnapshotMaxLimit = configuration.getInt(
Review Comment:
> 2\. Please remove this config from [OmMetadataManagerImpl](https://github.com/apache/ozone/blob/86d2027e328cd66c3319b355f3438842440ffa70/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java#L592) and [DBStoreBuilder](https://github.com/apache/ozone/blob/eb96ff489209ad9dab10a3cded277ae5ccd7cb4a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java#L101).
I was planning to clean them in https://issues.apache.org/jira/browse/HDDS-12690.
But if you feel ok, I can make the changes together in this patch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2034636764
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java:
##########
@@ -113,16 +119,37 @@ void init(@TempDir File tempDir) throws Exception {
OMConfigKeys.OZONE_OM_SNAPSHOT_CACHE_MAX_SIZE, 1);
configuration.setBoolean(
OMConfigKeys.OZONE_OM_SNAPSHOT_ROCKSDB_METRICS_ENABLED, false);
+ // Allow 2 fs snapshots
+ configuration.setInt(
+ OMConfigKeys.OZONE_OM_FS_SNAPSHOT_MAX_LIMIT, 2);
OmTestManagers omTestManagers = new OmTestManagers(configuration);
om = omTestManagers.getOzoneManager();
+ omMetadataManager = (OmMetadataManagerImpl) om.getMetadataManager();
+ omSnapshotManager = om.getOmSnapshotManager();
+ snapshotChainManager = omMetadataManager.getSnapshotChainManager();
}
@AfterAll
void stop() {
om.stop();
}
+ @AfterEach
+ void cleanup() throws IOException {
+ Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable();
Review Comment:
Or `invalidateAllCache` ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8157:
URL: https://github.com/apache/ozone/pull/8157#issuecomment-2760270904
> 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12596. OM fs snapshot max limit is not enforced [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #8157:
URL: https://github.com/apache/ozone/pull/8157#discussion_r2014566441
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -165,6 +165,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ // Check snapshot limit
+ int maxSnapshots = ozoneManager.getFsSnapshotMaxLimit();
+ if (omMetrics.getNumSnapshotActive() >= maxSnapshots) {
Review Comment:
Thanks for the patch @peterxcli. Could we move this check up before we take any locks?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org