No error/warning on failed snapshot deletion #10676

modules/core/core-api/src/main/java/com/enonic/xp/node/DeleteSnapshotsResult.java

@@ -13,9 +13,17 @@
 public class DeleteSnapshotsResult
     extends AbstractImmutableEntitySet<String>
 {
+    private final Set<String> failedSnapshotNames;
+
     private DeleteSnapshotsResult( final Builder builder )
     {
         super( ImmutableSet.copyOf( builder.snapshotNames ) );
+        this.failedSnapshotNames = ImmutableSet.copyOf( builder.failedSnapshotNames );
+    }
+
+    public Set<String> getFailedSnapshotNames()
+    {
+        return failedSnapshotNames;
     }
 
     public static Builder create()
@@ -27,19 +35,28 @@ public static class Builder
     {
         private final Set<String> snapshotNames = new HashSet<>();
 
+        private final Set<String> failedSnapshotNames = new HashSet<>();
+
 
         public Builder add( final String snapshotName )
         {
             this.snapshotNames.add( snapshotName );
             return this;
         }
 
+        @Deprecated
         public Builder addAll( final Collection<String> snapshotNames )
         {
             this.snapshotNames.addAll( snapshotNames );
             return this;
         }
 
+        public Builder addFailed( final String snapshotName )
+        {
+            this.failedSnapshotNames.add( snapshotName );
+            return this;
+        }
+
         public DeleteSnapshotsResult build()
         {
             return new DeleteSnapshotsResult( this );

modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java

@@ -2,7 +2,6 @@
 
 import java.nio.file.Path;
 import java.time.Instant;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
@@ -316,46 +315,54 @@ private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params )
 
         if ( !params.getSnapshotNames().isEmpty() )
         {
-            builder.addAll( deleteByName( params.getSnapshotNames() ) );
+            deleteByName( builder, params.getSnapshotNames() );
         }
 
         if ( params.getBefore() != null )
         {
-            builder.addAll( deleteByBefore( params.getBefore() ) );
+            deleteByBefore( builder, params.getBefore() );
         }
 
         return builder.build();
     }
 
-    private Set<String> deleteByBefore( final Instant before )
+    private void deleteByBefore( final DeleteSnapshotsResult.Builder builder, final Instant before )
     {
-        final Set<String> deleted = new HashSet<>();
-
         final SnapshotResults snapshotResults = doListSnapshots();
 
         for ( final SnapshotResult snapshotResult : snapshotResults )
         {
             if ( snapshotResult.getTimestamp().isBefore( before ) )
             {
-                doDeleteSnapshot( snapshotResult.getName() );
-                deleted.add( snapshotResult.getName() );
+                try
+                {
+                    doDeleteSnapshot( snapshotResult.getName() );
+                    builder.add( snapshotResult.getName() );
+                }
+                catch ( Exception e )
+                {
+                    LOG.error( "Snapshot delete failed: {}", snapshotResult.getName(), e );
+                    builder.addFailed( snapshotResult.getName() );
+                }
             }
         }
-
-        return deleted;
     }
 
-    private Set<String> deleteByName( final Set<String> snapshotNames )
+    private void deleteByName( final DeleteSnapshotsResult.Builder builder, final Set<String> snapshotNames )
     {
-        final Set<String> deletedNames = new HashSet<>();
-
         for ( final String name : snapshotNames )
         {
-            doDeleteSnapshot( name );
-            deletedNames.add( name );
+            try
+            {
+                doDeleteSnapshot( name );
+                builder.add( name );
+            }
+            catch ( Exception e )
+            {
+                LOG.error( "Snapshot delete failed: {}", name, e );
+                builder.addFailed( name );
+            }
         }
-
-        return deletedNames;
     }
 
     private void doDeleteSnapshot( final String snapshotName )

modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTask.java

@@ -42,20 +42,13 @@ public VacuumTaskResult execute( final VacuumTaskParams params )
         }
 
         final VacuumTaskResult.Builder builder = VacuumTaskResult.create().taskName( NAME );
-        try
-        {
-            final DeleteSnapshotsResult deleteSnapshotsResult = snapshotService.delete(
-                DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );
+        final DeleteSnapshotsResult deleteSnapshotsResult =
+            snapshotService.delete( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );
 
-            deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() );
+        deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() );
+        deleteSnapshotsResult.getFailedSnapshotNames().forEach( snapshot -> builder.failed() );
 
