Skip to content

Commit 83c8c05

Browse files
msau42msau42
committed
Update snapshotter to use CSI spec 1.0. This breaks the snapshot handling of error and uploading status
1 parent 9be7a7d commit 83c8c05

File tree

4 files changed

+102
-73
lines changed

4 files changed

+102
-73
lines changed

‎pkg/connection/connection.go‎

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import (
2525

2626
"github.com/container-storage-interface/spec/lib/go/csi"
2727
"github.com/golang/glog"
28+
"github.com/golang/protobuf/ptypes"
29+
"github.com/golang/protobuf/ptypes/timestamp"
2830
"google.golang.org/grpc"
2931
"google.golang.org/grpc/connectivity"
3032
"k8s.io/api/core/v1"
@@ -46,13 +48,13 @@ type CSIConnection interface {
4648
SupportsControllerListSnapshots(ctx context.Context) (bool, error)
4749

4850
// CreateSnapshot creates a snapshot for a volume
49-
CreateSnapshot(ctx context.Context, snapshotName string, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (driverName string, snapshotId string, timestamp int64, size int64, status *csi.SnapshotStatus, err error)
51+
CreateSnapshot(ctx context.Context, snapshotName string, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (driverName string, snapshotId string, timestamp int64, size int64, readyToUse bool, err error)
5052

5153
// DeleteSnapshot deletes a snapshot from a volume
5254
DeleteSnapshot(ctx context.Context, snapshotID string, snapshotterCredentials map[string]string) (err error)
5355

54-
// GetSnapshotStatus returns a snapshot's status, creation time, and restore size.
55-
GetSnapshotStatus(ctx context.Context, snapshotID string) (*csi.SnapshotStatus, int64, int64, error)
56+
// GetSnapshotStatus returns if a snapshot is ready to use, creation time, and restore size.
57+
GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, int64, int64, error)
5658

5759
// Probe checks that the CSI driver is ready to process requests
5860
Probe(ctx context.Context) error
@@ -188,41 +190,45 @@ func (c *csiConnection) SupportsControllerListSnapshots(ctx context.Context) (bo
188190
return false, nil
189191
}
190192

191-
func (c *csiConnection) CreateSnapshot(ctx context.Context, snapshotName string, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, *csi.SnapshotStatus, error) {
193+
func (c *csiConnection) CreateSnapshot(ctx context.Context, snapshotName string, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, bool, error) {
192194
glog.V(5).Infof("CSI CreateSnapshot: %s", snapshotName)
193195
if volume.Spec.CSI == nil {
194-
return "", "", 0, 0, nil, fmt.Errorf("CSIPersistentVolumeSource not defined in spec")
196+
return "", "", 0, 0, false, fmt.Errorf("CSIPersistentVolumeSource not defined in spec")
195197
}
196198

197199
client := csi.NewControllerClient(c.conn)
198200

199201
driverName, err := c.GetDriverName(ctx)
200202
if err != nil {
201-
return "", "", 0, 0, nil, err
203+
return "", "", 0, 0, false, err
202204
}
203205

204206
req := csi.CreateSnapshotRequest{
205-
SourceVolumeId: volume.Spec.CSI.VolumeHandle,
206-
Name: snapshotName,
207-
Parameters: parameters,
208-
CreateSnapshotSecrets: snapshotterCredentials,
207+
SourceVolumeId: volume.Spec.CSI.VolumeHandle,
208+
Name: snapshotName,
209+
Parameters: parameters,
210+
Secrets: snapshotterCredentials,
209211
}
210212

211213
rsp, err := client.CreateSnapshot(ctx, &req)
212214
if err != nil {
213-
return "", "", 0, 0, nil, err
215+
return "", "", 0, 0, false, err
214216
}
215217

216-
glog.V(5).Infof("CSI CreateSnapshot: %s driver name [%s] snapshot ID [%s] time stamp [%d] size [%d] status [%s]", snapshotName, driverName, rsp.Snapshot.Id, rsp.Snapshot.CreatedAt, rsp.Snapshot.SizeBytes, *rsp.Snapshot.Status)
217-
return driverName, rsp.Snapshot.Id, rsp.Snapshot.CreatedAt, rsp.Snapshot.SizeBytes, rsp.Snapshot.Status, nil
218+
glog.V(5).Infof("CSI CreateSnapshot: %s driver name [%s] snapshot ID [%s] time stamp [%d] size [%d] readyToUse [%v]", snapshotName, driverName, rsp.Snapshot.SnapshotId, rsp.Snapshot.CreationTime, rsp.Snapshot.SizeBytes, rsp.Snapshot.ReadyToUse)
219+
creationTime, err := timestampToUnixTime(rsp.Snapshot.CreationTime)
220+
if err != nil {
221+
return "", "", 0, 0, false, err
222+
}
223+
return driverName, rsp.Snapshot.SnapshotId, creationTime, rsp.Snapshot.SizeBytes, rsp.Snapshot.ReadyToUse, nil
218224
}
219225

220226
func (c *csiConnection) DeleteSnapshot(ctx context.Context, snapshotID string, snapshotterCredentials map[string]string) (err error) {
221227
client := csi.NewControllerClient(c.conn)
222228

223229
req := csi.DeleteSnapshotRequest{
224-
SnapshotId: snapshotID,
225-
DeleteSnapshotSecrets: snapshotterCredentials,
230+
SnapshotId: snapshotID,
231+
Secrets: snapshotterCredentials,
226232
}
227233

228234
if _, err := client.DeleteSnapshot(ctx, &req); err != nil {
@@ -232,7 +238,7 @@ func (c *csiConnection) DeleteSnapshot(ctx context.Context, snapshotID string, s
232238
return nil
233239
}
234240

235-
func (c *csiConnection) GetSnapshotStatus(ctx context.Context, snapshotID string) (*csi.SnapshotStatus, int64, int64, error) {
241+
func (c *csiConnection) GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, int64, int64, error) {
236242
client := csi.NewControllerClient(c.conn)
237243

238244
req := csi.ListSnapshotsRequest{
@@ -241,14 +247,18 @@ func (c *csiConnection) GetSnapshotStatus(ctx context.Context, snapshotID string
241247

242248
rsp, err := client.ListSnapshots(ctx, &req)
243249
if err != nil {
244-
return nil, 0, 0, err
250+
return false, 0, 0, err
245251
}
246252

247253
if rsp.Entries == nil || len(rsp.Entries) == 0 {
248-
return nil, 0, 0, fmt.Errorf("can not find snapshot for snapshotID %s", snapshotID)
254+
return false, 0, 0, fmt.Errorf("can not find snapshot for snapshotID %s", snapshotID)
249255
}
250256

251-
return rsp.Entries[0].Snapshot.Status, rsp.Entries[0].Snapshot.CreatedAt, rsp.Entries[0].Snapshot.SizeBytes, nil
257+
creationTime, err := timestampToUnixTime(rsp.Entries[0].Snapshot.CreationTime)
258+
if err != nil {
259+
return false, 0, 0, err
260+
}
261+
return rsp.Entries[0].Snapshot.ReadyToUse, creationTime, rsp.Entries[0].Snapshot.SizeBytes, nil
252262
}
253263

254264
func (c *csiConnection) Close() error {
@@ -262,3 +272,13 @@ func logGRPC(ctx context.Context, method string, req, reply interface{}, cc *grp
262272
glog.V(5).Infof("GRPC error: %v", err)
263273
return err
264274
}
275+
276+
func timestampToUnixTime(t *timestamp.Timestamp) (int64, error) {
277+
time, err := ptypes.Timestamp(t)
278+
if err != nil {
279+
return -1, err
280+
}
281+
// TODO: clean this up, we probably don't need this translation layer
282+
// and can just use time.Time
283+
return time.UnixNano(), nil
284+
}

‎pkg/connection/connection_test.go‎

Lines changed: 39 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ import (
2121
"fmt"
2222
"reflect"
2323
"testing"
24-
"time"
2524

2625
"github.com/container-storage-interface/spec/lib/go/csi"
2726
"github.com/golang/mock/gomock"
27+
"github.com/golang/protobuf/ptypes"
2828
"github.com/kubernetes-csi/csi-test/driver"
2929
"google.golang.org/grpc/codes"
3030
"google.golang.org/grpc/status"
@@ -378,7 +378,11 @@ func TestSupportsControllerListSnapshots(t *testing.T) {
378378
func TestCreateSnapshot(t *testing.T) {
379379
defaultName := "snapshot-test"
380380
defaultID := "testid"
381-
createTime := time.Now().UnixNano()
381+
createTimestamp := ptypes.TimestampNow()
382+
createTime, err := ptypes.Timestamp(createTimestamp)
383+
if err != nil {
384+
t.Fatalf("Failed to convert timestamp to time: %v", err)
385+
}
382386

383387
createSecrets := map[string]string{"foo": "bar"}
384388
defaultParameter := map[string]string{
@@ -401,21 +405,18 @@ func TestCreateSnapshot(t *testing.T) {
401405
}
402406

403407
secretsRequest := &csi.CreateSnapshotRequest{
404-
Name: defaultName,
405-
SourceVolumeId: csiVolume.Spec.CSI.VolumeHandle,
406-
CreateSnapshotSecrets: createSecrets,
408+
Name: defaultName,
409+
SourceVolumeId: csiVolume.Spec.CSI.VolumeHandle,
410+
Secrets: createSecrets,
407411
}
408412

409413
defaultResponse := &csi.CreateSnapshotResponse{
410414
Snapshot: &csi.Snapshot{
411-
Id: defaultID,
415+
SnapshotId: defaultID,
412416
SizeBytes: 1000,
413417
SourceVolumeId: csiVolume.Spec.CSI.VolumeHandle,
414-
CreatedAt: createTime,
415-
Status: &csi.SnapshotStatus{
416-
Type: csi.SnapshotStatus_READY,
417-
Details: "success",
418-
},
418+
CreationTime: createTimestamp,
419+
ReadyToUse: true,
419420
},
420421
}
421422

@@ -432,18 +433,15 @@ func TestCreateSnapshot(t *testing.T) {
432433
snapshotId string
433434
timestamp int64
434435
size int64
435-
status *csi.SnapshotStatus
436+
readyToUse bool
436437
}
437438

438439
result := &snapshotResult{
439440
size: 1000,
440441
driverName: driverName,
441442
snapshotId: defaultID,
442-
timestamp: createTime,
443-
status: &csi.SnapshotStatus{
444-
Type: csi.SnapshotStatus_READY,
445-
Details: "success",
446-
},
443+
timestamp: createTime.UnixNano(),
444+
readyToUse: true,
447445
}
448446

449447
tests := []struct {
@@ -537,7 +535,7 @@ func TestCreateSnapshot(t *testing.T) {
537535
controllerServer.EXPECT().CreateSnapshot(gomock.Any(), in).Return(out, injectedErr).Times(1)
538536
}
539537

540-
driverName, snapshotId, timestamp, size, status, err := csiConn.CreateSnapshot(context.Background(), test.snapshotName, test.volume, test.parameters, test.secrets)
538+
driverName, snapshotId, timestamp, size, readyToUse, err := csiConn.CreateSnapshot(context.Background(), test.snapshotName, test.volume, test.parameters, test.secrets)
541539
if test.expectError && err == nil {
542540
t.Errorf("test %q: Expected error, got none", test.name)
543541
}
@@ -561,8 +559,8 @@ func TestCreateSnapshot(t *testing.T) {
561559
t.Errorf("test %q: expected size: %v, got: %v", test.name, test.expectResult.size, size)
562560
}
563561

564-
if !reflect.DeepEqual(status, test.expectResult.status) {
565-
t.Errorf("test %q: expected status: %v, got: %v", test.name, test.expectResult.status, status)
562+
if !reflect.DeepEqual(readyToUse, test.expectResult.readyToUse) {
563+
t.Errorf("test %q: expected readyToUse: %v, got: %v", test.name, test.expectResult.readyToUse, readyToUse)
566564
}
567565
}
568566
}
@@ -577,8 +575,8 @@ func TestDeleteSnapshot(t *testing.T) {
577575
}
578576

579577
secretsRequest := &csi.DeleteSnapshotRequest{
580-
SnapshotId: defaultID,
581-
DeleteSnapshotSecrets: secrets,
578+
SnapshotId: defaultID,
579+
Secrets: secrets,
582580
}
583581

584582
tests := []struct {
@@ -657,8 +655,12 @@ func TestDeleteSnapshot(t *testing.T) {
657655

658656
func TestGetSnapshotStatus(t *testing.T) {
659657
defaultID := "testid"
660-
createdAt := time.Now().UnixNano()
661658
size := int64(1000)
659+
createTimestamp := ptypes.TimestampNow()
660+
createTime, err := ptypes.Timestamp(createTimestamp)
661+
if err != nil {
662+
t.Fatalf("Failed to convert timestamp to time: %v", err)
663+
}
662664

663665
defaultRequest := &csi.ListSnapshotsRequest{
664666
SnapshotId: defaultID,
@@ -668,14 +670,11 @@ func TestGetSnapshotStatus(t *testing.T) {
668670
Entries: []*csi.ListSnapshotsResponse_Entry{
669671
{
670672
Snapshot: &csi.Snapshot{
671-
Id: defaultID,
673+
SnapshotId: defaultID,
672674
SizeBytes: size,
673675
SourceVolumeId: "volumeid",
674-
CreatedAt: createdAt,
675-
Status: &csi.SnapshotStatus{
676-
Type: csi.SnapshotStatus_READY,
677-
Details: "success",
678-
},
676+
CreationTime: createTimestamp,
677+
ReadyToUse: true,
679678
},
680679
},
681680
},
@@ -688,21 +687,18 @@ func TestGetSnapshotStatus(t *testing.T) {
688687
output *csi.ListSnapshotsResponse
689688
injectError codes.Code
690689
expectError bool
691-
expectStatus *csi.SnapshotStatus
690+
expectReady bool
692691
expectCreateAt int64
693692
expectSize int64
694693
}{
695694
{
696-
name: "success",
697-
snapshotID: defaultID,
698-
input: defaultRequest,
699-
output: defaultResponse,
700-
expectError: false,
701-
expectStatus: &csi.SnapshotStatus{
702-
Type: csi.SnapshotStatus_READY,
703-
Details: "success",
704-
},
705-
expectCreateAt: createdAt,
695+
name: "success",
696+
snapshotID: defaultID,
697+
input: defaultRequest,
698+
output: defaultResponse,
699+
expectError: false,
700+
expectReady: true,
701+
expectCreateAt: createTime.UnixNano(),
706702
expectSize: size,
707703
},
708704
{
@@ -744,15 +740,15 @@ func TestGetSnapshotStatus(t *testing.T) {
744740
controllerServer.EXPECT().ListSnapshots(gomock.Any(), in).Return(out, injectedErr).Times(1)
745741
}
746742

747-
status, createTime, size, err := csiConn.GetSnapshotStatus(context.Background(), test.snapshotID)
743+
ready, createTime, size, err := csiConn.GetSnapshotStatus(context.Background(), test.snapshotID)
748744
if test.expectError && err == nil {
749745
t.Errorf("test %q: Expected error, got none", test.name)
750746
}
751747
if !test.expectError && err != nil {
752748
t.Errorf("test %q: got error: %v", test.name, err)
753749
}
754-
if test.expectStatus != nil && !reflect.DeepEqual(test.expectStatus, status) {
755-
t.Errorf("test %q: expected status: %v, got: %v", test.name, test.expectStatus, status)
750+
if test.expectReady != ready {
751+
t.Errorf("test %q: expected status: %v, got: %v", test.name, test.expectReady, ready)
756752
}
757753
if test.expectCreateAt != createTime {
758754
t.Errorf("test %q: expected createTime: %v, got: %v", test.name, test.expectCreateAt, createTime)

‎pkg/controller/csi_handler.go‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,16 @@ import (
2222
"strings"
2323
"time"
2424

25-
"github.com/container-storage-interface/spec/lib/go/csi"
2625
crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1"
2726
"github.com/kubernetes-csi/external-snapshotter/pkg/connection"
2827
"k8s.io/api/core/v1"
2928
)
3029

3130
// Handler is responsible for handling VolumeSnapshot events from informer.
3231
type Handler interface {
33-
CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, *csi.SnapshotStatus, error)
32+
CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, bool, error)
3433
DeleteSnapshot(content *crdv1.VolumeSnapshotContent, snapshotterCredentials map[string]string) error
35-
GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (*csi.SnapshotStatus, int64, int64, error)
34+
GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (bool, int64, int64, error)
3635
}
3736

3837
// csiHandler is a handler that calls CSI to create/delete volume snapshot.
@@ -57,14 +56,14 @@ func NewCSIHandler(
5756
}
5857
}
5958

60-
func (handler *csiHandler) CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, *csi.SnapshotStatus, error) {
59+
func (handler *csiHandler) CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume *v1.PersistentVolume, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, int64, int64, bool, error) {
6160

6261
ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)
6362
defer cancel()
6463

6564
snapshotName, err := makeSnapshotName(handler.snapshotNamePrefix, string(snapshot.UID), handler.snapshotNameUUIDLength)
6665
if err != nil {
67-
return "", "", 0, 0, nil, err
66+
return "", "", 0, 0, false, err
6867
}
6968
return handler.csiConnection.CreateSnapshot(ctx, snapshotName, volume, parameters, snapshotterCredentials)
7069
}
@@ -84,16 +83,16 @@ func (handler *csiHandler) DeleteSnapshot(content *crdv1.VolumeSnapshotContent,
8483
return nil
8584
}
8685

87-
func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (*csi.SnapshotStatus, int64, int64, error) {
86+
func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (bool, int64, int64, error) {
8887
if content.Spec.CSI == nil {
89-
return nil, 0, 0, fmt.Errorf("CSISnapshot not defined in spec")
88+
return false, 0, 0, fmt.Errorf("CSISnapshot not defined in spec")
9089
}
9190
ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)
9291
defer cancel()
9392

9493
csiSnapshotStatus, timestamp, size, err := handler.csiConnection.GetSnapshotStatus(ctx, content.Spec.CSI.SnapshotHandle)
9594
if err != nil {
96-
return nil, 0, 0, fmt.Errorf("failed to list snapshot data %s: %q", content.Name, err)
95+
return false, 0, 0, fmt.Errorf("failed to list snapshot data %s: %q", content.Name, err)
9796
}
9897
return csiSnapshotStatus, timestamp, size, nil
9998

0 commit comments

Comments
 (0)