Copilot commented on code in PR #13577:
URL: https://github.com/apache/ignite/pull/13577#discussion_r4155392270
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:
##########
@@ -701,46 +708,134 @@ 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(
+ SnapshotFileTree sft = new SnapshotFileTree(
cctx.kernalContext(),
snpDir.getName(),
snpDir.getParent(),
ft.folderName(),
- pdsSettings.consistentId().toString()));
+ pdsSettings.consistentId().toString()
+ );
+
+ deleteLocalSnapshot(sft, true, false);
}
- /** */
- public void deleteSnapshot(SnapshotFileTree sft) {
+ /**
+ * TODO : revise the incremental snapshots parts in the scoped case
https://issues.apache.org/jira/browse/IGNITE-29095
+ *
+ * Tries to delete local snapshot data.
+ *
+ * @param sft Snapshot file tree.
+ * @param ignoreErrs If {@code true}, logs possible deletion exceptions
and marks the result as not deleted.
+ * @param scoped If {@code true}, deletes only node's data and deletes
shared snapshot directories only if they are empty.
+ * Otherwise, completely deletes all the snapshot data.
+ * @return A pair of {@code boolean} values. The first indicates whether
snapshot was completely deleted. The second
+ * indicates whether the snapshot was found.
+ */
+ public T2<Boolean, Boolean> deleteLocalSnapshot(SnapshotFileTree sft,
boolean scoped, boolean ignoreErrs) {
+ T2<Boolean, Boolean> res = new T2<>(false, false);
+
+ List<File> allStorages = sft.allStorages().toList();
+
+ if (sft.root().exists())
+ res.set2(true);
+ else {
+ for (File storage : allStorages) {
+ if (storage.exists()) {
+ res.set2(true);
+
+ break;
+ }
+ }
+ }
+
+ // Not found at all - nothing to delete.
+ if (!res.get2())
+ return res;
+
+ // Assume we'll succeed.
+ res.set1(true);
+
+ // The 'exists' checks are for a concurrent deletion when nodes share
their working and snapshot directories.
+ // Nodes may steal removal jobs and the files aren't synchronized.
There are gaps between and `exists()` and `delete()`.
+ // We try to delete first. If snapshot data wasn't deleted because it
doesn't already exist is not a removal error here.
try {
- U.delete(sft.binaryMeta());
- sft.allStorages().forEach(U::delete);
- U.delete(sft.meta());
+ if (!sft.meta().delete() && sft.meta().exists())
Review Comment:
The metadata file is deleted before the snapshot data. When a later file or
directory cannot be removed, this returns a partial result but leaves the
node's data without the metadata needed to discover it on a retry; this is
especially reproducible with dedicated work directories. Keep the metadata
until all data cleanup succeeds, or otherwise preserve it whenever deletion is
partial.
This issue also appears in the following locations of the same file:
- line 848
- line 2005
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:
##########
@@ -0,0 +1,415 @@
+/*
+ * 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.io.IOException;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.ignite.IgniteIllegalStateException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.cluster.ClusterNode;
+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.typedef.F;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteReducer;
+import org.jetbrains.annotations.Nullable;
+
+import static
org.apache.ignite.internal.processors.rollingupgrade.feature.CoreFeatureRegistry.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. ";
+
+ /** */
+ public static final String OP_REJECT_FEATURE_MSG = OP_REJECT_MSG + "The
snapshot deletion feature isn't activated yet.";
+
+ /** */
+ private static final String CONCURRENT_OP_PREF = "Snapshot with the same
name is being ";
+
+ /** */
+ public static final String BEING_CREATED_PREF = CONCURRENT_OP_PREF +
"created ";
+
+ /** */
+ public static final String BEING_RESTORED_PREF = CONCURRENT_OP_PREF +
"restored ";
+
+ /** */
+ public static final String BEING_CHECKED_PREF = CONCURRENT_OP_PREF +
"checked ";
+
+ /** 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<>();
+
+ /** Current operations represented by the full canonical path. */
+ private final Set<File> requests = ConcurrentHashMap.newKeySet();
+
+ /** The distributed process. */
+ private final DistributedProcess<SnapshotDeleteRequest,
SnapshotDeleteResponse> distrProc;
+
+ /**
+ * @param ctx Kernal context.
+ */
+ public SnapshotDeleteProcess(GridKernalContext ctx) {
+ 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 IgniteInternalFuture<SnapshotDeleteProcessResult> start(String
snpName, @Nullable String snpPath) {
+ GridFutureAdapter<SnapshotDeleteProcessResult> clusterOpFut = new
GridFutureAdapter<>();
+
+ if
(!kctx.rollingUpgrade().features().isActive(SNAPSHOT_DELETE_FEATURE)) {
+ clusterOpFut.onDone(new
IgniteIllegalStateException(OP_REJECT_FEATURE_MSG));
+
+ return clusterOpFut;
+ }
+
+ UUID reqId = UUID.randomUUID();
+
+ clusterOpFut.listen(fut -> clusterOpFuts.remove(reqId));
+
+ try {
+ 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 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());
+
+ kctx.security().authorize(ADMIN_SNAPSHOT);
+
+ IgniteSnapshotManager snpMgr = kctx.cache().context().snapshotMgr();
+
+ SnapshotOperationRequest curCreateRq = snpMgr.currentCreateRequest();
+
+ if (curCreateRq != null &&
curCreateRq.snpName.equalsIgnoreCase(req.snpName)) {
+ return new GridFinishedFuture<>(new
IgniteIllegalStateException(OP_REJECT_MSG + BEING_CREATED_PREF +
+ "[req=" + req + ']'));
+ }
+
+ if (snpMgr.isRestoring(req.snpName)) {
+ return new GridFinishedFuture<>(new
IgniteIllegalStateException(OP_REJECT_MSG + BEING_RESTORED_PREF +
+ "[req=" + req + ']'));
+ }
+
+ if (snpMgr.isSnapshotChecking(req.snpName)) {
+ return new GridFinishedFuture<>(new
IgniteIllegalStateException(OP_REJECT_MSG + BEING_CHECKED_PREF +
+ "[req=" + req + ']'));
+ }
+
+ File resolvedFullPath = null;
+
+ // Future to delete snapshot contents according to snapshot metadatas.
+ GridFutureAdapter<SnapshotDeleteResponse> resultFut = new
GridFutureAdapter<>();
+
+ try {
+ resolvedFullPath = resolveFullPath(req.snpName, req.snpPath);
+
+ if (!requests.add(resolvedFullPath)) {
+ return new GridFinishedFuture<>(new
IgniteIllegalStateException("Deletion of the snapshot has already " +
+ "started [req=" + req + ']'));
+ }
+
+ File rootPath = resolvedFullPath.getParentFile();
+
+ SnapshotFileTree snpFiles = new SnapshotFileTree(kctx,
req.snpName, rootPath.getAbsolutePath());
+
+ // We need to find and read snapshot metas to ensure the content
is a snapshot. Also, the metas contain
+ // initial cluster topology and actual snapshot folder names.
+ List<SnapshotMetadata> locMetas =
kctx.cache().context().snapshotMgr().readSnapshotMetadatas(snpFiles, false);
Review Comment:
Passing `false` makes unreadable or mismatched metadata files get logged and
skipped. If a snapshot has multiple metadata files and one is corrupt, the
remaining metadata still drives deletion, so the command can remove part of a
snapshot despite its safety contract that invalid metadata prevents deletion.
Use the fail-fast overload here so no deletion is scheduled unless all metadata
can be read.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:
##########
@@ -1478,8 +1605,11 @@ public boolean isRestoring() {
* @param snpName Snapshot name.
* @return {@code True} if the snapshot restore operation from the
specified snapshot is in progress locally.
*/
- public boolean isRestoring(String snpName) {
- return snpName.equals(restoreCacheGrpProc.restoringSnapshotName());
+ public boolean isRestoring(@Nullable String snpName) {
+ if (snpName == null)
+ return false;
+
+ return
snpName.equalsIgnoreCase(restoreCacheGrpProc.restoringSnapshotName());
Review Comment:
`isRestoring` now compares names case-insensitively for every filesystem. On
a case-sensitive filesystem, a restore of `Foo` therefore blocks deletion of
the distinct snapshot `foo`; snapshot task identity elsewhere remains
case-sensitive. Use filesystem-aware/path-aware comparison instead of
unconditional `equalsIgnoreCase`.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotProcessor.java:
##########
@@ -138,7 +138,8 @@ void process(
WALRecord rec = walRec.getValue();
- if (rec.type() == CLUSTER_SNAPSHOT &&
((ClusterSnapshotRecord)rec).clusterSnapshotName().equals(sft.name())) {
+ // A filesystem might not support the character case of
directory or file name.
+ if (rec.type() == CLUSTER_SNAPSHOT &&
((ClusterSnapshotRecord)rec).clusterSnapshotName().equalsIgnoreCase(sft.name()))
{
Review Comment:
`equalsIgnoreCase` conflates distinct snapshot names on case-sensitive
filesystems: the rest of the snapshot lifecycle treats names such as `Foo` and
`foo` as distinct, so both can have WAL records. The incremental scan can stop
at the first record with either casing and attach the increment to the wrong
snapshot. Apply case-insensitive matching only when the underlying snapshot
filesystem is case-insensitive, while retaining exact matching otherwise.
##########
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommandArg.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.management.snapshot;
+
+import org.apache.ignite.internal.Order;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.Positional;
+import org.jetbrains.annotations.Nullable;
+
+/** */
+public class SnapshotDeleteCommandArg extends IgniteDataTransferObject {
+ /** */
+ private static final long serialVersionUID = 0;
+
+ /** */
+ @Order(0)
+ @Positional
+ @Argument(description = "Snapshot name")
+ @Nullable String snapshotName;
+
+ /** */
+ @Order(1)
+ @Argument(example = "path/to/snapshots", optional = true, description =
"Path to snapshot location directory. If not specified " +
+ "or specified a relative path, the default snapshot configuration
directory will be used")
Review Comment:
`src` is forwarded unchanged to `SnapshotFileTree`, whose `new File(path,
name)` construction resolves a relative path against each node's JVM working
directory, not the node's configured snapshot directory. Thus the documented
relative `--src` behavior targets the wrong location; normalize relative paths
against the configured snapshot/work directory or correct the contract and help
text.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/rollingupgrade/feature/CoreFeatureRegistry.java:
##########
@@ -93,4 +93,7 @@
public class CoreFeatureRegistry {
/** */
public static final IgniteFeature ROLLING_UPGRADE_FEATURE = new
IgniteCoreFeature(0);
+
+ /** */
+ public static final IgniteFeature SNAPSHOT_DELETE_FEATURE = new
IgniteCoreFeature(1);
Review Comment:
Feature ID 1 is already used by
`TestIgniteReleaseFeatures_2_19_2.VER_2_19_2_ID_1_FEATURE`
(modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_2.java:26),
but this assigns the same ID to the snapshot-delete feature introduced in
2.19.1. Rolling-upgrade feature sets compare IDs, so this changes the meaning
of an existing ID and can activate/deactivate the wrong behavior; the release
feature fixtures and IDs need to be reconciled before adding this feature.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotCheckProcess.java:
##########
@@ -487,12 +489,17 @@ private IgniteInternalFuture<SnapshotCheckResponse>
prepareAndCheckMetas(UUID ig
if (nodeStopping)
return new GridFinishedFuture<>(new NodeStoppingException("The
node is stopping: " + kctx.localNodeId()));
- ctx = contexts.computeIfAbsent(req.snapshotName(), snpName -> new
SnapshotCheckContext(req));
+ ctx =
contexts.computeIfAbsent(req.snapshotName().toLowerCase(Locale.ROOT), snpName
-> new SnapshotCheckContext(req));
Review Comment:
The context key is lowercased unconditionally, so on a case-sensitive
filesystem snapshots named `Foo` and `foo` are treated as the same validation
operation even though the snapshot lifecycle uses exact names and both
directories can coexist. A check of one snapshot can therefore reject a check
of the other; key by the actual snapshot path or apply case folding only when
the filesystem is case-insensitive.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:
##########
@@ -0,0 +1,415 @@
+/*
+ * 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.io.IOException;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.ignite.IgniteIllegalStateException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.cluster.ClusterNode;
+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.typedef.F;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteReducer;
+import org.jetbrains.annotations.Nullable;
+
+import static
org.apache.ignite.internal.processors.rollingupgrade.feature.CoreFeatureRegistry.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. ";
+
+ /** */
+ public static final String OP_REJECT_FEATURE_MSG = OP_REJECT_MSG + "The
snapshot deletion feature isn't activated yet.";
+
+ /** */
+ private static final String CONCURRENT_OP_PREF = "Snapshot with the same
name is being ";
+
+ /** */
+ public static final String BEING_CREATED_PREF = CONCURRENT_OP_PREF +
"created ";
+
+ /** */
+ public static final String BEING_RESTORED_PREF = CONCURRENT_OP_PREF +
"restored ";
+
+ /** */
+ public static final String BEING_CHECKED_PREF = CONCURRENT_OP_PREF +
"checked ";
+
+ /** 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<>();
+
+ /** Current operations represented by the full canonical path. */
+ private final Set<File> requests = ConcurrentHashMap.newKeySet();
+
+ /** The distributed process. */
+ private final DistributedProcess<SnapshotDeleteRequest,
SnapshotDeleteResponse> distrProc;
+
+ /**
+ * @param ctx Kernal context.
+ */
+ public SnapshotDeleteProcess(GridKernalContext ctx) {
+ 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 IgniteInternalFuture<SnapshotDeleteProcessResult> start(String
snpName, @Nullable String snpPath) {
+ GridFutureAdapter<SnapshotDeleteProcessResult> clusterOpFut = new
GridFutureAdapter<>();
+
+ if
(!kctx.rollingUpgrade().features().isActive(SNAPSHOT_DELETE_FEATURE)) {
+ clusterOpFut.onDone(new
IgniteIllegalStateException(OP_REJECT_FEATURE_MSG));
+
+ return clusterOpFut;
+ }
+
+ UUID reqId = UUID.randomUUID();
+
+ clusterOpFut.listen(fut -> clusterOpFuts.remove(reqId));
+
+ try {
+ 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 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());
+
+ kctx.security().authorize(ADMIN_SNAPSHOT);
+
+ IgniteSnapshotManager snpMgr = kctx.cache().context().snapshotMgr();
+
+ SnapshotOperationRequest curCreateRq = snpMgr.currentCreateRequest();
+
+ if (curCreateRq != null &&
curCreateRq.snpName.equalsIgnoreCase(req.snpName)) {
Review Comment:
This case-insensitive comparison rejects deleting `foo` while `Foo` is being
created even on a case-sensitive filesystem, where those are distinct snapshot
directories and snapshot tasks compare names exactly. Use the same
filesystem-aware/path-aware identity rule as snapshot creation so unrelated
case-distinct snapshots do not block each other.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java:
##########
@@ -0,0 +1,931 @@
+/*
+ * 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.io.RandomAccessFile;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Random;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Supplier;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteIllegalStateException;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.TestRecordingCommunicationSpi;
+import org.apache.ignite.internal.processors.cache.persistence.file.FileIO;
+import
org.apache.ignite.internal.processors.cache.persistence.file.RandomAccessFileIOFactory;
+import
org.apache.ignite.internal.processors.cache.persistence.filename.SnapshotFileTree;
+import org.apache.ignite.internal.util.distributed.DistributedProcess;
+import org.apache.ignite.internal.util.distributed.SingleNodeMessage;
+import org.apache.ignite.internal.util.future.IgniteFutureImpl;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.G;
+import org.apache.ignite.internal.util.typedef.T2;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteFuture;
+import org.apache.ignite.plugin.AbstractTestPluginProvider;
+import org.apache.ignite.plugin.PluginContext;
+import org.jetbrains.annotations.Nullable;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.CHECK_SNAPSHOT_METAS;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.CHECK_SNAPSHOT_PARTS;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.DELETE_SNAPSHOT;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.END_SNAPSHOT;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.RESTORE_CACHE_GROUP_SNAPSHOT_PREPARE;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.RESTORE_CACHE_GROUP_SNAPSHOT_ROLLBACK;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.RESTORE_CACHE_GROUP_SNAPSHOT_START;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.RESTORE_INCREMENTAL_SNAPSHOT_START;
+import static
org.apache.ignite.internal.util.distributed.DistributedProcess.DistributedProcessType.START_SNAPSHOT;
+import static
org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeTrue;
+
+/** */
+public class IgniteClusterSnapshotDeleteTest extends AbstractSnapshotSelfTest {
+ /** */
+ private static final int CACHE_KEYS_RANGE = 10;
+
+ /** */
+ private static final int INC_CACHE_KEYS_RANGE = 15;
+
+ /** Extra storage path. */
+ private static final String EXT_STORAGE_PATH = "extStorage";
+
+ /** */
+ private static boolean posixPermissions;
+
+ /** */
+ private boolean separatedWorkDir;
+
+ /** */
+ private boolean extraStorages;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental = true;
+
+ /** */
+ private @Nullable String cstIdSuffix;
+
+ /** */
+ private @Nullable String[] extStoragePaths;
+
+ /** Parameters. */
+ @Parameterized.Parameters(name = "encryption={0}, onlyPrimary={1},
incremental={2}")
+ public static Collection<?> runParams() {
+ /** Use {@link #incremental} only. */
+ return F.asList(
+ new Object[] {false, false, false},
+ new Object[] {false, false, true}
+ );
+ }
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String
igniteInstanceName) throws Exception {
+ var cfg = super.getConfiguration(igniteInstanceName);
+
+ String workDir = separatedWorkDir
+ ? new File(U.defaultWorkDirectory(),
igniteInstanceName).getAbsolutePath()
+ : U.defaultWorkDirectory();
+
+ cfg.setWorkDirectory(workDir);
+
+ if (cstIdSuffix != null)
+ cfg.setConsistentId(cfg.getConsistentId().toString() + '_' +
cstIdSuffix);
+
+ if (extraStorages) {
+ cfg.getDataStorageConfiguration().setExtraStoragePaths(
+ workDir + File.separator,
+ workDir + File.separator + EXT_STORAGE_PATH
+ );
+
+ extStoragePaths =
cfg.getDataStorageConfiguration().getExtraStoragePaths();
+
+ cfg.getDataStorageConfiguration().setExtraSnapshotPaths("",
EXT_STORAGE_PATH);
+ }
+
+ return cfg;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void afterTestSnapshot() throws Exception {
+ super.afterTestSnapshot();
+
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override public void beforeTestSnapshot() throws Exception {
+ super.beforeTestSnapshot();
+
+ /** Handy if test running is interrupted and {@link
#afterTestSnapshot()} isn't invoked. */
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ super.beforeTestsStarted();
+
+ File workDir = new File(U.defaultWorkDirectory());
+
+ assertTrue(workDir.exists());
+
+ Path workPath = workDir.toPath();
+
+ try {
+ Files.getPosixFilePermissions(workPath);
+
+ posixPermissions = true;
+ }
+ catch (UnsupportedOperationException ignored) {
+ // No-op.
+ }
+ }
+
+ /** */
+ @Test
+ public void testDeniedPermissions() throws Exception {
+ assumeTrue(posixPermissions);
+
+ // Doesn't matter here.
+ assumeFalse(incremental);
+
+ separatedWorkDir = true;
+
+ AtomicReference<Set<PosixFilePermission>> prevPerms = new
AtomicReference<>();
+ AtomicReference<Path> pathRef = new AtomicReference<>();
+
+ pluginProvider = new AbstractTestPluginProvider() {
+ @Override public String name() {
+ return "TestSnpMgrProvider";
+ }
+
+ @Override public <T> T createComponent(PluginContext ctx, Class<T>
cls) {
+ if (IgniteSnapshotManager.class.isAssignableFrom(cls)) {
+ return (T)new
IgniteSnapshotManager(((IgniteEx)ctx.grid()).context()) {
+ @Override public T2<Boolean, Boolean>
deleteLocalSnapshot(
+ SnapshotFileTree sft,
+ boolean scoped,
+ boolean ignoreErrs
+ ) {
+ if
(ctx.localNode().id().equals(grid(1).localNode().id())) {
+ Path path = sft.root().toPath();
+
+ pathRef.set(path);
+
+ try {
+
prevPerms.set(Files.getPosixFilePermissions(path));
+
+ // Denies writing (deletion).
+ Files.setPosixFilePermissions(path,
PosixFilePermissions.fromString("r-xr-x---"));
+ }
+ catch (Exception e) {
+ throw new IgniteException("Unable to set
the posix permissions.", e);
+ }
+ }
+
+ return super.deleteLocalSnapshot(sft, scoped,
ignoreErrs);
+ }
+ };
+ }
+
+ return super.createComponent(ctx, cls);
+ }
+ };
+
+ startGridsWithCache(3, CACHE_KEYS_RANGE, i -> i, dfltCacheCfg);
+
+ snp(grid(0)).createSnapshot(SNAPSHOT_NAME).get(getTestTimeout());
+
+ try {
+ SnapshotDeleteProcessResult res =
snp(grid(0)).deleteSnapshot(SNAPSHOT_NAME, null).get(getTestTimeout());
+
+ assertEquals(1, res.uncompletedNodes().size());
+ assertEquals(2, res.completedNodes().size());
+ }
+ finally {
+ if (pathRef.get() != null && pathRef.get() != null)
+ Files.setPosixFilePermissions(pathRef.get(), prevPerms.get());
Review Comment:
The cleanup guard checks `pathRef` twice and never verifies that `prevPerms`
was captured. If permission discovery fails after `pathRef.set`, the `finally`
block passes null permissions to `setPosixFilePermissions` and masks the
original failure. Check `prevPerms.get()` in the second condition.
--
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]
