Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[cinder-csi-plugin] Fix sanity failures 3/3 (kubernetes#1057)
  • Loading branch information
ramineni committed May 6, 2020
1 parent bff65f4 commit 31ab896
Show file tree
Hide file tree
Showing 8 changed files with 146 additions and 96 deletions.
20 changes: 20 additions & 0 deletions .zuul.yaml
Expand Up @@ -85,6 +85,26 @@
- go.mod$
- go.sum$
- Makefile$
irrelevant-files:
- docs/.*$
- ^.*\.md$
- ^OWNERS$
- ^SECURITY_CONTACTS$
cloud-provider-openstack-sanity-test-csi-cinder:
jobs:
- cloud-provider-openstack-sanity-test-csi-cinder:
files:
- cmd/cinder-csi-plugin/.*
- pkg/csi/cinder/.*
- pkg/util/.*
- go.mod$
- go.sum$
- Makefile$
irrelevant-files:
- docs/.*$
- ^.*\.md$
- ^OWNERS$
- ^SECURITY_CONTACTS$
cloud-provider-openstack-multinode-csi-migration-test:
jobs:
- cloud-provider-openstack-multinode-csi-migration-test:
Expand Down
58 changes: 33 additions & 25 deletions pkg/csi/cinder/controllerserver.go
Expand Up @@ -18,6 +18,7 @@ package cinder

import (
"fmt"
"strconv"

"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/golang/protobuf/ptypes"
Expand Down Expand Up @@ -303,7 +304,7 @@ func (cs *controllerServer) ListVolumes(ctx context.Context, req *csi.ListVolume

func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequest) (*csi.CreateSnapshotResponse, error) {
name := req.Name
volumeId := req.SourceVolumeId
volumeId := req.GetSourceVolumeId()

if name == "" {
return nil, status.Error(codes.InvalidArgument, "Snapshot name must be provided in CreateSnapshot request")
Expand All @@ -314,7 +315,9 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
}

// Verify a snapshot with the provided name doesn't already exist for this tenant
snapshots, err := cs.Cloud.GetSnapshotByNameAndVolumeID(name, volumeId)
filters := map[string]string{}
filters["Name"] = name
snapshots, _, err := cs.Cloud.ListSnapshots(filters)
if err != nil {
klog.V(3).Infof("Failed to query for existing Snapshot during CreateSnapshot: %v", err)
}
Expand Down Expand Up @@ -368,7 +371,7 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS

func (cs *controllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequest) (*csi.DeleteSnapshotResponse, error) {

id := req.SnapshotId
id := req.GetSnapshotId()

if id == "" {
return nil, status.Error(codes.InvalidArgument, "Snapshot ID must be provided in DeleteSnapshot request")
Expand All @@ -389,11 +392,15 @@ func (cs *controllerServer) DeleteSnapshot(ctx context.Context, req *csi.DeleteS

func (cs *controllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) {

if len(req.GetSnapshotId()) != 0 {
snap, err := cs.Cloud.GetSnapshotByID(req.SnapshotId)
snapshotID := req.GetSnapshotId()
if len(snapshotID) != 0 {
snap, err := cs.Cloud.GetSnapshotByID(snapshotID)
if err != nil {
klog.V(3).Infof("Failed to Get snapshot: %v", err)
return &csi.ListSnapshotsResponse{}, nil
if cpoerrors.IsNotFound(err) {
klog.V(3).Infof("Snapshot %s not found", snapshotID)
return &csi.ListSnapshotsResponse{}, nil
}
return nil, status.Errorf(codes.Internal, "Failed to GetSnapshot %s : %v", snapshotID, err)
}

ctime, err := ptypes.TimestampProto(snap.CreatedAt)
Expand All @@ -415,35 +422,35 @@ func (cs *controllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnap

}

// FIXME: honor the limit, offset and filters later
filters := map[string]string{}

var vlist []snapshots.Snapshot
var slist []snapshots.Snapshot
var err error
var nextPageToken string

// Add the filters
if len(req.GetSourceVolumeId()) != 0 {
vlist, err = cs.Cloud.GetSnapshotByNameAndVolumeID("", req.GetSourceVolumeId())
if err != nil {
klog.V(3).Infof("Failed to ListSnapshots: %v", err)
return nil, status.Error(codes.Internal, fmt.Sprintf("ListSnapshots get snapshot failed with error %v", err))
}
filters["VolumeID"] = req.GetSourceVolumeId()
} else {
vlist, err = cs.Cloud.ListSnapshots(int(req.MaxEntries), 0, filters)
if err != nil {
klog.V(3).Infof("Failed to ListSnapshots: %v", err)
return nil, status.Error(codes.Internal, fmt.Sprintf("ListSnapshots get snapshot failed with error %v", err))

}
filters["Limit"] = strconv.Itoa(int(req.MaxEntries))
filters["Marker"] = req.StartingToken
}

// Only retrieve snapshots that are available
filters["Status"] = "available"
slist, nextPageToken, err = cs.Cloud.ListSnapshots(filters)
if err != nil {
klog.V(3).Infof("Failed to ListSnapshots: %v", err)
return nil, status.Errorf(codes.Internal, "ListSnapshots failed with error %v", err)
}

var ventries []*csi.ListSnapshotsResponse_Entry
for _, v := range vlist {
var sentries []*csi.ListSnapshotsResponse_Entry
for _, v := range slist {
ctime, err := ptypes.TimestampProto(v.CreatedAt)
if err != nil {
klog.Errorf("Error to convert time to timestamp: %v", err)
}
ventry := csi.ListSnapshotsResponse_Entry{
sentry := csi.ListSnapshotsResponse_Entry{
Snapshot: &csi.Snapshot{
SizeBytes: int64(v.Size * 1024 * 1024 * 1024),
SnapshotId: v.ID,
Expand All @@ -452,10 +459,11 @@ func (cs *controllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnap
ReadyToUse: true,
},
}
ventries = append(ventries, &ventry)
sentries = append(sentries, &sentry)
}
return &csi.ListSnapshotsResponse{
Entries: ventries,
Entries: sentries,
NextToken: nextPageToken,
}, nil

}
Expand Down
12 changes: 4 additions & 8 deletions pkg/csi/cinder/controllerserver_test.go
Expand Up @@ -359,7 +359,8 @@ func TestListVolumes(t *testing.T) {
// Test CreateSnapshot
func TestCreateSnapshot(t *testing.T) {

osmock.On("CreateSnapshot", FakeSnapshotName, FakeVolID, "", &map[string]string{"tag": "tag1"}).Return(&FakeSnapshotRes, nil)
osmock.On("CreateSnapshot", FakeSnapshotName, FakeVolID, &map[string]string{"tag": "tag1"}).Return(&FakeSnapshotRes, nil)
osmock.On("ListSnapshots", map[string]string{"Name": FakeSnapshotName}).Return(FakeSnapshotListEmpty, "", nil)
osmock.On("WaitSnapshotReady", FakeSnapshotID).Return(nil)

// Init assert
Expand Down Expand Up @@ -413,22 +414,17 @@ func TestDeleteSnapshot(t *testing.T) {

func TestListSnapshots(t *testing.T) {

osmock.On("ListSnapshots", 0, 0, map[string]string{}).Return(FakeSnapshotsRes, nil)

// Init assert
osmock.On("ListSnapshots", map[string]string{"Limit": "1", "Marker": FakeVolID, "Status": "available"}).Return(FakeSnapshotsRes, "", nil)
assert := assert.New(t)

fakeReq := &csi.ListSnapshotsRequest{}

// Invoke ListVolumes
fakeReq := &csi.ListSnapshotsRequest{MaxEntries: 1, StartingToken: FakeVolID}
actualRes, err := fakeCs.ListSnapshots(FakeCtx, fakeReq)
if err != nil {
t.Errorf("failed to ListSnapshots: %v", err)
}

// Assert
assert.Equal(FakeVolID, actualRes.Entries[0].Snapshot.SourceVolumeId)

assert.NotNil(FakeSnapshotID, actualRes.Entries[0].Snapshot.SnapshotId)
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/csi/cinder/fake.go
Expand Up @@ -88,13 +88,15 @@ var FakeSnapshotRes = snapshots.Snapshot{
ID: FakeSnapshotID,
Name: "fake-snapshot",
VolumeID: FakeVolID,
Size: 1,
}

var FakeSnapshotsRes = []snapshots.Snapshot{FakeSnapshotRes}

var FakeVolListMultiple = []volumes.Volume{FakeVol1, FakeVol3}
var FakeVolList = []volumes.Volume{FakeVol1}
var FakeVolListEmpty = []volumes.Volume{}
var FakeSnapshotListEmpty = []snapshots.Snapshot{}

var FakeInstanceID = "321a8b81-3660-43e5-bab8-6470b65ee4e8"

Expand Down
3 changes: 1 addition & 2 deletions pkg/csi/cinder/openstack/openstack.go
Expand Up @@ -53,9 +53,8 @@ type IOpenStack interface {
GetVolume(volumeID string) (*volumes.Volume, error)
GetVolumesByName(name string) ([]volumes.Volume, error)
CreateSnapshot(name, volID string, tags *map[string]string) (*snapshots.Snapshot, error)
ListSnapshots(limit, offset int, filters map[string]string) ([]snapshots.Snapshot, error)
ListSnapshots(filters map[string]string) ([]snapshots.Snapshot, string, error)
DeleteSnapshot(snapID string) error
GetSnapshotByNameAndVolumeID(n string, volumeId string) ([]snapshots.Snapshot, error)
GetSnapshotByID(snapshotID string) (*snapshots.Snapshot, error)
WaitSnapshotReady(snapshotID string) error
GetInstanceByID(instanceID string) (*servers.Server, error)
Expand Down
33 changes: 18 additions & 15 deletions pkg/csi/cinder/openstack/openstack_mock.go
Expand Up @@ -209,26 +209,35 @@ func (_m *OpenStackMock) GetVolumesByName(name string) ([]volumes.Volume, error)
}

// ListSnapshots provides a mock function with given fields: limit, offset, filters
func (_m *OpenStackMock) ListSnapshots(limit int, offset int, filters map[string]string) ([]snapshots.Snapshot, error) {
ret := _m.Called(limit, offset, filters)
func (_m *OpenStackMock) ListSnapshots(filters map[string]string) ([]snapshots.Snapshot, string, error) {
ret := _m.Called(filters)

var r0 []snapshots.Snapshot
if rf, ok := ret.Get(0).(func(int, int, map[string]string) []snapshots.Snapshot); ok {
r0 = rf(limit, offset, filters)
if rf, ok := ret.Get(0).(func(map[string]string) []snapshots.Snapshot); ok {
r0 = rf(filters)
} else {
if ret.Get(0) != nil {
r0 = ret.Get(0).([]snapshots.Snapshot)
}
}
var r1 string

var r1 error
if rf, ok := ret.Get(1).(func(int, int, map[string]string) error); ok {
r1 = rf(limit, offset, filters)
if rf, ok := ret.Get(1).(func(map[string]string) string); ok {
r1 = rf(filters)
} else {
r1 = ret.Error(1)
if ret.Get(1) != nil {
r1 = ret.Get(1).(string)
}
}

return r0, r1
var r2 error
if rf, ok := ret.Get(2).(func(map[string]string) error); ok {
r2 = rf(filters)
} else {
r2 = ret.Error(2)
}

return r0, r1, r2
}

// CreateSnapshot provides a mock function with given fields: name, volID, tags
Expand Down Expand Up @@ -301,12 +310,6 @@ func (_m *OpenStackMock) ListVolumes(limit int, marker string) ([]volumes.Volume
return r0, r1, r2
}

func (_m *OpenStackMock) GetSnapshotByNameAndVolumeID(n string, volumeId string) ([]snapshots.Snapshot, error) {
var slist []snapshots.Snapshot
slist = append(slist, fakeSnapshot)
return slist, nil
}

func (_m *OpenStackMock) GetAvailabilityZone() (string, error) {
ret := _m.Called()
var r0 string
Expand Down
63 changes: 35 additions & 28 deletions pkg/csi/cinder/openstack/openstack_snapshots.go
Expand Up @@ -20,6 +20,7 @@ package openstack

import (
"fmt"
"net/url"
"strconv"
"time"

Expand Down Expand Up @@ -78,42 +79,48 @@ func (os *OpenStack) CreateSnapshot(name, volID string, tags *map[string]string)
// ListSnapshots retrieves a list of active snapshots from Cinder for the corresponding Tenant. We also
// provide the ability to provide limit and offset to enable the consumer to provide accurate pagination.
// In addition the filters argument provides a mechanism for passing in valid filter strings to the list
// operation. Valid filter keys are: Name, Status, VolumeID (TenantID has no effect)
func (os *OpenStack) ListSnapshots(limit, offset int, filters map[string]string) ([]snapshots.Snapshot, error) {
// FIXME: honor the limit, offset and filters later
opts := snapshots.ListOpts{Status: snapshotReadyStatus}
pages, err := snapshots.List(os.blockstorage, opts).AllPages()
if err != nil {
klog.V(3).Infof("Failed to retrieve snapshots from Cinder: %v", err)
return nil, err
}
snaps, err := snapshots.ExtractSnapshots(pages)
if err != nil {
klog.V(3).Infof("Failed to extract snapshot pages from Cinder: %v", err)
return nil, err
// operation. Valid filter keys are: Name, Status, VolumeID, Limit, Marker (TenantID has no effect)
func (os *OpenStack) ListSnapshots(filters map[string]string) ([]snapshots.Snapshot, string, error) {
// Build the Opts
opts := snapshots.ListOpts{}
nextPageToken := ""

for key, val := range filters {
switch key {
case "Status":
opts.Status = val
case "Name":
opts.Name = val
case "VolumeID":
opts.VolumeID = val
case "Marker":
opts.Marker = val
case "Limit":
opts.Limit, _ = strconv.Atoi(val)
default:
klog.V(3).Infof("Not a valid filter key %s", key)
}
}
// There's little value in rewrapping these gophercloud types into yet another abstraction/type, instead just
// return the gophercloud item
return snaps, nil

}

// GetSnapshotByNameAndVolumeID returns a list of snapshot references with the specified name and volume ID
func (os *OpenStack) GetSnapshotByNameAndVolumeID(n string, volumeId string) ([]snapshots.Snapshot, error) {
opts := snapshots.ListOpts{Name: n, VolumeID: volumeId}
pages, err := snapshots.List(os.blockstorage, opts).AllPages()
if err != nil {
klog.V(3).Infof("Failed to retrieve snapshots from Cinder: %v", err)
return nil, err
klog.V(3).Infof("Failed to retrieve snapshots: %v", err)
return nil, nextPageToken, err
}
snaps, err := snapshots.ExtractSnapshots(pages)
if err != nil {
klog.V(3).Infof("Failed to extract snapshot pages from Cinder: %v", err)
return nil, err
klog.V(3).Infof("Failed to extract snapshot pages: %v", err)
return nil, nextPageToken, err
}
// There's little value in rewrapping these gophercloud types into yet another abstraction/type, instead just
// return the gophercloud item
return snaps, nil

nextPageURL, err := pages.NextPageURL()
if err != nil && nextPageURL != "" {
if queryParams, nerr := url.ParseQuery(nextPageURL); nerr != nil {
nextPageToken = queryParams.Get("marker")
}
}
return snaps, nextPageToken, nil

}

// DeleteSnapshot issues a request to delete the Snapshot with the specified ID from the Cinder backend
Expand Down

0 comments on commit 31ab896

Please sign in to comment.