Skip to content

Commit

Permalink
PWX-35577: correlation tracing for Snapshot function (#2407)
Browse files Browse the repository at this point in the history 
Signed-off-by: Adam Krpan <[email protected]>
  • Loading branch information
@alicelyy @Pure-AdamuKaapan
alicelyy authored and Pure-AdamuKaapan committed Feb 13, 2024
1 parent 967cfd8 commit 923878b
Show file tree
Hide file tree
Showing 19 changed files with 48 additions and 94 deletions.
6 changes: 1 addition & 5 deletions api/client/volume/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,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,
Expand Down
2 changes: 1 addition & 1 deletion api/server/sdk/volume_ops.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 5 additions & 5 deletions api/server/sdk/volume_ops_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down Expand Up @@ -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),

Expand Down Expand Up @@ -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),

Expand Down Expand Up @@ -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),

Expand Down Expand Up @@ -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),

Expand Down
2 changes: 1 addition & 1 deletion api/server/sdk/volume_snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion api/server/sdk/volume_snapshot_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
29 changes: 0 additions & 29 deletions api/server/volume.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1486,35 +1486,6 @@ func (vd *volAPI) volumeusage(w http.ResponseWriter, r *http.Request) {
json.NewEncoder(w).Encode(capacityInfo)
}

func (vd *volAPI) volumeBytesUsedByNode(w http.ResponseWriter, r *http.Request) {
var err error

method := "volumeBytesUsedByNode"
var req api.SdkVolumeBytesUsedRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
vd.sendError(vd.name, method, w, err.Error(), http.StatusBadRequest)
return
}
d, err := vd.getVolDriver(r)
if err != nil {
notFound(w, r)
return
}

volUtilInfo, err := d.VolumeBytesUsedByNode(req.NodeId, req.Ids)
if err != nil {
var e error
if err != nil {
e = fmt.Errorf("Failed to get volumeBytesUsedByNode: %s", err.Error())
}
vd.sendError(vd.name, method, w, e.Error(), http.StatusInternalServerError)
return
}
var result api.SdkVolumeBytesUsedResponse
result.VolUtilInfo = volUtilInfo
json.NewEncoder(w).Encode(&result)
}

// swagger:operation GET /osd-volumes/quiesce/{id} volume quiesceVolume
//
// Quiesce volume with specified id.
Expand Down
12 changes: 6 additions & 6 deletions api/server/volume_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -696,15 +696,15 @@ 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)
assert.Nil(t, err)
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)
Expand Down Expand Up @@ -1650,7 +1650,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
Expand Down Expand Up @@ -1711,7 +1711,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
Expand Down
2 changes: 1 addition & 1 deletion cli/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions csi/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,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),
)
Expand Down Expand Up @@ -1929,7 +1929,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().
Expand Down Expand Up @@ -2036,7 +2036,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).
Expand Down Expand Up @@ -2925,7 +2925,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),

