From e65dddd192ef9944ec997f96ecfe61bc02e67d1f Mon Sep 17 00:00:00 2001 From: alice-px Date: Thu, 25 Jan 2024 16:27:35 -0800 Subject: [PATCH] PWX-35577: correlation tracing for Snapshot function (#2407) --- api/client/volume/client.go | 6 +----- api/server/sdk/volume_ops.go | 2 +- api/server/sdk/volume_ops_test.go | 10 +++++----- api/server/sdk/volume_snapshot.go | 2 +- api/server/sdk/volume_snapshot_test.go | 2 +- api/server/volume_test.go | 12 ++++++------ cli/volumes.go | 2 +- csi/controller_test.go | 8 ++++---- pkg/sanity/snapshot.go | 6 +++--- volume/drivers/btrfs/btrfs.go | 4 ++-- volume/drivers/buse/buse.go | 2 +- volume/drivers/fake/fake.go | 2 +- volume/drivers/mock/driver.mock.go | 8 ++++---- volume/drivers/nfs/nfs.go | 2 +- volume/drivers/test/driver.go | 6 ++---- volume/volume.go | 2 +- volume/volume_not_supported.go | 2 +- 17 files changed, 36 insertions(+), 42 deletions(-) diff --git a/api/client/volume/client.go b/api/client/volume/client.go index 22e9073f9..5a03c06f2 100644 --- a/api/client/volume/client.go +++ b/api/client/volume/client.go @@ -200,11 +200,7 @@ func (v *volumeClient) Delete(ctx context.Context, volumeID string) error { // Snap specified volume. IO to the underlying volume should be quiesced before // calling this function. // Errors ErrEnoEnt may be returned -func (v *volumeClient) Snapshot(volumeID string, - readonly bool, - locator *api.VolumeLocator, - noRetry bool, -) (string, error) { +func (v *volumeClient) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { response := &api.SnapCreateResponse{} request := &api.SnapCreateRequest{ Id: volumeID, diff --git a/api/server/sdk/volume_ops.go b/api/server/sdk/volume_ops.go index e3cbebfb8..12931d788 100644 --- a/api/server/sdk/volume_ops.go +++ b/api/server/sdk/volume_ops.go @@ -171,7 +171,7 @@ func (s *VolumeServer) create( } // Create a snapshot from the parent - id, err = s.driver(ctx).Snapshot(parent.GetId(), false, &api.VolumeLocator{ + id, err = s.driver(ctx).Snapshot(ctx, parent.GetId(), false, &api.VolumeLocator{ Name: volName, }, false) if err != nil { diff --git a/api/server/sdk/volume_ops_test.go b/api/server/sdk/volume_ops_test.go index a4b86f85d..00a66d13d 100644 --- a/api/server/sdk/volume_ops_test.go +++ b/api/server/sdk/volume_ops_test.go @@ -313,7 +313,7 @@ func TestSdkVolumeClone(t *testing.T) { s.MockDriver(). EXPECT(). - Snapshot(parentid, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parentid, false, &api.VolumeLocator{Name: name}, false). Return(id, nil). Times(1), @@ -1087,7 +1087,7 @@ func TestSdkCloneOwnership(t *testing.T) { mv. EXPECT(). - Snapshot(parentid, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parentid, false, &api.VolumeLocator{Name: name}, false). Return(id, nil). Times(1), @@ -1134,7 +1134,7 @@ func TestSdkCloneOwnership(t *testing.T) { mv. EXPECT(). - Snapshot(parentid, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parentid, false, &api.VolumeLocator{Name: name}, false). Return(id, nil). Times(1), @@ -1205,7 +1205,7 @@ func TestSdkCloneOwnership(t *testing.T) { mv. EXPECT(). - Snapshot(parentid, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parentid, false, &api.VolumeLocator{Name: name}, false). Return(id, nil). Times(1), @@ -1261,7 +1261,7 @@ func TestSdkCloneOwnership(t *testing.T) { mv. EXPECT(). - Snapshot(parentid, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parentid, false, &api.VolumeLocator{Name: name}, false). Return(id, nil). Times(1), diff --git a/api/server/sdk/volume_snapshot.go b/api/server/sdk/volume_snapshot.go index fdf0b323a..3d2cd72fb 100644 --- a/api/server/sdk/volume_snapshot.go +++ b/api/server/sdk/volume_snapshot.go @@ -47,7 +47,7 @@ func (s *VolumeServer) SnapshotCreate( } readonly := true - snapshotID, err := s.driver(ctx).Snapshot(req.GetVolumeId(), readonly, &api.VolumeLocator{ + snapshotID, err := s.driver(ctx).Snapshot(ctx, req.GetVolumeId(), readonly, &api.VolumeLocator{ Name: req.GetName(), VolumeLabels: req.GetLabels(), }, false) diff --git a/api/server/sdk/volume_snapshot_test.go b/api/server/sdk/volume_snapshot_test.go index 3e432582e..b93c3e620 100644 --- a/api/server/sdk/volume_snapshot_test.go +++ b/api/server/sdk/volume_snapshot_test.go @@ -78,7 +78,7 @@ func TestSdkVolumeSnapshotCreate(t *testing.T) { Times(1) s.MockDriver(). EXPECT(). - Snapshot(req.GetVolumeId(), true, &api.VolumeLocator{ + Snapshot(gomock.Any(), req.GetVolumeId(), true, &api.VolumeLocator{ Name: snapName, }, false). Return(snapid, nil). diff --git a/api/server/volume_test.go b/api/server/volume_test.go index f0ecec172..aedacf861 100644 --- a/api/server/volume_test.go +++ b/api/server/volume_test.go @@ -471,7 +471,7 @@ func TestVolumeSnapshotCreateSuccess(t *testing.T) { Readonly: true, } - _, err = driverclient.Snapshot(id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + _, err = driverclient.Snapshot(context.TODO(), id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Nil(t, err) _, err = volumes.Delete(ctx, &api.SdkVolumeDeleteRequest{ @@ -529,7 +529,7 @@ func TestVolumeSnapshotCreateFailed(t *testing.T) { Readonly: true, } - res, _ := driverclient.Snapshot("doesnotexist", req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + res, _ := driverclient.Snapshot(context.TODO(), "doesnotexist", req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Equal(t, "", res) _, err = volumes.Delete(ctx, &api.SdkVolumeDeleteRequest{ @@ -696,7 +696,7 @@ func TestVolumeSnapshotList(t *testing.T) { Readonly: true, } - _, err = driverclient.Snapshot(id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + _, err = driverclient.Snapshot(context.TODO(), id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Nil(t, err) res, err := driverclient.SnapEnumerate([]string{id}, nil) @@ -704,7 +704,7 @@ func TestVolumeSnapshotList(t *testing.T) { assert.NotNil(t, res) assert.Len(t, res, 1) - _, err = driverclient.Snapshot(id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + _, err = driverclient.Snapshot(context.TODO(), id, req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Nil(t, err) res, err = driverclient.SnapEnumerate([]string{id}, nil) @@ -1651,7 +1651,7 @@ func TestVolumeRestoreSuccess(t *testing.T) { Readonly: true, } - res, err := driverclient.Snapshot(req2.GetId(), req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + res, err := driverclient.Snapshot(context.TODO(), req2.GetId(), req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Nil(t, err) // create client @@ -1712,7 +1712,7 @@ func TestVolumeRestoreFailed(t *testing.T) { Readonly: true, } - _, err = driverclient.Snapshot(req2.GetId(), req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) + _, err = driverclient.Snapshot(context.TODO(), req2.GetId(), req2.GetReadonly(), req2.GetLocator(), req2.GetNoRetry()) assert.Nil(t, err) // create client diff --git a/cli/volumes.go b/cli/volumes.go index b3a2a56f6..956df1082 100644 --- a/cli/volumes.go +++ b/cli/volumes.go @@ -304,7 +304,7 @@ func (v *volDriver) snapCreate(cliContext *cli.Context) { } readonly := cliContext.Bool("readonly") noRetry := cliContext.Bool("noretry") - id, err := v.volDriver.Snapshot(volumeID, readonly, locator, noRetry) + id, err := v.volDriver.Snapshot(context.TODO(), volumeID, readonly, locator, noRetry) if err != nil { cmdError(cliContext, fn, err) return diff --git a/csi/controller_test.go b/csi/controller_test.go index 6bf9d242d..cac51e16b 100644 --- a/csi/controller_test.go +++ b/csi/controller_test.go @@ -1331,7 +1331,7 @@ func TestControllerCreateVolumeBadSnapshot(t *testing.T) { // Return an error from snapshot s.MockDriver(). EXPECT(). - Snapshot(parent, false, &api.VolumeLocator{Name: name}, false). + Snapshot(gomock.Any(), parent, false, &api.VolumeLocator{Name: name}, false). Return("", fmt.Errorf("snapshoterr")). Times(1), ) @@ -1937,7 +1937,7 @@ func TestControllerCreateVolumeFromSnapshot(t *testing.T) { // create s.MockDriver(). EXPECT(). - Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(snapID, nil). Times(1), s.MockDriver(). @@ -2044,7 +2044,7 @@ func TestControllerCreateVolumeSnapshotThroughParameters(t *testing.T) { // create snap s.MockDriver(). EXPECT(). - Snapshot(mockParentID, false, &api.VolumeLocator{ + Snapshot(gomock.Any(), mockParentID, false, &api.VolumeLocator{ Name: name, }, false). @@ -2933,7 +2933,7 @@ func TestControllerCreateSnapshot(t *testing.T) { // snapshot s.MockDriver(). EXPECT(). - Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Snapshot(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(snapId, nil). Times(1), diff --git a/pkg/sanity/snapshot.go b/pkg/sanity/snapshot.go index 2def3186d..9d6180c9e 100644 --- a/pkg/sanity/snapshot.go +++ b/pkg/sanity/snapshot.go @@ -115,7 +115,7 @@ var _ = Describe("Volume [Snapshot Tests]", func() { Name: "snapshot-of-" + volumeID, } - snapID, err = volumedriver.Snapshot(volumeID, true, loc, false) + snapID, err = volumedriver.Snapshot(context.TODO(), volumeID, true, loc, false) Expect(err).NotTo(HaveOccurred()) Expect(snapID).To(Not(BeNil())) @@ -204,7 +204,7 @@ var _ = Describe("Volume [Snapshot Tests]", func() { Name: "snapshot-" + strconv.Itoa(i) + "-of-" + volumeID, } - snapID, err = volumedriver.Snapshot(volumeID, true, loc, false) + snapID, err = volumedriver.Snapshot(context.TODO(), volumeID, true, loc, false) Expect(err).NotTo(HaveOccurred()) Expect(snapID).To(Not(BeNil())) @@ -298,7 +298,7 @@ var _ = Describe("Volume [Snapshot Tests]", func() { Name: "snapshot-of-" + volumeID, } - snapID, err = volumedriver.Snapshot(volumeID, true, loc, false) + snapID, err = volumedriver.Snapshot(context.TODO(), volumeID, true, loc, false) Expect(err).NotTo(HaveOccurred()) Expect(snapID).To(Not(BeNil())) diff --git a/volume/drivers/btrfs/btrfs.go b/volume/drivers/btrfs/btrfs.go index ffcfe8a31..e93ae23bb 100644 --- a/volume/drivers/btrfs/btrfs.go +++ b/volume/drivers/btrfs/btrfs.go @@ -145,7 +145,7 @@ func (d *driver) Unmount(ctx context.Context, volumeID string, mountpath string) return d.UpdateVol(v) } -func (d *driver) Set(volumeID string, locator *api.VolumeLocator, spec *api.VolumeSpec) error { +func (d *driver) Set(ctx context.Context, volumeID string, locator *api.VolumeLocator, spec *api.VolumeSpec) error { if spec != nil { return volume.ErrNotSupported } @@ -160,7 +160,7 @@ func (d *driver) Set(volumeID string, locator *api.VolumeLocator, spec *api.Volu } // Snapshot create new subvolume from volume -func (d *driver) Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { +func (d *driver) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { vols, err := d.Inspect([]string{volumeID}) if err != nil { return "", err diff --git a/volume/drivers/buse/buse.go b/volume/drivers/buse/buse.go index 8cc136837..b76fe7bb5 100644 --- a/volume/drivers/buse/buse.go +++ b/volume/drivers/buse/buse.go @@ -324,7 +324,7 @@ func (d *driver) Unmount(ctx context.Context, volumeID string, mountpath string, return d.UpdateVol(v) } -func (d *driver) Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { +func (d *driver) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { volIDs := make([]string, 1) volIDs[0] = volumeID vols, err := d.Inspect(nil, volIDs) diff --git a/volume/drivers/fake/fake.go b/volume/drivers/fake/fake.go index 6fd57c8bf..9eb20e561 100644 --- a/volume/drivers/fake/fake.go +++ b/volume/drivers/fake/fake.go @@ -277,7 +277,7 @@ func (d *driver) Unmount(ctx context.Context, volumeID string, mountpath string, return d.UpdateVol(v) } -func (d *driver) Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { +func (d *driver) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { if len(locator.GetName()) == 0 { return "", fmt.Errorf("Name for snapshot must be provided") diff --git a/volume/drivers/mock/driver.mock.go b/volume/drivers/mock/driver.mock.go index ed286c982..02132118f 100644 --- a/volume/drivers/mock/driver.mock.go +++ b/volume/drivers/mock/driver.mock.go @@ -913,18 +913,18 @@ func (mr *MockVolumeDriverMockRecorder) SnapEnumerate(arg0, arg1 interface{}) *g } // Snapshot mocks base method. -func (m *MockVolumeDriver) Snapshot(arg0 string, arg1 bool, arg2 *api.VolumeLocator, arg3 bool) (string, error) { +func (m *MockVolumeDriver) Snapshot(arg0 context.Context, arg1 string, arg2 bool, arg3 *api.VolumeLocator, arg4 bool) (string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Snapshot", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "Snapshot", arg0, arg1, arg2, arg3, arg4) ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } // Snapshot indicates an expected call of Snapshot. -func (mr *MockVolumeDriverMockRecorder) Snapshot(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +func (mr *MockVolumeDriverMockRecorder) Snapshot(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Snapshot", reflect.TypeOf((*MockVolumeDriver)(nil).Snapshot), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Snapshot", reflect.TypeOf((*MockVolumeDriver)(nil).Snapshot), arg0, arg1, arg2, arg3, arg4) } // SnapshotGroup mocks base method. diff --git a/volume/drivers/nfs/nfs.go b/volume/drivers/nfs/nfs.go index 0cc8a5002..ea4558735 100644 --- a/volume/drivers/nfs/nfs.go +++ b/volume/drivers/nfs/nfs.go @@ -674,7 +674,7 @@ func (d *driver) clone(newVolumeID, volumeID string) error { return nil } -func (d *driver) Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { +func (d *driver) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { volIDs := []string{volumeID} vols, err := d.Inspect(nil, volIDs) if err != nil { diff --git a/volume/drivers/test/driver.go b/volume/drivers/test/driver.go index 5e867b402..7138dc314 100644 --- a/volume/drivers/test/driver.go +++ b/volume/drivers/test/driver.go @@ -149,7 +149,7 @@ func set(t *testing.T, ctx *Context) { require.Equal(t, vols[0].Id, ctx.volID, "Expect volID %v actual %v", ctx.volID, vols[0].Id) vols[0].Locator.VolumeLabels["UpdateTest"] = "Success" - err = ctx.Set(correlation.TODO(), ctx.volID, vols[0].Locator, nil) + err = ctx.Set(context.TODO(), ctx.volID, vols[0].Locator, nil) if err != volume.ErrNotSupported { require.NoError(t, err, "Failed in Update") vols, err = ctx.Inspect(correlation.TODO(), []string{ctx.volID}) @@ -321,9 +321,7 @@ func snap(t *testing.T, ctx *Context) { attach(t, ctx) labels := map[string]string{"oh": "snap"} require.NotEqual(t, ctx.volID, "", "invalid volume ID") - id, err := ctx.Snapshot(ctx.volID, false, - &api.VolumeLocator{Name: "snappy", VolumeLabels: labels}, - false) + id, err := ctx.Snapshot(context.TODO(), ctx.volID, false, &api.VolumeLocator{Name: "snappy", VolumeLabels: labels}, false) require.NoError(t, err, "Failed in creating a snapshot") ctx.snapID = id } diff --git a/volume/volume.go b/volume/volume.go index cbd70c12d..e86817117 100644 --- a/volume/volume.go +++ b/volume/volume.go @@ -118,7 +118,7 @@ type IODriver interface { type SnapshotDriver interface { // Snapshot create volume snapshot. // Errors ErrEnoEnt may be returned - Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) + Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) // Restore restores volume to specified snapshot. Restore(volumeID string, snapshotID string) error // SnapshotGroup takes a snapshot of a group of volumes that can be specified with either of the following diff --git a/volume/volume_not_supported.go b/volume/volume_not_supported.go index d230029b4..7a344d483 100644 --- a/volume/volume_not_supported.go +++ b/volume/volume_not_supported.go @@ -52,7 +52,7 @@ func (b *blockNotSupported) Detach(ctx context.Context, volumeID string, options type snapshotNotSupported struct{} -func (s *snapshotNotSupported) Snapshot(volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { +func (s *snapshotNotSupported) Snapshot(ctx context.Context, volumeID string, readonly bool, locator *api.VolumeLocator, noRetry bool) (string, error) { return "", ErrNotSupported }