From 5e74bff245efe69e09dc003e7fab33636ab07398 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 22 Mar 2022 16:58:55 -0400 Subject: [PATCH] csi: fix timestamp from volume snapshot responses 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. --- .changelog/12352.txt | 3 +++ command/volume_snapshot_list.go | 2 +- nomad/csi_endpoint_test.go | 4 ++++ plugins/csi/client_test.go | 24 +++++++++++++++++------- plugins/csi/plugin.go | 2 +- 5 files changed, 26 insertions(+), 9 deletions(-) create mode 100644 .changelog/12352.txt diff --git a/.changelog/12352.txt b/.changelog/12352.txt new file mode 100644 index 000000000000..688a1a9ba544 --- /dev/null +++ b/.changelog/12352.txt @@ -0,0 +1,3 @@ +```release-note:bug +csi: Fixed a bug where volume snapshot timestamps were always zero values +``` diff --git a/command/volume_snapshot_list.go b/command/volume_snapshot_list.go index 814081aaab32..1da460b7d6f8 100644 --- a/command/volume_snapshot_list.go +++ b/command/volume_snapshot_list.go @@ -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, )) } diff --git a/nomad/csi_endpoint_test.go b/nomad/csi_endpoint_test.go index 6e91c4e7873e..1ed2e8f6172b 100644 --- a/nomad/csi_endpoint_test.go +++ b/nomad/csi_endpoint_test.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" "testing" + "time" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/hashicorp/nomad/acl" @@ -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, } diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 35abf626a822..3d67381a8dca 100644 --- a/plugins/csi/client_test.go +++ b/plugins/csi/client_test.go @@ -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" @@ -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) { @@ -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 @@ -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, }, }, @@ -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()) } }) } @@ -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{ { @@ -1138,6 +1144,7 @@ func TestClient_RPC_ControllerListSnapshots(t *testing.T) { SnapshotId: "snap-12345", SourceVolumeId: "vol-12345", ReadyToUse: true, + CreationTime: timestamppb.New(now), }, }, }, @@ -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()) }) } } @@ -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 diff --git a/plugins/csi/plugin.go b/plugins/csi/plugin.go index c03390688b69..3ceb2621c57a 100644 --- a/plugins/csi/plugin.go +++ b/plugins/csi/plugin.go @@ -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(), }, })