Expand Down
21 changes: 10 additions & 11 deletions csi/v0.3/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ func (s *OsdCsiServer) ControllerUnpublishVolume(
// Note: The method used here to return errors is still not part of the spec.
// See: https://github.com/container-storage-interface/spec/pull/115
// Discussion: https://groups.google.com/forum/#!topic/kubernetes-sig-storage-wg-csi/TpTrNFbRa1I
//
func (s *OsdCsiServer) ValidateVolumeCapabilities(
ctx context.Context,
req *csi.ValidateVolumeCapabilitiesRequest,
Expand All @@ -133,7 +132,7 @@ func (s *OsdCsiServer) ValidateVolumeCapabilities(
attributes)

// Check ID is valid with the specified volume capabilities
volumes, err := s.driver.Inspect([]string{id})
volumes, err := s.driver.Inspect(ctx, []string{id})
if err != nil || len(volumes) == 0 {
return nil, status.Error(codes.NotFound, "ID not found")
}
Expand Down Expand Up @@ -343,7 +342,7 @@ func (s *OsdCsiServer) CreateVolume(
}

// Check if the volume has already been created or is in process of creation
v, err := util.VolumeFromName(s.driver, req.GetName())
v, err := util.VolumeFromName(ctx, s.driver, req.GetName())
if err == nil {
// Check the requested arguments match that of the existing volume
if spec.Size != v.GetSpec().GetSize() {
Expand Down Expand Up @@ -378,15 +377,15 @@ func (s *OsdCsiServer) CreateVolume(
var id string
if source != nil && len(source.GetParent()) != 0 {
// Get parent volume information
parent, err := util.VolumeFromName(s.driver, source.Parent)
parent, err := util.VolumeFromName(ctx, s.driver, source.Parent)
if err != nil {
e := fmt.Sprintf("unable to get parent volume information: %s\n", err.Error())
logrus.Errorln(e)
return nil, status.Error(codes.InvalidArgument, e)
}

// Create a snapshot from the parent
id, err = s.driver.Snapshot(parent.GetId(), false, &api.VolumeLocator{
id, err = s.driver.Snapshot(ctx, parent.GetId(), false, &api.VolumeLocator{
Name: req.GetName(),
},
false)
Expand Down Expand Up @@ -415,7 +414,7 @@ func (s *OsdCsiServer) CreateVolume(
}

// id must have been set
v, err = util.VolumeFromName(s.driver, id)
v, err = util.VolumeFromName(ctx, s.driver, id)
if err != nil {
e := fmt.Sprintf("Unable to find newly created volume: %s", err.Error())
logrus.Errorln(e)
Expand All @@ -440,7 +439,7 @@ func (s *OsdCsiServer) DeleteVolume(
}

// If the volume is not found, then we can return OK
volumes, err := s.driver.Inspect([]string{req.GetVolumeId()})
volumes, err := s.driver.Inspect(ctx, []string{req.GetVolumeId()})
if (err == nil && len(volumes) == 0) ||
(err != nil && err == kvdb.ErrNotFound) {
return &csi.DeleteVolumeResponse{}, nil
Expand Down Expand Up @@ -492,7 +491,7 @@ func (s *OsdCsiServer) CreateSnapshot(
}

// Check if the snapshot with this name already exists
v, err := util.VolumeFromName(s.driver, req.GetName())
v, err := util.VolumeFromName(ctx, s.driver, req.GetName())
if err == nil {
// Verify the parent is the same
if req.GetSourceVolumeId() != v.GetSource().GetParent() {
Expand Down Expand Up @@ -526,7 +525,7 @@ func (s *OsdCsiServer) CreateSnapshot(

// Create snapshot
readonly := true
snapshotID, err := s.driver.Snapshot(req.GetSourceVolumeId(), readonly, &api.VolumeLocator{
snapshotID, err := s.driver.Snapshot(ctx, req.GetSourceVolumeId(), readonly, &api.VolumeLocator{
Name: req.GetName(),
VolumeLabels: locator.GetVolumeLabels(),
}, false)
Expand All @@ -537,7 +536,7 @@ func (s *OsdCsiServer) CreateSnapshot(
return nil, status.Errorf(codes.Internal, "Failed to create snapshot: %v", err)
}

snapInfo, err := util.VolumeFromName(s.driver, snapshotID)
snapInfo, err := util.VolumeFromName(ctx, s.driver, snapshotID)
if err != nil {
return nil, status.Errorf(codes.Internal, "Failed to get information about the snapshot: %v", err)
}
Expand Down Expand Up @@ -571,7 +570,7 @@ func (s *OsdCsiServer) DeleteSnapshot(
}

// If the snapshot is not found, then we can return OK
volumes, err := s.driver.Inspect([]string{req.GetSnapshotId()})
volumes, err := s.driver.Inspect(ctx, []string{req.GetSnapshotId()})
if (err == nil && len(volumes) == 0) ||
(err != nil && err == kvdb.ErrNotFound) {
return &csi.DeleteSnapshotResponse{}, nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/sanity/snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()))

Expand Down Expand Up @@ -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()))

Expand Down Expand Up @@ -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()))

Expand Down
4 changes: 2 additions & 2 deletions volume/drivers/btrfs/btrfs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,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
}
Expand All @@ -159,7 +159,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
Expand Down
2 changes: 1 addition & 1 deletion volume/drivers/buse/buse.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,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)
Expand Down
12 changes: 1 addition & 11 deletions volume/drivers/fake/fake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,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")
Expand Down Expand Up @@ -406,16 +406,6 @@ func (d *driver) UsedSize(volumeID string) (uint64, error) {
return uint64(12345), nil
}

func (d *driver) VolumeBytesUsedByNode(nodeMID string, volumes []uint64) (*api.VolumeBytesUsedByNode, error) {
volusage := []*api.VolumeBytesUsed{}
for _, id := range volumes {
volusage = append(volusage, &api.VolumeBytesUsed{VolumeId: strconv.FormatUint(id, 10), TotalBytes: 12345})
}
return &api.VolumeBytesUsedByNode{
NodeId: nodeMID,
VolUsage: volusage,
}, nil
}
func (d *driver) Stats(ctx context.Context, volumeID string, cumulative bool) (*api.Stats, error) {
vols, err := d.Inspect(correlation.TODO(), []string{volumeID})
if err == kvdb.ErrNotFound {
Expand Down
Loading

0 comments on commit 923878b

Please sign in to comment.