Copilot commented on code in PR #13577: URL: https://github.com/apache/ignite/pull/13577#discussion_r4044169999
########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.cache.persistence.snapshot; + +import java.io.File; +import java.util.ArrayList; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.ignite.IgniteIllegalStateException; +import org.apache.ignite.IgniteLogger; +import org.apache.ignite.internal.GridKernalContext; +import org.apache.ignite.internal.IgniteInternalFuture; +import org.apache.ignite.internal.NodeStoppingException; +import org.apache.ignite.internal.processors.cache.persistence.filename.SnapshotFileTree; +import org.apache.ignite.internal.util.distributed.DistributedProcess; +import org.apache.ignite.internal.util.future.GridFinishedFuture; +import org.apache.ignite.internal.util.future.GridFutureAdapter; +import org.apache.ignite.internal.util.future.IgniteFutureImpl; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.lang.IgniteFuture; +import org.jetbrains.annotations.Nullable; + +import static org.apache.ignite.internal.processors.rollingupgrade.feature.SupportedFeatureRegistry.SNAPSHOT_DELETE_FEATURE; +import static org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.DELETE_SNAPSHOT; +import static org.apache.ignite.plugin.security.SecurityPermission.ADMIN_SNAPSHOT; + +/** + * Distributed process to delete a cluster snapshot. The operation is rejected if any concurrent snapshot operation is + * active. + */ +public class SnapshotDeleteProcess { + /** Reject operation messages. */ + private static final String OP_REJECT_MSG = "Snapshot deletion was rejected. "; + + /** */ + private static final String SNP_PATH_ERR_PREF = "Provided snapshot path "; + + /** Kernal context. */ + private final GridKernalContext kctx; + + /** Logger. */ + private final IgniteLogger log; + + /** */ + private volatile boolean interrupted; + + /** Cluster-wide operation futures per request id on certain node. */ + private final Map<UUID, GridFutureAdapter<SnapshotDeleteProcessResult>> clusterOpFuts = new ConcurrentHashMap<>(); + + /** Process requests per snapshot name on each server node. */ + private final Set<SnapshotDeleteRequest> requests = ConcurrentHashMap.newKeySet(); + + /** The distributed process. */ + private final DistributedProcess<SnapshotDeleteRequest, SnapshotDeleteResponse> distrProc; + + /** + * @param ctx Kernal context. + */ + public SnapshotDeleteProcess(GridKernalContext ctx) { + this.kctx = ctx; + + log = ctx.log(getClass()); + + distrProc = new DistributedProcess<>(ctx, DELETE_SNAPSHOT, this::deletePhase, this::reducePhase); + } + + /** + * Starts the cluster snapshot delete process. + * + * @param snpName Snapshot name. + * @param snpPath Snapshot directory path (optional). + * @return Future that will be completed when the snapshot is deleted. + */ + public IgniteFuture<SnapshotDeleteProcessResult> start(String snpName, @Nullable String snpPath) { + UUID reqId = UUID.randomUUID(); + + var clusterOpFut = new GridFutureAdapter<SnapshotDeleteProcessResult>(); + + clusterOpFut.listen(fut -> clusterOpFuts.remove(reqId)); + + try { + synchronized (clusterOpFuts) { + if (interrupted || kctx.isStopping()) + throw new NodeStoppingException("Failed to start snapshot delete process: node is stopping."); + + clusterOpFuts.put(reqId, clusterOpFut); + } + + SnapshotDeleteRequest req = new SnapshotDeleteRequest(reqId, snpName, snpPath); + + distrProc.start(reqId, req); Review Comment: The feature check is only performed inside `deletePhase`, after this call has broadcast an `InitMessage` containing the new `SnapshotDeleteRequest`. During rolling upgrade, 2.19.0 nodes do not register this message, so invoking deletion while `SNAPSHOT_DELETE_FEATURE` is inactive can fail during discovery-message deserialization instead of returning the expected feature-not-activated error. Check the feature before starting the distributed process and complete the future locally. This issue also appears in the following locations of the same file: - line 186 - line 244 ########## modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java: ########## @@ -21,4 +21,7 @@ public class TestIgniteReleaseFeatures_2_19_1 { /** */ public static final IgniteFeature ROLLING_UPGRADE_FEATURE = TestIgniteReleaseFeatures_2_19_0.ROLLING_UPGRADE_FEATURE; + + /** */ + public static final IgniteFeature SNAPSHOT_DELETE_FEATURE = SupportedFeatureRegistry.SNAPSHOT_DELETE_FEATURE; Review Comment: This assigns core feature ID 1 to snapshot deletion, but the existing simulated 2.19.2 release already assigns the same ID to `VER_2_19_2_ID_1_FEATURE` and continues with ID 2. `IgniteCoreFeature` equality is based only on the numeric ID, so the later release feature set now aliases an unrelated feature and the rolling-upgrade feature history becomes inconsistent; shift the subsequent release IDs/aliases (and their expected feature ranges) to preserve the new ID 1 meaning. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.cache.persistence.snapshot; + +import java.io.File; +import java.util.ArrayList; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.ignite.IgniteIllegalStateException; +import org.apache.ignite.IgniteLogger; +import org.apache.ignite.internal.GridKernalContext; +import org.apache.ignite.internal.IgniteInternalFuture; +import org.apache.ignite.internal.NodeStoppingException; +import org.apache.ignite.internal.processors.cache.persistence.filename.SnapshotFileTree; +import org.apache.ignite.internal.util.distributed.DistributedProcess; +import org.apache.ignite.internal.util.future.GridFinishedFuture; +import org.apache.ignite.internal.util.future.GridFutureAdapter; +import org.apache.ignite.internal.util.future.IgniteFutureImpl; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.lang.IgniteFuture; +import org.jetbrains.annotations.Nullable; + +import static org.apache.ignite.internal.processors.rollingupgrade.feature.SupportedFeatureRegistry.SNAPSHOT_DELETE_FEATURE; +import static org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.DELETE_SNAPSHOT; +import static org.apache.ignite.plugin.security.SecurityPermission.ADMIN_SNAPSHOT; + +/** + * Distributed process to delete a cluster snapshot. The operation is rejected if any concurrent snapshot operation is + * active. + */ +public class SnapshotDeleteProcess { + /** Reject operation messages. */ + private static final String OP_REJECT_MSG = "Snapshot deletion was rejected. "; + + /** */ + private static final String SNP_PATH_ERR_PREF = "Provided snapshot path "; + + /** Kernal context. */ + private final GridKernalContext kctx; + + /** Logger. */ + private final IgniteLogger log; + + /** */ + private volatile boolean interrupted; + + /** Cluster-wide operation futures per request id on certain node. */ + private final Map<UUID, GridFutureAdapter<SnapshotDeleteProcessResult>> clusterOpFuts = new ConcurrentHashMap<>(); + + /** Process requests per snapshot name on each server node. */ + private final Set<SnapshotDeleteRequest> requests = ConcurrentHashMap.newKeySet(); + + /** The distributed process. */ + private final DistributedProcess<SnapshotDeleteRequest, SnapshotDeleteResponse> distrProc; + + /** + * @param ctx Kernal context. + */ + public SnapshotDeleteProcess(GridKernalContext ctx) { + this.kctx = ctx; + + log = ctx.log(getClass()); + + distrProc = new DistributedProcess<>(ctx, DELETE_SNAPSHOT, this::deletePhase, this::reducePhase); + } + + /** + * Starts the cluster snapshot delete process. + * + * @param snpName Snapshot name. + * @param snpPath Snapshot directory path (optional). + * @return Future that will be completed when the snapshot is deleted. + */ + public IgniteFuture<SnapshotDeleteProcessResult> start(String snpName, @Nullable String snpPath) { + UUID reqId = UUID.randomUUID(); + + var clusterOpFut = new GridFutureAdapter<SnapshotDeleteProcessResult>(); + + clusterOpFut.listen(fut -> clusterOpFuts.remove(reqId)); + + try { + synchronized (clusterOpFuts) { + if (interrupted || kctx.isStopping()) + throw new NodeStoppingException("Failed to start snapshot delete process: node is stopping."); + + clusterOpFuts.put(reqId, clusterOpFut); + } + + SnapshotDeleteRequest req = new SnapshotDeleteRequest(reqId, snpName, snpPath); + + distrProc.start(reqId, req); + } + catch (Throwable t) { + log.error("Failed to start distributed delete snapshot process [snpName=" + snpName + ", snpPath=" + snpPath + ']', t); + + clusterOpFut.onDone(t); + } + + return new IgniteFutureImpl<>(clusterOpFut); + } + + /** */ + private IgniteInternalFuture<SnapshotDeleteResponse> deletePhase(UUID ignored, SnapshotDeleteRequest req) { + if (interrupted || kctx.isStopping()) { + return new GridFinishedFuture<>(new NodeStoppingException(OP_REJECT_MSG + + " Node is stopping [req=" + req + ']')); + } + + if (kctx.cluster().get().localNode().isClient()) + return new GridFinishedFuture<>(new SnapshotDeleteResponse(null)); + + kctx.security().authorize(ADMIN_SNAPSHOT); + + IgniteSnapshotManager snpMgr = kctx.cache().context().snapshotMgr(); + + var curCreateRq = snpMgr.currentCreateRequest(); + + if (curCreateRq != null && curCreateRq.snpName.equals(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being created [req=" + req + ']')); + } + + if (snpMgr.isRestoring(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being restored [req=" + req + ']')); + } + + if (snpMgr.isSnapshotChecking(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being checked [req=" + req + ']')); + } + + if (!kctx.rollingUpgrade().features().isActive(SNAPSHOT_DELETE_FEATURE)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "The snapshot deletion feature isn't activated yet [req=" + req + ']')); + } + + File path = null; + + if (!F.isEmpty(req.snpPath)) { + path = new File(req.snpPath); + + if (!path.isAbsolute()) + path = new File(kctx.pdsFolderResolver().fileTree().snapshotsRoot(), req.snpPath); + + String pathValidationErr = validateAbsoluteSnapshotRoot(path); + + if (pathValidationErr != null) { + return new GridFinishedFuture<>(new IllegalArgumentException(OP_REJECT_MSG + + pathValidationErr + " [req=" + req + ']')); + } + } + + try { + if (!requests.add(req)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException("Deletion of the snapshot has already " + + "started [req=" + req + ']')); + } + + GridFutureAdapter<SnapshotDeleteResponse> reqLocFut = new GridFutureAdapter<>(); + + File path0 = path; + + kctx.pools().getSnapshotExecutorService().submit(() -> { + try { + AtomicBoolean foundFlag = new AtomicBoolean(); + + var sft = new SnapshotFileTree(kctx, req.snpName, path0 == null ? null : path0.getAbsolutePath()); + + boolean deleted = snpMgr.deleteLocalSnapshot(sft, foundFlag); + + SnapshotDeleteResponse.SnapshotDeleteStatus res; + + if (foundFlag.get()) { + if (deleted && log.isInfoEnabled()) + log.info("Snapshot successfully deleted [req=" + req + ']'); + else if (!deleted) + log.warning("Snapshot deleted not completely [req=" + req + ']'); + + res = deleted + ? SnapshotDeleteResponse.SnapshotDeleteStatus.DELETED + : SnapshotDeleteResponse.SnapshotDeleteStatus.PARTLY_DELETED; + } + else { + if (log.isInfoEnabled()) + log.info("Snapshot not found to delete [req=" + req + ']'); + + res = SnapshotDeleteResponse.SnapshotDeleteStatus.NOT_FOUND; + } + + reqLocFut.onDone(new SnapshotDeleteResponse(res)); + } + finally { + requests.remove(req); + } + }); + + if (log.isInfoEnabled()) + log.info("Deletion of snapshot initialized [req=" + req + ']'); + + return reqLocFut; + } + catch (Throwable t) { + requests.remove(req); + + log.warning("An error occurred during snapshot deletion [req=" + req + ']', t); + + return new GridFinishedFuture<>(t); + } + } + + /** */ + private @Nullable String validateAbsoluteSnapshotRoot(@Nullable File path) { + if (path == null) + return null; + + assert path.isAbsolute(); + + var ignWorkRoot = kctx.pdsFolderResolver().fileTree(); + var ignWorkRootStr = kctx.pdsFolderResolver().fileTree().root().getAbsolutePath(); + var pathStr = path.getAbsolutePath(); + + if (pathStr.startsWith(ignWorkRootStr) && !pathStr.startsWith(ignWorkRoot.snapshotsRoot().getAbsolutePath())) + return "belongs to Ignite's working directory"; + + if (!path.exists()) + return SNP_PATH_ERR_PREF + "doesn't exist"; + + if (!path.isDirectory()) + return SNP_PATH_ERR_PREF + "is not a directory"; + + return null; + } + + /** */ + private void reducePhase(UUID reqId, Map<UUID, SnapshotDeleteResponse> results, Map<UUID, Throwable> errors) { + var clusterOpFut = clusterOpFuts.get(reqId); + + if (clusterOpFut == null) + return; + + assert clusterOpFut != null; + + try { + var errP = F.isEmpty(errors) ? null : F.first(errors.entrySet()); + + if (errP != null) { + log.warning("Snapshot deletion finished with an error [reqId=" + reqId + ", nodeId=" + + errP.getKey() + ", err='" + errP.getValue().getMessage() + "']", errP.getValue()); + + clusterOpFut.onDone(errP.getValue()); + + return; + } + + var completedNodes = new ArrayList<UUID>(results.size()); + var uncompletedNodes = new ArrayList<UUID>(results.size()); + var emptyNodes = new ArrayList<UUID>(results.size()); + + results.forEach((nodeId, nodeRes) -> { + if (nodeRes.res != null) { + switch (nodeRes.res) { + case NOT_FOUND: + emptyNodes.add(nodeId); + break; + case DELETED: + completedNodes.add(nodeId); + break; + case PARTLY_DELETED: + uncompletedNodes.add(nodeId); + break; + default: + throw new IgniteIllegalStateException("Unknown snapshot deletion node result, [nodeRes=" + + nodeRes + ", nodeId=" + nodeId + ']'); + } + } + }); + + clusterOpFut.onDone(new SnapshotDeleteProcessResult( + completedNodes.isEmpty() ? null : completedNodes, + uncompletedNodes.isEmpty() ? null : uncompletedNodes, + emptyNodes.isEmpty() ? null : emptyNodes + )); + } + catch (Throwable t) { + clusterOpFut.onDone(t); + } + } + + /** */ + public boolean isSnapshotDeleting(String snpName, @Nullable String snpPath) { + return requests.contains(new SnapshotDeleteRequest(null, snpName, snpPath)); Review Comment: The set is keyed by the raw path string, but `deletePhase` treats an empty path as the default directory and resolves relative paths before deleting. Consequently `deleteSnapshot(name, "")` deletes the default snapshot while `isSnapshotDeleting(name, null)` returns false, allowing a concurrent create/check/restore of that same snapshot to proceed. Normalize the path once before storing and comparing requests. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java: ########## @@ -0,0 +1,325 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.cache.persistence.snapshot; + +import java.io.File; +import java.util.ArrayList; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.ignite.IgniteIllegalStateException; +import org.apache.ignite.IgniteLogger; +import org.apache.ignite.internal.GridKernalContext; +import org.apache.ignite.internal.IgniteInternalFuture; +import org.apache.ignite.internal.NodeStoppingException; +import org.apache.ignite.internal.processors.cache.persistence.filename.SnapshotFileTree; +import org.apache.ignite.internal.util.distributed.DistributedProcess; +import org.apache.ignite.internal.util.future.GridFinishedFuture; +import org.apache.ignite.internal.util.future.GridFutureAdapter; +import org.apache.ignite.internal.util.future.IgniteFutureImpl; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.lang.IgniteFuture; +import org.jetbrains.annotations.Nullable; + +import static org.apache.ignite.internal.processors.rollingupgrade.feature.SupportedFeatureRegistry.SNAPSHOT_DELETE_FEATURE; +import static org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.DELETE_SNAPSHOT; +import static org.apache.ignite.plugin.security.SecurityPermission.ADMIN_SNAPSHOT; + +/** + * Distributed process to delete a cluster snapshot. The operation is rejected if any concurrent snapshot operation is + * active. + */ +public class SnapshotDeleteProcess { + /** Reject operation messages. */ + private static final String OP_REJECT_MSG = "Snapshot deletion was rejected. "; + + /** */ + private static final String SNP_PATH_ERR_PREF = "Provided snapshot path "; + + /** Kernal context. */ + private final GridKernalContext kctx; + + /** Logger. */ + private final IgniteLogger log; + + /** */ + private volatile boolean interrupted; + + /** Cluster-wide operation futures per request id on certain node. */ + private final Map<UUID, GridFutureAdapter<SnapshotDeleteProcessResult>> clusterOpFuts = new ConcurrentHashMap<>(); + + /** Process requests per snapshot name on each server node. */ + private final Set<SnapshotDeleteRequest> requests = ConcurrentHashMap.newKeySet(); + + /** The distributed process. */ + private final DistributedProcess<SnapshotDeleteRequest, SnapshotDeleteResponse> distrProc; + + /** + * @param ctx Kernal context. + */ + public SnapshotDeleteProcess(GridKernalContext ctx) { + this.kctx = ctx; + + log = ctx.log(getClass()); + + distrProc = new DistributedProcess<>(ctx, DELETE_SNAPSHOT, this::deletePhase, this::reducePhase); + } + + /** + * Starts the cluster snapshot delete process. + * + * @param snpName Snapshot name. + * @param snpPath Snapshot directory path (optional). + * @return Future that will be completed when the snapshot is deleted. + */ + public IgniteFuture<SnapshotDeleteProcessResult> start(String snpName, @Nullable String snpPath) { + UUID reqId = UUID.randomUUID(); + + var clusterOpFut = new GridFutureAdapter<SnapshotDeleteProcessResult>(); + + clusterOpFut.listen(fut -> clusterOpFuts.remove(reqId)); + + try { + synchronized (clusterOpFuts) { + if (interrupted || kctx.isStopping()) + throw new NodeStoppingException("Failed to start snapshot delete process: node is stopping."); + + clusterOpFuts.put(reqId, clusterOpFut); + } + + SnapshotDeleteRequest req = new SnapshotDeleteRequest(reqId, snpName, snpPath); + + distrProc.start(reqId, req); + } + catch (Throwable t) { + log.error("Failed to start distributed delete snapshot process [snpName=" + snpName + ", snpPath=" + snpPath + ']', t); + + clusterOpFut.onDone(t); + } + + return new IgniteFutureImpl<>(clusterOpFut); + } + + /** */ + private IgniteInternalFuture<SnapshotDeleteResponse> deletePhase(UUID ignored, SnapshotDeleteRequest req) { + if (interrupted || kctx.isStopping()) { + return new GridFinishedFuture<>(new NodeStoppingException(OP_REJECT_MSG + + " Node is stopping [req=" + req + ']')); + } + + if (kctx.cluster().get().localNode().isClient()) + return new GridFinishedFuture<>(new SnapshotDeleteResponse(null)); + + kctx.security().authorize(ADMIN_SNAPSHOT); + + IgniteSnapshotManager snpMgr = kctx.cache().context().snapshotMgr(); + + var curCreateRq = snpMgr.currentCreateRequest(); + + if (curCreateRq != null && curCreateRq.snpName.equals(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being created [req=" + req + ']')); + } + + if (snpMgr.isRestoring(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being restored [req=" + req + ']')); + } + + if (snpMgr.isSnapshotChecking(req.snpName)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "Snapshot with this name is being checked [req=" + req + ']')); + } + + if (!kctx.rollingUpgrade().features().isActive(SNAPSHOT_DELETE_FEATURE)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG + + "The snapshot deletion feature isn't activated yet [req=" + req + ']')); + } + + File path = null; + + if (!F.isEmpty(req.snpPath)) { + path = new File(req.snpPath); + + if (!path.isAbsolute()) + path = new File(kctx.pdsFolderResolver().fileTree().snapshotsRoot(), req.snpPath); + + String pathValidationErr = validateAbsoluteSnapshotRoot(path); + + if (pathValidationErr != null) { + return new GridFinishedFuture<>(new IllegalArgumentException(OP_REJECT_MSG + + pathValidationErr + " [req=" + req + ']')); + } + } + + try { + if (!requests.add(req)) { + return new GridFinishedFuture<>(new IgniteIllegalStateException("Deletion of the snapshot has already " + + "started [req=" + req + ']')); + } + + GridFutureAdapter<SnapshotDeleteResponse> reqLocFut = new GridFutureAdapter<>(); + + File path0 = path; + + kctx.pools().getSnapshotExecutorService().submit(() -> { + try { + AtomicBoolean foundFlag = new AtomicBoolean(); + + var sft = new SnapshotFileTree(kctx, req.snpName, path0 == null ? null : path0.getAbsolutePath()); + + boolean deleted = snpMgr.deleteLocalSnapshot(sft, foundFlag); + + SnapshotDeleteResponse.SnapshotDeleteStatus res; + + if (foundFlag.get()) { + if (deleted && log.isInfoEnabled()) + log.info("Snapshot successfully deleted [req=" + req + ']'); + else if (!deleted) + log.warning("Snapshot deleted not completely [req=" + req + ']'); + + res = deleted + ? SnapshotDeleteResponse.SnapshotDeleteStatus.DELETED + : SnapshotDeleteResponse.SnapshotDeleteStatus.PARTLY_DELETED; + } + else { + if (log.isInfoEnabled()) + log.info("Snapshot not found to delete [req=" + req + ']'); + + res = SnapshotDeleteResponse.SnapshotDeleteStatus.NOT_FOUND; + } + + reqLocFut.onDone(new SnapshotDeleteResponse(res)); + } + finally { + requests.remove(req); + } + }); + + if (log.isInfoEnabled()) + log.info("Deletion of snapshot initialized [req=" + req + ']'); + + return reqLocFut; + } + catch (Throwable t) { + requests.remove(req); + + log.warning("An error occurred during snapshot deletion [req=" + req + ']', t); + + return new GridFinishedFuture<>(t); + } + } + + /** */ + private @Nullable String validateAbsoluteSnapshotRoot(@Nullable File path) { + if (path == null) + return null; + + assert path.isAbsolute(); + + var ignWorkRoot = kctx.pdsFolderResolver().fileTree(); + var ignWorkRootStr = kctx.pdsFolderResolver().fileTree().root().getAbsolutePath(); + var pathStr = path.getAbsolutePath(); + + if (pathStr.startsWith(ignWorkRootStr) && !pathStr.startsWith(ignWorkRoot.snapshotsRoot().getAbsolutePath())) Review Comment: This lexical prefix check does not ensure that the path is actually below `snapshotsRoot`: values such as `<work>/snapshots/../db` or `<work>/snapshots-other` pass the second condition. `deletePhase` then builds a `SnapshotFileTree` from that path and recursively deletes `<path>/<name>`, so an ADMIN_SNAPSHOT caller can delete arbitrary data under Ignite's work directory. Resolve/canonicalize the path and use a path-segment-aware containment check before allowing it. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java: ########## @@ -701,46 +708,90 @@ public IgniteSnapshotManager(GridKernalContext ctx) { /** * @param snpDir Snapshot dir. */ - public void deleteSnapshot(File snpDir) { + public void deleteLocalSnapshot(File snpDir) { if (!snpDir.exists()) return; if (!snpDir.isDirectory()) return; - deleteSnapshot(new SnapshotFileTree( + var sft = new SnapshotFileTree( cctx.kernalContext(), snpDir.getName(), snpDir.getParent(), ft.folderName(), - pdsSettings.consistentId().toString())); + pdsSettings.consistentId().toString() + ); + + deleteLocalSnapshot(sft); } - /** */ - public void deleteSnapshot(SnapshotFileTree sft) { - try { - U.delete(sft.binaryMeta()); - sft.allStorages().forEach(U::delete); - U.delete(sft.meta()); + /** + * Deletes local shapshot data. Review Comment: The new Javadoc misspells “snapshot”. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java: ########## @@ -1446,6 +1507,27 @@ public boolean isSnapshotChecking(String snpName) { return checkSnpProc.isSnapshotChecking(snpName); } + /** + * @return {@code True} if a snapshot {@code snpName} delete operation is in progress. + */ + public boolean isSnapshotDeleting(String snpName, @Nullable String snpPath) { + return deleteSnpProc.isSnapshotDeleting(snpName, snpPath); + } + + /** + * Deletes the cluster-wide snapshot with the given name. + * <p> + * The operation is rejected if a concurrent snapshot operation (create, restore, check, etc...) is in progress + * for the snapshot. + * + * @param name Snapshot name. + * @param snpPath Snapshot directory path. If {@code null}, the default configured snapshot directory will be used. + * @return Future which will be completed when the snapshot is deleted on all the baseline nodes. Review Comment: The distributed process waits for `serverNodes(topVer)`, not baseline nodes, and `deletePhase` processes every online server. This public API contract is therefore inaccurate when a non-baseline server is online; document that completion covers all online server nodes. ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteRequest.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.processors.cache.persistence.snapshot; + +import java.util.Objects; +import java.util.UUID; +import org.apache.ignite.internal.Order; +import org.apache.ignite.internal.util.typedef.internal.S; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageFactory; +import org.jetbrains.annotations.Nullable; + +/** + * Cluster snapshot delete distributed process request. + * + * @see SnapshotDeleteProcess + */ +public class SnapshotDeleteRequest implements Message { + /** Request ID. */ + @Order(0) + UUID reqId; + + /** Snapshot name. */ + @Order(1) + String snpName; + + /** Snapshot directory path. */ + @Order(2) + @Nullable String snpPath; + + /** Default constructor for {@link MessageFactory}. */ + public SnapshotDeleteRequest() { + // No-op. + } + + /** + * @param reqId Request ID. + * @param snpName Snapshot name. + * @param snpPath Snapshot directory path. + */ + SnapshotDeleteRequest(UUID reqId, String snpName, @Nullable String snpPath) { + this.reqId = reqId; + this.snpName = snpName; + this.snpPath = snpPath; + } + + /** {@inheritDoc} */ + @Override public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) + return false; + + SnapshotDeleteRequest other = (SnapshotDeleteRequest)o; + + return snpName.equals(other.snpName) && Objects.equals(snpPath, other.snpPath); Review Comment: Request identity uses the raw path string, so `null`, the default snapshots root, and an equivalent explicit/relative path are treated as different operations. A second delete—or a create/restore/check using an equivalent path representation—can therefore run against the same files concurrently. Normalize the effective snapshot root before comparing or storing the request identity. ########## modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/AbstractSnapshotSelfTest.java: ########## @@ -283,6 +294,17 @@ public void afterTestSnapshot() throws Exception { cleanPersistenceDir(); } + /** {@inheritDoc} */ + @Override protected void cleanPersistenceDir() throws Exception { + super.cleanPersistenceDir(); + + // Clean all: also separated snapshot working directories and custom snapshot pathes. Review Comment: The new test cleanup comment misspells “paths”. ########## modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java: ########## @@ -0,0 +1,235 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.util; + +import java.io.File; +import java.nio.file.DirectoryStream; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Collection; +import org.apache.ignite.IgniteDataStreamer; +import org.apache.ignite.configuration.IgniteConfiguration; +import org.apache.ignite.internal.IgniteEx; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.internal.util.typedef.internal.U; +import org.apache.ignite.testframework.GridTestUtils; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +import static java.nio.file.Files.newDirectoryStream; +import static org.apache.ignite.cluster.ClusterState.ACTIVE; +import static org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK; +import static org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp; +import static org.apache.ignite.testframework.GridTestUtils.waitForCondition; +import static org.junit.Assume.assumeTrue; + +/** Test for the command '--snapshot delete'. */ +@RunWith(Parameterized.class) +public class GridCommandHandlerDeleteSnapshotTest extends GridCommandHandlerAbstractTest { + /** Value: -1 - do not use, 1 - server node, 0 - client node. */ + @Parameter(1) + public int extraNodeIsServer = -1; + + /** */ + @Parameter(2) + public boolean addIncrements; + + /** */ + @Parameter(3) + public boolean changeBaseline; + + /** */ + @Parameter(4) + public boolean customPath; + + /** */ + @Parameter(5) + public boolean separatedWorkDir; + + /** */ + @Parameters(name = "client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}") + public static Collection<?> parameters() { + return GridTestUtils.cartesianProduct( + commandHandlers(), + F.asList(-1, 1, 0), // Use extra node (do not use at all, server node, client node); + F.asList(false, true), // Add increments to the test snapshot; + F.asList(false, true), // Change baseline; + F.asList(false, true), // Use custom snapshot path; + F.asList(true, false) // Separated (own) work directory. + ); + } + + /** {@inheritDoc} */ + @Override protected void afterTest() throws Exception { + super.afterTest(); + + stopAllGrids(); + } + + /** {@inheritDoc} */ + @Override protected void beforeTest() throws Exception { + super.beforeTest(); + + /** Handy if test running is interrupted and {@link #afterTest()} isn't invoked. */ + cleanPersistenceDir(); + } + + /** {@inheritDoc} */ + @Override protected void cleanPersistenceDir() throws Exception { + super.cleanPersistenceDir(); + + // Also cleans separated snapshot working directories and custom snapshot pacthes. Review Comment: The new test cleanup comment misspells “patches”. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
