From fe65d80501013f2e98066b9402da0054ee0bd4c2 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 23 Mar 2022 10:39:28 -0400 Subject: [PATCH] CSI: fix timestamp from volume snapshot responses (#12352) 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 +- go.mod | 2 +- nomad/csi_endpoint_test.go | 3 +++ plugins/csi/client_test.go | 24 +++++++++++++++++------- plugins/csi/plugin.go | 2 +- 6 files changed, 26 insertions(+), 10 deletions(-) create mode 100644 .changelog/12352.txt diff --git a/.changelog/12352.txt b/.changelog/12352.txt new file mode 100644 index 000000000..688a1a9ba --- /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 814081aaa..a27e71bfa 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*1e9), // seconds to nanoseconds v.IsReady, )) } diff --git a/go.mod b/go.mod index 3e522bd89..0218687a4 100644 --- a/go.mod +++ b/go.mod @@ -122,6 +122,7 @@ require ( golang.org/x/sys v0.0.0-20220114195835-da31bd327af9 golang.org/x/time v0.0.0-20200630173020-3af7569d3a1e google.golang.org/grpc v1.44.0 + google.golang.org/protobuf v1.27.1 gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 gopkg.in/tomb.v2 v2.0.0-20140626144623-14b3d72120e8 oss.indeed.com/go/libtime v1.5.0 @@ -265,7 +266,6 @@ require ( google.golang.org/api v0.60.0 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto v0.0.0-20211104193956-4c6863e31247 // indirect - google.golang.org/protobuf v1.27.1 // indirect gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect gopkg.in/fsnotify.v1 v1.4.7 // indirect gopkg.in/resty.v1 v1.12.0 // indirect diff --git a/nomad/csi_endpoint_test.go b/nomad/csi_endpoint_test.go index 6e91c4e78..78e806a99 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,14 @@ func TestCSIVolumeEndpoint_CreateSnapshot(t *testing.T) { testutil.WaitForLeader(t, srv.RPC) + now := time.Now().Unix() fake := newMockClientCSI() fake.NextCreateSnapshotError = nil fake.NextCreateSnapshotResponse = &cstructs.ClientCSIControllerCreateSnapshotResponse{ ID: "snap-12345", ExternalSourceVolumeID: "vol-12345", SizeBytes: 42, + CreateTime: now, IsReady: true, } diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 35abf626a..3d67381a8 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 c03390688..3ceb2621c 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(), }, })