Skip to content

Commit

Permalink
🐛 fix: recreate errored snapshots
Browse files Browse the repository at this point in the history
  • Loading branch information
@jfbus
jfbus committed Jan 27, 2025
1 parent 7e1048c commit 24e68cb
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 82 deletions.
4 changes: 4 additions & 0 deletions pkg/cloud/cloud.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ func (s *Snapshot) IsReadyToUse() bool {
return s.State == "completed"
}

func (s *Snapshot) IsError() bool {
return s.State == "error"
}

// ListSnapshotsResponse is the container for our snapshots along with a pagination token to pass back to the caller
type ListSnapshotsResponse struct {
Snapshots []Snapshot
Expand Down
17 changes: 12 additions & 5 deletions pkg/driver/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,13 +424,20 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
return nil, status.Error(codes.InvalidArgument, "Snapshot volume source ID not provided")
}
snapshot, err := d.cloud.GetSnapshotByName(ctx, snapshotName)
if err != nil && !errors.Is(err, cloud.ErrNotFound) {
switch {
case errors.Is(err, cloud.ErrNotFound):
// No snapshot found, continue with creation.
case err != nil:
return nil, err
}
if !cloud.IsNilSnapshot(snapshot) {
if snapshot.SourceVolumeID != volumeID {
return nil, status.Errorf(codes.AlreadyExists, "Snapshot %s already exists for different volume (%s)", snapshotName, snapshot.SourceVolumeID)
case snapshot.IsError():
// A previously created snapshot is in error, delete it.
klog.FromContext(ctx).V(3).Info("Deleting previous snapshot in error")
if _, err := d.cloud.DeleteSnapshot(ctx, snapshot.SnapshotID); err != nil && !errors.Is(err, cloud.ErrNotFound) {
return nil, status.Errorf(codes.Internal, "Could not delete existing snapshot in error %q: %v", snapshot.SnapshotID, err)
}
case snapshot.SourceVolumeID != volumeID:
return nil, status.Errorf(codes.AlreadyExists, "Snapshot %s already exists for different volume (%s)", snapshotName, snapshot.SourceVolumeID)
default:
klog.FromContext(ctx).V(3).Info("Snapshot already exists")
return newCreateSnapshotResponse(snapshot)
}
Expand Down
158 changes: 81 additions & 77 deletions pkg/driver/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/outscale/osc-bsu-csi-driver/pkg/driver/mocks"
"github.com/outscale/osc-bsu-csi-driver/pkg/util"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
Expand Down Expand Up @@ -1305,9 +1306,7 @@ func TestPickAvailabilityZone(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual := pickAvailabilityZone(tc.requirement)
if actual != tc.expZone {
t.Fatalf("Expected zone %v, got zone: %v", tc.expZone, actual)
}
assert.Equal(t, tc.expZone, actual)
})
}
}
Expand All @@ -1325,36 +1324,32 @@ func TestCreateSnapshot(t *testing.T) {
Parameters: nil,
SourceVolumeId: "vol-test",
}
expSnapshot := &csi.Snapshot{
ReadyToUse: true,
}

ctx := context.Background()
mockSnapshot := cloud.Snapshot{
SnapshotID: fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
SourceVolumeID: req.SourceVolumeId,
Size: 1,
CreationTime: time.Now(),
State: "completed",
}
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()

mockCloud := mocks.NewMockCloud(mockCtl)
mockCloud.EXPECT().CreateSnapshot(gomock.Eq(ctx), gomock.Eq(req.SourceVolumeId), gomock.Any()).Return(mockSnapshot, nil)
mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(req.GetName())).Return(cloud.Snapshot{}, cloud.ErrNotFound)
mockCloud.EXPECT().CreateSnapshot(gomock.Eq(ctx), gomock.Eq(req.SourceVolumeId), gomock.Any()).Return(mockSnapshot, nil)

