Skip to content

Commit

Permalink
No error/warning on failed snapshot deletion #10676
Browse files Browse the repository at this point in the history
  • Loading branch information
anatol-sialitski committed Dec 10, 2024
1 parent d7f125d commit 166f4c0
Show file tree
Hide file tree
Showing 8 changed files with 234 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.enonic.xp.annotation.PublicApi;
import com.enonic.xp.support.AbstractImmutableEntitySet;

@Deprecated
@PublicApi
public class DeleteSnapshotsResult
extends AbstractImmutableEntitySet<String>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package com.enonic.xp.node;

import java.util.Set;

import com.google.common.collect.ImmutableSet;

import com.enonic.xp.annotation.PublicApi;

@PublicApi
public final class RemoveSnapshotsResult
{
private final Set<String> snapshotNames;

private final Set<String> failedSnapshotNames;

private RemoveSnapshotsResult( final Builder builder )
{
this.snapshotNames = builder.snapshotNames.build();
this.failedSnapshotNames = builder.failedSnapshotNames.build();
}

public Set<String> getSnapshotNames()
{
return snapshotNames;
}

public Set<String> getFailedSnapshotNames()
{
return failedSnapshotNames;
}

public static Builder create()
{
return new Builder();
}

public static final class Builder
{
private final ImmutableSet.Builder<String> snapshotNames = ImmutableSet.builder();

private final ImmutableSet.Builder<String> failedSnapshotNames = ImmutableSet.builder();

public Builder addProcessed( final String snapshotName )
{
this.snapshotNames.add( snapshotName );
return this;
}

public Builder addFailed( final String snapshotName )
{
this.failedSnapshotNames.add( snapshotName );
return this;
}

public RemoveSnapshotsResult build()
{
return new RemoveSnapshotsResult( this );
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.enonic.xp.node.DeleteSnapshotParams;
import com.enonic.xp.node.DeleteSnapshotsResult;
import com.enonic.xp.node.RemoveSnapshotsResult;
import com.enonic.xp.node.RestoreParams;
import com.enonic.xp.node.RestoreResult;
import com.enonic.xp.node.SnapshotParams;
Expand All @@ -14,7 +15,10 @@ public interface SnapshotService

RestoreResult restore( RestoreParams restoreParams );

@Deprecated
DeleteSnapshotsResult delete( DeleteSnapshotParams params );

RemoveSnapshotsResult remove( DeleteSnapshotParams params );

SnapshotResults list();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -33,6 +32,7 @@
import com.enonic.xp.event.EventPublisher;
import com.enonic.xp.node.DeleteSnapshotParams;
import com.enonic.xp.node.DeleteSnapshotsResult;
import com.enonic.xp.node.RemoveSnapshotsResult;
import com.enonic.xp.node.RestoreParams;
import com.enonic.xp.node.RestoreResult;
import com.enonic.xp.node.SnapshotParams;
Expand Down Expand Up @@ -197,8 +197,18 @@ private SnapshotResults doListSnapshots()
return SnapshotResultsFactory.create( getSnapshotsResponse );
}

@Deprecated
@Override
public DeleteSnapshotsResult delete( final DeleteSnapshotParams params )
{
return NodeHelper.runAsAdmin( () -> {
final RemoveSnapshotsResult result = doDelete( params );
return DeleteSnapshotsResult.create().addAll( result.getSnapshotNames() ).build();

Check warning on line 206 in modules/core/core-repo/src/main/java/com/enonic/xp/repo/impl/elasticsearch/snapshot/SnapshotServiceImpl.java

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L204 - L206 were not covered by tests
} );
}

@Override
public RemoveSnapshotsResult remove( final DeleteSnapshotParams params )
{
return NodeHelper.runAsAdmin( () -> doDelete( params ) );
}
Expand Down Expand Up @@ -310,52 +320,60 @@ private SnapshotInfo getSnapshot( final String snapshotName )
}
}

private DeleteSnapshotsResult doDelete( final DeleteSnapshotParams params )
private RemoveSnapshotsResult doDelete( final DeleteSnapshotParams params )
{
final DeleteSnapshotsResult.Builder builder = DeleteSnapshotsResult.create();
final RemoveSnapshotsResult.Builder builder = RemoveSnapshotsResult.create();

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 RemoveSnapshotsResult.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.addProcessed( 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 RemoveSnapshotsResult.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.addProcessed( name );
}
catch ( Exception e )
{
LOG.error( "Snapshot delete failed: {}", name, e );
builder.addFailed( name );
}
}

return deletedNames;
}

private void doDeleteSnapshot( final String snapshotName )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.enonic.xp.node.DeleteSnapshotParams;
import com.enonic.xp.node.DeleteSnapshotsResult;
import com.enonic.xp.node.RemoveSnapshotsResult;
import com.enonic.xp.repo.impl.vacuum.VacuumTask;
import com.enonic.xp.repo.impl.vacuum.VacuumTaskParams;
import com.enonic.xp.snapshot.SnapshotService;
Expand All @@ -19,8 +17,6 @@
public class SnapshotsVacuumTask
implements VacuumTask
{
private static final Logger LOG = LoggerFactory.getLogger( SnapshotsVacuumTask.class );

private static final int ORDER = 400;

private static final String NAME = "SnapshotsVacuumTask";
Expand All @@ -42,20 +38,14 @@ 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() );

deleteSnapshotsResult.stream().forEach( snapshot -> builder.processed() );
final RemoveSnapshotsResult deleteSnapshotsResult =
snapshotService.remove( DeleteSnapshotParams.create().before( Instant.now().minusMillis( params.getAgeThreshold() ) ).build() );

return builder.build();
}
catch ( Exception e )
{
LOG.error( "Failed to vacuum snapshots", e );
return builder.failed().build();
}
deleteSnapshotsResult.getSnapshotNames().forEach( snapshot -> builder.processed() );
deleteSnapshotsResult.getFailedSnapshotNames().forEach( snapshot -> builder.failed() );

return builder.build();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -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.RemoveSnapshotsResult;
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 testRemove()
{
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 RemoveSnapshotsResult result = instance.remove(
DeleteSnapshotParams.create().add( "snapshot1" ).add( "snapshot2" ).before( now.minus( 2, ChronoUnit.HOURS ) ).build() );

assertEquals( 2, result.getSnapshotNames().size() );
assertTrue( result.getSnapshotNames().contains( "snapshot1" ) );
assertTrue( result.getSnapshotNames().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;
}
}
Loading

0 comments on commit 166f4c0

Please sign in to comment.