-            return builder.build();
-        }
-        catch ( Exception e )
-        {
-            LOG.error( "Failed to vacuum snapshots", e );
-            return builder.failed().build();
-        }
+        return builder.build();
     }
 
     @Override

modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImplTest.java

@@ -0,0 +1,120 @@
+package com.enonic.xp.repo.impl.elasticsearch.snapshot;
+
+import java.nio.file.Path;
+import java.time.Instant;
+import java.time.temporal.ChronoUnit;
+import java.util.List;
+
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.ActionFuture;
+import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesRequest;
+import org.elasticsearch.action.admin.cluster.repositories.get.GetRepositoriesResponse;
+import org.elasticsearch.action.admin.cluster.snapshots.delete.DeleteSnapshotRequest;
+import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsRequest;
+import org.elasticsearch.action.admin.cluster.snapshots.get.GetSnapshotsResponse;
+import org.elasticsearch.client.AdminClient;
+import org.elasticsearch.client.Client;
+import org.elasticsearch.client.ClusterAdminClient;
+import org.elasticsearch.cluster.metadata.RepositoryMetaData;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.snapshots.SnapshotInfo;
+import org.elasticsearch.snapshots.SnapshotState;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import com.enonic.xp.event.EventPublisher;
+import com.enonic.xp.node.DeleteSnapshotParams;
+import com.enonic.xp.node.DeleteSnapshotsResult;
+import com.enonic.xp.repo.impl.config.RepoConfiguration;
+import com.enonic.xp.repo.impl.index.IndexServiceInternal;
+import com.enonic.xp.repo.impl.repository.RepositoryEntryService;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class SnapshotServiceImplTest
+{
+    private ClusterAdminClient clusterAdminClient;
+
+    private SnapshotServiceImpl instance;
+
+    @BeforeEach
+    void setUp()
+    {
+        final Client client = mock( Client.class );
+        clusterAdminClient = mock( ClusterAdminClient.class );
+
+        when( client.admin() ).thenReturn( mock( AdminClient.class ) );
+        when( client.admin().cluster() ).thenReturn( clusterAdminClient );
+
+        final RepoConfiguration configuration = mock( RepoConfiguration.class );
+        when( configuration.getSnapshotsDir() ).thenReturn( Path.of( "tmp", "data" ) );
+
+        final RepositoryEntryService repositoryEntryService = mock( RepositoryEntryService.class );
+        final EventPublisher eventPublisher = mock( EventPublisher.class );
+        final IndexServiceInternal indexServiceInternal = mock( IndexServiceInternal.class );
+
+        instance = new SnapshotServiceImpl( client, configuration, repositoryEntryService, eventPublisher, indexServiceInternal );
+    }
+
+    @Test
+    void testDelete()
+    {
+        final RepositoryMetaData repositoryMetaData = new RepositoryMetaData( "enonic-xp-snapshot-repo", "fs", Settings.settingsBuilder()
+            .put( "compress", true )
+            .put( "location", Path.of( "tmp", "data" ).toString() )
+            .build() );
+
+        final GetRepositoriesResponse getRepositoriesResponse = mock( GetRepositoriesResponse.class );
+        when( getRepositoriesResponse.repositories() ).thenReturn( List.of( repositoryMetaData ) );
+
+        final ActionFuture<GetRepositoriesResponse> getRepositoryActionFuture = mock( ActionFuture.class );
+        when( getRepositoryActionFuture.actionGet() ).thenReturn( getRepositoriesResponse );
+
+        when( clusterAdminClient.getRepositories( any( GetRepositoriesRequest.class ) ) ).thenReturn( getRepositoryActionFuture );
+
+        final Instant now = Instant.now();
+
+        final SnapshotInfo snapshot3 = mockSnapshot( "snapshot3", SnapshotState.SUCCESS, now.minus( 3, ChronoUnit.HOURS ) );
+        final SnapshotInfo snapshot4 = mockSnapshot( "snapshot4", SnapshotState.PARTIAL, now.minus( 4, ChronoUnit.HOURS ) );
+        final SnapshotInfo snapshot5 = mockSnapshot( "snapshot5", SnapshotState.SUCCESS, now.minus( 1, ChronoUnit.HOURS ) );
+
+        final GetSnapshotsResponse getSnapshotsResponse = mock( GetSnapshotsResponse.class );
+        when( getSnapshotsResponse.getSnapshots() ).thenReturn( List.of( snapshot3, snapshot4, snapshot5 ) );
+
+        final ActionFuture<GetSnapshotsResponse> getSnapshotsActionFuture = mock( ActionFuture.class );
+        when( getSnapshotsActionFuture.actionGet() ).thenReturn( getSnapshotsResponse );
+
+        when( clusterAdminClient.getSnapshots( any( GetSnapshotsRequest.class ) ) ).thenReturn( getSnapshotsActionFuture );
+
+        when( clusterAdminClient.deleteSnapshot( any( DeleteSnapshotRequest.class ) ) ).thenReturn( mock( ActionFuture.class ) );
+        when( clusterAdminClient.deleteSnapshot(
+            argThat( request -> "snapshot2".equals( request.snapshot() ) || "snapshot4".equals( request.snapshot() ) ) ) ).thenThrow(
+            new ElasticsearchException( "Failed to delete snapshot" ) );
+
+        final DeleteSnapshotsResult result = instance.delete(
+            DeleteSnapshotParams.create().add( "snapshot1" ).add( "snapshot2" ).before( now.minus( 2, ChronoUnit.HOURS ) ).build() );
+
+        assertEquals( 2, result.getSize() );
+        assertTrue( result.getSet().contains( "snapshot1" ) );
+        assertTrue( result.getSet().contains( "snapshot3" ) );
+        assertEquals( 2, result.getFailedSnapshotNames().size() );
+        assertTrue( result.getFailedSnapshotNames().contains( "snapshot2" ) );
+        assertTrue( result.getFailedSnapshotNames().contains( "snapshot4" ) );
+    }
+
+    private static SnapshotInfo mockSnapshot( String name, SnapshotState state, Instant endTime )
+    {
+        final SnapshotInfo snapshot = mock( SnapshotInfo.class );
+
+        when( snapshot.state() ).thenReturn( state );
+        when( snapshot.name() ).thenReturn( name );
+        when( snapshot.endTime() ).thenReturn( endTime.toEpochMilli() );
+
+        return snapshot;
+    }
+}

