Skip to content

Commit

Permalink
csi: fix timestamp from volume snapshot responses
Browse files Browse the repository at this point in the history
Listing snapshots was incorrectly returning nanoseconds instead of
seconds, and formatting of timestamps both list and create snapshot
was treating the timestamp as though it were nanoseconds instead of
seconds. This resulted in create timestamps always being displayed as
zero values.

Fix the unit conversion error in the command line and the incorrect
extraction in the CSI plugin client code. Beef up the unit tests to
make sure this code is actually exercised.
  • Loading branch information
tgross committed Mar 22, 2022
1 parent 73afdea commit 5e74bff
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 9 deletions.
3 changes: 3 additions & 0 deletions .changelog/12352.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
csi: Fixed a bug where volume snapshot timestamps were always zero values
```
2 changes: 1 addition & 1 deletion command/volume_snapshot_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ func csiFormatSnapshots(snapshots []*api.CSISnapshot, verbose bool) string {
v.ID,
limit(v.ExternalSourceVolumeID, length),
humanize.IBytes(uint64(v.SizeBytes)),
formatUnixNanoTime(v.CreateTime),
formatUnixNanoTime(v.CreateTime*1000000000), // this time is in seconds
v.IsReady,
))
}
Expand Down
4 changes: 4 additions & 0 deletions nomad/csi_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"strings"
"testing"
"time"

msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/hashicorp/nomad/acl"
Expand Down Expand Up @@ -1322,12 +1323,15 @@ func TestCSIVolumeEndpoint_CreateSnapshot(t *testing.T) {

testutil.WaitForLeader(t, srv.RPC)

now := time.Now()

fake := newMockClientCSI()
fake.NextCreateSnapshotError = nil
fake.NextCreateSnapshotResponse = &cstructs.ClientCSIControllerCreateSnapshotResponse{
ID: "snap-12345",
ExternalSourceVolumeID: "vol-12345",
SizeBytes: 42,
CreateTime: 0,
IsReady: true,
}

Expand Down
24 changes: 17 additions & 7 deletions plugins/csi/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"path/filepath"
"testing"
"time"

csipbv1 "github.com/container-storage-interface/spec/lib/go/csi"
"github.com/golang/protobuf/ptypes/wrappers"
Expand All @@ -16,6 +17,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/timestamppb"
)

func newTestClient(t *testing.T) (*fake.IdentityClient, *fake.ControllerClient, *fake.NodeClient, CSIPlugin) {
Expand Down Expand Up @@ -990,6 +992,8 @@ func TestClient_RPC_ControllerListVolume(t *testing.T) {
func TestClient_RPC_ControllerCreateSnapshot(t *testing.T) {
ci.Parallel(t)

now := time.Now()

cases := []struct {
Name string
Request *ControllerCreateSnapshotRequest
Expand Down Expand Up @@ -1024,6 +1028,7 @@ func TestClient_RPC_ControllerCreateSnapshot(t *testing.T) {
SizeBytes: 100000,
SnapshotId: "snap-12345",
SourceVolumeId: "vol-12345",
CreationTime: timestamppb.New(now),
ReadyToUse: true,
},
},
Expand All @@ -1039,11 +1044,13 @@ func TestClient_RPC_ControllerCreateSnapshot(t *testing.T) {
// note: there's nothing interesting to assert about the response
// here other than that we don't throw a NPE during transformation
// from protobuf to our struct
_, err := client.ControllerCreateSnapshot(context.TODO(), tc.Request)
resp, err := client.ControllerCreateSnapshot(context.TODO(), tc.Request)
if tc.ExpectedErr != nil {
require.EqualError(t, err, tc.ExpectedErr.Error())
} else {
require.NoError(t, err, tc.Name)
require.NotZero(t, resp.Snapshot.CreateTime)
require.Equal(t, now.Second(), time.Unix(resp.Snapshot.CreateTime, 0).Second())
}
})
}
Expand Down Expand Up @@ -1120,16 +1127,15 @@ func TestClient_RPC_ControllerListSnapshots(t *testing.T) {
},
}

now := time.Now()

for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
_, cc, _, client := newTestClient(t)
defer client.Close()

cc.NextErr = tc.ResponseErr
if tc.ResponseErr != nil {
// note: there's nothing interesting to assert here other than
// that we don't throw a NPE during transformation from
// protobuf to our struct
if tc.ResponseErr == nil {
cc.NextListSnapshotsResponse = &csipbv1.ListSnapshotsResponse{
Entries: []*csipbv1.ListSnapshotsResponse_Entry{
{
Expand All @@ -1138,6 +1144,7 @@ func TestClient_RPC_ControllerListSnapshots(t *testing.T) {
SnapshotId: "snap-12345",
SourceVolumeId: "vol-12345",
ReadyToUse: true,
CreationTime: timestamppb.New(now),
},
},
},
Expand All @@ -1152,7 +1159,10 @@ func TestClient_RPC_ControllerListSnapshots(t *testing.T) {
}
require.NoError(t, err, tc.Name)
require.NotNil(t, resp)

require.Len(t, resp.Entries, 1)
require.NotZero(t, resp.Entries[0].Snapshot.CreateTime)
require.Equal(t, now.Second(),
time.Unix(resp.Entries[0].Snapshot.CreateTime, 0).Second())
})
}
}
Expand Down Expand Up @@ -1298,7 +1308,7 @@ func TestClient_RPC_NodePublishVolume(t *testing.T) {
}
func TestClient_RPC_NodeUnpublishVolume(t *testing.T) {
ci.Parallel(t)

cases := []struct {
Name string
ExternalID string
Expand Down
2 changes: 1 addition & 1 deletion plugins/csi/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ func NewListSnapshotsResponse(resp *csipbv1.ListSnapshotsResponse) *ControllerLi
SizeBytes: snap.GetSizeBytes(),
ID: snap.GetSnapshotId(),
SourceVolumeID: snap.GetSourceVolumeId(),
CreateTime: int64(snap.GetCreationTime().GetNanos()),
CreateTime: snap.GetCreationTime().GetSeconds(),
IsReady: snap.GetReadyToUse(),
},
})
Expand Down

0 comments on commit 5e74bff

Please sign in to comment.