oscDriver := controllerService{
cloud: mockCloud,
driverOptions: &DriverOptions{},
}
resp, err := oscDriver.CreateSnapshot(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if snap := resp.GetSnapshot(); snap == nil {
t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
}
snap := resp.GetSnapshot()
require.NotNil(t, snap)
assert.True(t, snap.ReadyToUse)
},
},
{
Expand All @@ -1374,17 +1369,11 @@ func TestCreateSnapshot(t *testing.T) {
cloud: mockCloud,
driverOptions: &DriverOptions{},
}
if _, err := oscDriver.CreateSnapshot(context.Background(), req); err != nil {
srvErr, ok := status.FromError(err)
if !ok {
t.Fatalf("Could not get error status code from error: %v", srvErr)
}
if srvErr.Code() != codes.InvalidArgument {
t.Fatalf("Expected error code %d, got %d message %s", codes.InvalidArgument, srvErr.Code(), srvErr.Message())
}
} else {
t.Fatalf("Expected error %v, got no error", codes.InvalidArgument)
}
_, err := oscDriver.CreateSnapshot(context.Background(), req)
require.Error(t, err)
srvErr, ok := status.FromError(err)
assert.True(t, ok, "Should get an error status code")
assert.Equal(t, codes.InvalidArgument, srvErr.Code())
},
},
{
Expand All @@ -1400,16 +1389,14 @@ func TestCreateSnapshot(t *testing.T) {
Parameters: nil,
SourceVolumeId: "vol-xxx",
}
expSnapshot := &csi.Snapshot{
ReadyToUse: true,
}

ctx := context.Background()
mockSnapshot := cloud.Snapshot{
SnapshotID: fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
SourceVolumeID: req.SourceVolumeId,
Size: 1,
CreationTime: time.Now(),
State: "completed",
}
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()
Expand All @@ -1434,9 +1421,8 @@ func TestCreateSnapshot(t *testing.T) {
t.Fatalf("Unexpected error: %v", err)
}
snap := resp.GetSnapshot()
if snap == nil {
t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
}
require.NotNil(t, snap)
assert.True(t, snap.ReadyToUse)

mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(extraReq.GetName())).Return(mockSnapshot, nil)
_, err = oscDriver.CreateSnapshot(ctx, extraReq)
Expand Down Expand Up @@ -1466,16 +1452,14 @@ func TestCreateSnapshot(t *testing.T) {
Parameters: nil,
SourceVolumeId: "vol-test",
}
expSnapshot := &csi.Snapshot{
ReadyToUse: true,
}

ctx := context.Background()
mockSnapshot := cloud.Snapshot{
SnapshotID: fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
SourceVolumeID: req.SourceVolumeId,
Size: 1,
CreationTime: time.Now(),
State: "completed",
}
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()
Expand All @@ -1489,19 +1473,14 @@ func TestCreateSnapshot(t *testing.T) {
driverOptions: &DriverOptions{},
}
resp, err := oscDriver.CreateSnapshot(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)
snap := resp.GetSnapshot()
if snap == nil {
t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
}
require.NotNil(t, snap)
assert.True(t, snap.ReadyToUse)

mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(extraReq.GetName())).Return(mockSnapshot, nil)
_, err = oscDriver.CreateSnapshot(ctx, extraReq)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)
},
},
{
Expand All @@ -1517,9 +1496,6 @@ func TestCreateSnapshot(t *testing.T) {
Parameters: nil,
SourceVolumeId: "vol-test",
}
expSnapshot := &csi.Snapshot{
ReadyToUse: true,
}
extraSnapshotTagKey := "foo"
extraSnapshotTagValue := "bar"
snapshotOptions := &cloud.SnapshotOptions{
Expand All @@ -1535,6 +1511,7 @@ func TestCreateSnapshot(t *testing.T) {
SourceVolumeID: req.SourceVolumeId,
Size: 1,
CreationTime: time.Now(),
State: "completed",
}
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()
Expand All @@ -1552,19 +1529,62 @@ func TestCreateSnapshot(t *testing.T) {
},
}
resp, err := oscDriver.CreateSnapshot(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)
snap := resp.GetSnapshot()
if snap == nil {
t.Fatalf("Expected snapshot %v, got nil", expSnapshot)
}
require.NotNil(t, snap)
assert.True(t, snap.ReadyToUse)

mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(extraReq.GetName())).Return(mockSnapshot, nil)
_, err = oscDriver.CreateSnapshot(ctx, extraReq)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
require.NoError(t, err)
},
},
{
name: "snapshot in error are recreated",
testFunc: func(t *testing.T) {
req := &csi.CreateSnapshotRequest{
Name: "test-snapshot",
Parameters: nil,
SourceVolumeId: "vol-test",
}
extraSnapshotTagKey := "foo"
extraSnapshotTagValue := "bar"
snapshotOptions := &cloud.SnapshotOptions{
Tags: map[string]string{
cloud.SnapshotNameTagKey: req.Name,
extraSnapshotTagKey: extraSnapshotTagValue,
},
}

ctx := context.Background()
mockSnapshot := cloud.Snapshot{
SnapshotID: fmt.Sprintf("snapshot-%d", rand.New(rand.NewSource(time.Now().UnixNano())).Uint64()),
SourceVolumeID: req.SourceVolumeId,
Size: 1,
CreationTime: time.Now(),
State: "completed",
}
mockCtl := gomock.NewController(t)
defer mockCtl.Finish()

mockCloud := mocks.NewMockCloud(mockCtl)
mockCloud.EXPECT().GetSnapshotByName(gomock.Eq(ctx), gomock.Eq(req.GetName())).
Return(cloud.Snapshot{SnapshotID: "snap_foo", State: "error"}, nil)
mockCloud.EXPECT().DeleteSnapshot(gomock.Eq(ctx), gomock.Eq("snap_foo")).Return(true, nil)
mockCloud.EXPECT().CreateSnapshot(gomock.Eq(ctx), gomock.Eq(req.SourceVolumeId), gomock.Eq(snapshotOptions)).Return(mockSnapshot, nil)

oscDriver := controllerService{
cloud: mockCloud,
driverOptions: &DriverOptions{
extraSnapshotTags: map[string]string{
extraSnapshotTagKey: extraSnapshotTagValue,
},
},
}
resp, err := oscDriver.CreateSnapshot(context.Background(), req)
require.NoError(t, err)
snap := resp.GetSnapshot()
require.NotNil(t, snap)
},
},
}
Expand Down Expand Up @@ -1674,9 +1694,7 @@ func TestListSnapshots(t *testing.T) {
}

resp, err := oscDriver.ListSnapshots(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if len(resp.GetEntries()) != len(mockCloudSnapshotsResponse.Snapshots) {
t.Fatalf("Expected %d entries, got %d", len(mockCloudSnapshotsResponse.Snapshots), len(resp.GetEntries()))
Expand All @@ -1700,9 +1718,7 @@ func TestListSnapshots(t *testing.T) {
}

resp, err := oscDriver.ListSnapshots(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, &csi.ListSnapshotsResponse{}) {
t.Fatalf("Expected empty response, got %+v", resp)
Expand Down Expand Up @@ -1735,9 +1751,7 @@ func TestListSnapshots(t *testing.T) {
}

resp, err := oscDriver.ListSnapshots(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if len(resp.GetEntries()) != 1 {
t.Fatalf("Expected %d entry, got %d", 1, len(resp.GetEntries()))
Expand All @@ -1764,9 +1778,7 @@ func TestListSnapshots(t *testing.T) {
}

resp, err := oscDriver.ListSnapshots(context.Background(), req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, &csi.ListSnapshotsResponse{}) {
t.Fatalf("Expected empty response, got %+v", resp)
Expand Down Expand Up @@ -1886,9 +1898,7 @@ func TestControllerPublishVolume(t *testing.T) {
}

resp, err := oscDriver.ControllerPublishVolume(ctx, req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, expResp) {
t.Fatalf("Expected resp to be %+v, got: %+v", expResp, resp)
Expand All @@ -1914,9 +1924,7 @@ func TestControllerPublishVolume(t *testing.T) {

oscDriver := controllerService{cloud: mockCloud}
resp, err := oscDriver.ControllerUnpublishVolume(ctx, req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, expResp) {
t.Fatalf("Expected resp to be %+v, got: %+v", expResp, resp)
Expand Down Expand Up @@ -2204,9 +2212,7 @@ func TestControllerPublishVolume(t *testing.T) {
}

resp, err := oscDriver.ControllerPublishVolume(ctx, req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, expResp) {
t.Fatalf("Expected resp to be %+v, got: %+v", expResp, resp)
Expand Down Expand Up @@ -2248,9 +2254,7 @@ func TestControllerUnpublishVolume(t *testing.T) {
}

resp, err := oscDriver.ControllerUnpublishVolume(ctx, req)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
require.NoError(t, err)

if !reflect.DeepEqual(resp, expResp) {
t.Fatalf("Expected resp to be %+v, got: %+v", expResp, resp)
Expand Down

0 comments on commit 24e68cb

Please sign in to comment.