modules/core/core-repo/src/test/java/com/enonic/xp/repo/impl/vacuum/snapshots/SnapshotsVacuumTaskTest.java

@@ -64,23 +64,4 @@ public void processed( final long count )
 
         verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) );
     }
-
-    @Test
-    void testFailed()
-    {
-        SnapshotService snapshotService = mock( SnapshotService.class );
-        when( snapshotService.delete( any( DeleteSnapshotParams.class ) ) ).thenThrow( new RuntimeException() );
-
-        SnapshotsVacuumTask instance = new SnapshotsVacuumTask( snapshotService );
-
-        VacuumTaskResult result = instance.execute( VacuumTaskParams.create().ageThreshold( 60 * 1000 ).build() );
-
-        assertEquals( "SnapshotsVacuumTask", result.getTaskName() );
-        assertEquals( 0, result.getProcessed() );
-        assertEquals( 0, result.getInUse() );
-        assertEquals( 0, result.getDeleted() );
-        assertEquals( 1, result.getFailed() );
-
-        verify( snapshotService, times( 1 ) ).delete( any( DeleteSnapshotParams.class ) );
-    }
 }

modules/server/server-rest/src/main/java/com/enonic/xp/impl/server/rest/model/DeleteSnapshotsResultJson.java

@@ -8,18 +8,26 @@ public class DeleteSnapshotsResultJson
 {
     private final Set<String> deletedSnapshots;
 
-    private DeleteSnapshotsResultJson( final Set<String> deletedSnapshots )
+    private final Set<String> failedSnapshots;
+
+    private DeleteSnapshotsResultJson( final Set<String> deletedSnapshots, final Set<String> failedSnapshots )
     {
         this.deletedSnapshots = deletedSnapshots;
+        this.failedSnapshots = failedSnapshots;
     }
 
     public static DeleteSnapshotsResultJson from( final DeleteSnapshotsResult result )
     {
-        return new DeleteSnapshotsResultJson( result.getSet() );
+        return new DeleteSnapshotsResultJson( result.getSet(), result.getFailedSnapshotNames() );
     }
 
     public Set<String> getDeletedSnapshots()
     {
         return deletedSnapshots;
     }
+
+    public Set<String> getFailedSnapshots()
+    {
+        return failedSnapshots;
+    }
 }

modules/server/server-rest/src/test/resources/com/enonic/xp/impl/server/rest/delete.json

@@ -1,5 +1,6 @@
 {
   "deletedSnapshots": [
     "snapshot1"
-  ]
-}
+  ],
+  "failedSnapshots": []
+}