zstan commented on code in PR #13577: URL: https://github.com/apache/ignite/pull/13577#discussion_r4093860710
########## docs/_docs/snapshots/snapshots.adoc: ########## @@ -287,6 +287,51 @@ control.(sh|bat) --snapshot restore snapshot_09062021 --groups cache-group1,cach control.(sh|bat) --snapshot restore snapshot_09062021 --increment 1 ---- +== Deleting Snapshot + +You can delete a snapshot using the `control.sh|bat` script. + +The deletion is performed on all *online* server nodes of the cluster. +[NOTE] +==== +The snapshot integrity, topology and correctness aren't checked. Snapshot data on offline server nodes aren't deleted. +==== + +[tabs] +-- +tab:Unix[] +[source,shell] +---- +# Delete the snapshot "snapshot_09062021". +control.sh --snapshot delete snapshot_09062021 + +# Delete the snapshot "snapshot_09062021" located in the "/tmp/ignite/snapshots" folder. +control.sh --snapshot delete snapshot_09062021 --src /tmp/ignite/snapshots +---- + +tab:Windows[] +[source,shell] +---- +# Delete the snapshot "snapshot_09062021". +control.bat --snapshot delete snapshot_09062021 + +# Delete the snapshot "snapshot_09062021" located in the "/tmp/ignite/snapshots" folder. +control.bat --snapshot delete snapshot_09062021 --src /tmp/ignite/snapshots +---- +-- + +=== Delete operation limitations + +The delete operation is subject to the following limitations: + +* The deletion is rejected if any snapshot operation (create, restore, check, delete) is active for the snapshot. +* The operation requires the snapshot administration permissions via `IgniteSecurity` (if configured). +* The operation cannot be undone and the deleted snapshot cannot be restored. The command prompts for a confirmation. +* Before deletion, no snapshot validation is done except finding and reading its metadata. If the metadata isn't found +or cannot be read, snapshot isn't deleted. +* If the provided snapshot path belongs to Ignite's home or to Ignite's a non-snapshot work directory, the operation fails. Review Comment: now my comment move to this line, no need additional mention here and code checks. ########## modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommandArg.java: ########## @@ -0,0 +1,61 @@ +/* + * 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; + +/** */ +public class SnapshotDeleteCommandArg extends IgniteDataTransferObject { + /** */ + private static final long serialVersionUID = 0; + + /** */ + @Order(0) + @Positional + @Argument(description = "Snapshot name") + String snapshotName; + + /** */ + @Order(1) + @Argument(example = "path", optional = true, description = "Path to snapshot location directory. If not specified " + + "or specified a relative path, the default snapshot configuration directory will be used") + String src; + + /** */ + public String snapshotName() { + return snapshotName; Review Comment: It\`s not about arg processing, it about `@Nullable` annotation ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java: ########## @@ -701,46 +708,108 @@ 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, null); } - /** */ - public void deleteSnapshot(SnapshotFileTree sft) { + /** + * Tries to delete local snapshot data. + * + * @param sft Snapshot file tree. + * @param existsFlag Flag to set {@code true} if any snapshot file or directory was found (existed). If {@code null}, ignored. + * @return {@code True}, if data is found and completely deleted; + * {@code False}, if nothing found or if data is found but might not be deleted completely. + */ + public boolean deleteLocalSnapshot(SnapshotFileTree sft, @Nullable AtomicBoolean existsFlag) { + var exFlag0 = new AtomicBoolean(); + + sft.allStorages().forEach(s -> { + if (s.exists()) + exFlag0.set(true); + }); + + if (sft.root().exists()) + exFlag0.set(true); + + if (existsFlag != null) + existsFlag.set(exFlag0.get()); + + // Nothing to delete. + if (!exFlag0.get()) + return false; + + boolean res = true; + try { - U.delete(sft.binaryMeta()); - sft.allStorages().forEach(U::delete); - U.delete(sft.meta()); + if (sft.binaryMeta().exists() && !U.delete(sft.binaryMeta()) && sft.binaryMeta().exists()) + res = false; Review Comment: overcomplicated, you need to use directory lock, check `public static class NodeFileLockHolder extends FileLockHolder` ########## modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommand.java: ########## @@ -0,0 +1,133 @@ +/* + * 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 java.util.Collection; +import java.util.Map; +import java.util.UUID; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcess; +import org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcessResult; +import org.apache.ignite.internal.processors.rollingupgrade.feature.SupportedFeatureRegistry; +import org.apache.ignite.internal.util.typedef.internal.U; + +/** + * Snapshot deletion command. + * + * @see SupportedFeatureRegistry#SNAPSHOT_DELETE_FEATURE + * @see SnapshotDeleteProcess + */ +public class SnapshotDeleteCommand extends AbstractSnapshotCommand<SnapshotDeleteCommandArg, SnapshotDeleteProcessResult> { + /** */ + public static final String DESC = "Deletes snapshot and all its incrementals from all the online server nodes"; + + /** */ + public static final String UNSURED_DELETION_PREF = "WARNING: the following nodes found snapshot data but might not " + + "remove it completely "; + + /** */ + public static final String REMOVED_PREF = "Snapshot removal is completed on "; + + /** */ + public static final String NODE_NOT_FOUND_PREF = "NOTE: the following nodes can't find any snapshot data, " + + "operation skipped "; + + /** */ + public static final String NOT_FOUND_PREF = "Snapshot not found on current server nodes "; + + /** */ + public static final String MISSING_BASELINES = "WARNING: the snapshot's baseline nodes with the following consistent " + + "ids are missing in current cluster "; + + /** + * {@inheritDoc} + */ + @Override public String description() { + return DESC; + } + + /** {@inheritDoc} */ + @Override public Class<SnapshotDeleteCommandArg> argClass() { + return SnapshotDeleteCommandArg.class; + } + + /** {@inheritDoc} */ + @Override public Class<SnapshotDeleteTask> taskClass() { + return SnapshotDeleteTask.class; + } + + /** {@inheritDoc} */ + @Override public void printResult(SnapshotDeleteCommandArg arg, SnapshotDeleteProcessResult res, Consumer<String> printer) { + boolean found = false; + + if (!res.uncompletedNodes().isEmpty()) { + found = true; + + printer.accept(UNSURED_DELETION_PREF + nodeIdPairsStrLst(res.uncompletedNodes())); + + printer.accept(""); + } + + if (!res.completedNodes().isEmpty()) { + found = true; + + printer.accept(REMOVED_PREF + nodeIdPairsStrLst(res.completedNodes())); + printer.accept(""); + } + + if (found) { + if (!res.emptyNodes().isEmpty()) + printer.accept(NODE_NOT_FOUND_PREF + nodeIdPairsStrLst(res.emptyNodes())); + + if (!res.absentBaselines().isEmpty()) + printer.accept(MISSING_BASELINES + nodeIdsStrLst(res.absentBaselines())); + } + else { + assert !res.emptyNodes().isEmpty(); + + printer.accept(NOT_FOUND_PREF); + } + } + + /** */ + private static String nodeIdPairsStrLst(Map<UUID, String> uuids) { + return "[cnt=" + uuids.size() + "]: " + uuids.entrySet().stream() + .map(e -> e.getValue() + " [uuid=" + e.getKey() + ']') + .collect(Collectors.joining(", ")); + } + + /** */ + private static String nodeIdsStrLst(Collection<String> uuids) { + return "[cnt=" + uuids.size() + "]: " + String.join(", ", uuids); + } + + /** {@inheritDoc} */ + @Override public String confirmationPrompt(SnapshotDeleteCommandArg arg) { + return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals." + + U.nl() + U.nl() + + "If the security is enabled, the operation requires the snapshot administration permissions." + + U.nl() + U.nl() + + "Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " + Review Comment: Too complex for understanding, also incorrect: snapshots Ignite's directories -> Ignite snapshot directories seems my variant still more readable ########## modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotSelfTest.java: ########## @@ -615,6 +615,43 @@ public void testSnapshotExistsException() throws Exception { waitForEvents(EVT_CLUSTER_SNAPSHOT_STARTED, EVT_CLUSTER_SNAPSHOT_FAILED); } + /** + * Tests that snapshot create detects concurrent deletion, or detects still existing snapshot or successfully + * proceeds if snapshot already deleted. + */ + @Test + public void testConcurrentSnapshotDeleteOperation() throws Exception { Review Comment: From java doc: "Tests that snapshot create detects concurrent deletion", but i see that it tests only already existing snap ? Seems this test is not cover conc deletion opertion, other conc tests need to be re checked too ########## modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommand.java: ########## @@ -0,0 +1,133 @@ +/* + * 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 java.util.Collection; +import java.util.Map; +import java.util.UUID; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcess; +import org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcessResult; +import org.apache.ignite.internal.processors.rollingupgrade.feature.SupportedFeatureRegistry; +import org.apache.ignite.internal.util.typedef.internal.U; + +/** + * Snapshot deletion command. + * + * @see SupportedFeatureRegistry#SNAPSHOT_DELETE_FEATURE + * @see SnapshotDeleteProcess + */ +public class SnapshotDeleteCommand extends AbstractSnapshotCommand<SnapshotDeleteCommandArg, SnapshotDeleteProcessResult> { + /** */ + public static final String DESC = "Deletes snapshot and all its incrementals from all the online server nodes"; + + /** */ + public static final String UNSURED_DELETION_PREF = "WARNING: the following nodes found snapshot data but might not " + + "remove it completely "; + + /** */ + public static final String REMOVED_PREF = "Snapshot removal is completed on "; + + /** */ + public static final String NODE_NOT_FOUND_PREF = "NOTE: the following nodes can't find any snapshot data, " + + "operation skipped "; + + /** */ + public static final String NOT_FOUND_PREF = "Snapshot not found on current server nodes "; + + /** */ + public static final String MISSING_BASELINES = "WARNING: the snapshot's baseline nodes with the following consistent " + + "ids are missing in current cluster "; + + /** + * {@inheritDoc} + */ + @Override public String description() { + return DESC; + } + + /** {@inheritDoc} */ + @Override public Class<SnapshotDeleteCommandArg> argClass() { + return SnapshotDeleteCommandArg.class; + } + + /** {@inheritDoc} */ + @Override public Class<SnapshotDeleteTask> taskClass() { + return SnapshotDeleteTask.class; + } + + /** {@inheritDoc} */ + @Override public void printResult(SnapshotDeleteCommandArg arg, SnapshotDeleteProcessResult res, Consumer<String> printer) { + boolean found = false; + + if (!res.uncompletedNodes().isEmpty()) { + found = true; + + printer.accept(UNSURED_DELETION_PREF + nodeIdPairsStrLst(res.uncompletedNodes())); + + printer.accept(""); + } + + if (!res.completedNodes().isEmpty()) { + found = true; + + printer.accept(REMOVED_PREF + nodeIdPairsStrLst(res.completedNodes())); + printer.accept(""); + } + + if (found) { + if (!res.emptyNodes().isEmpty()) + printer.accept(NODE_NOT_FOUND_PREF + nodeIdPairsStrLst(res.emptyNodes())); + + if (!res.absentBaselines().isEmpty()) + printer.accept(MISSING_BASELINES + nodeIdsStrLst(res.absentBaselines())); + } + else { + assert !res.emptyNodes().isEmpty(); + + printer.accept(NOT_FOUND_PREF); + } + } + + /** */ + private static String nodeIdPairsStrLst(Map<UUID, String> uuids) { + return "[cnt=" + uuids.size() + "]: " + uuids.entrySet().stream() + .map(e -> e.getValue() + " [uuid=" + e.getKey() + ']') + .collect(Collectors.joining(", ")); + } + + /** */ + private static String nodeIdsStrLst(Collection<String> uuids) { + return "[cnt=" + uuids.size() + "]: " + String.join(", ", uuids); + } + + /** {@inheritDoc} */ + @Override public String confirmationPrompt(SnapshotDeleteCommandArg arg) { + return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals." + + U.nl() + U.nl() + + "If the security is enabled, the operation requires the snapshot administration permissions." + + U.nl() + U.nl() + + "Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " + + "metadata are prohibited." + + U.nl() + U.nl() + + "The operation cannot be reverted." + + U.nl() + U.nl() + + "NOTE: Snapshot data on offline server nodes will remain untouched."; + } Review Comment: Copilot found the same- fix it ! ########## modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommandArg.java: ########## @@ -0,0 +1,61 @@ +/* + * 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; + +/** */ +public class SnapshotDeleteCommandArg extends IgniteDataTransferObject { + /** */ + private static final long serialVersionUID = 0; + + /** */ + @Order(0) + @Positional + @Argument(description = "Snapshot name") + String snapshotName; + + /** */ + @Order(1) + @Argument(example = "path", optional = true, description = "Path to snapshot location directory. If not specified " + + "or specified a relative path, the default snapshot configuration directory will be used") + String src; + + /** */ + public String snapshotName() { + return snapshotName; + } + + /** */ + public void snapshotName(String snapshotName) { + this.snapshotName = snapshotName; + } + + /** */ + public String src() { + return src; Review Comment: I not aware about other commands and I not reviewed them, if it possible to return null - plz annotate it correctly ########## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java: ########## @@ -701,46 +708,108 @@ 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, null); } - /** */ - public void deleteSnapshot(SnapshotFileTree sft) { + /** + * Tries to delete local snapshot data. + * + * @param sft Snapshot file tree. + * @param existsFlag Flag to set {@code true} if any snapshot file or directory was found (existed). If {@code null}, ignored. + * @return {@code True}, if data is found and completely deleted; + * {@code False}, if nothing found or if data is found but might not be deleted completely. + */ + public boolean deleteLocalSnapshot(SnapshotFileTree sft, @Nullable AtomicBoolean existsFlag) { Review Comment: I question about atomic - you reply about lambda and final ) Seems this complication cause you want return IgniteSnapshotManager#deleteLocalSnapshot 2 states here, from return mechanism and from AtomicBoolean state, overcomplicated, change it plz -- 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]
