Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

journal: store CreationTime for VolumeGroups #4753

Merged
merged 3 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions internal/csi-addons/rbd/replication.go
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ func (rs *ReplicationServer) DemoteVolume(ctx context.Context,
return nil, status.Error(codes.Internal, err.Error())
}

creationTime, err := rbdVol.GetCreationTime()
creationTime, err := rbdVol.GetCreationTime(ctx)
if err != nil {
log.ErrorLog(ctx, err.Error())

Expand Down Expand Up @@ -693,7 +693,7 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
ready = checkRemoteSiteStatus(ctx, sts.GetAllSitesStatus())
}

creationTime, err := rbdVol.GetCreationTime()
creationTime, err := rbdVol.GetCreationTime(ctx)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to get image info for %s: %s", rbdVol, err.Error())
}
Expand All @@ -717,8 +717,8 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
if sErr != nil {
return nil, status.Errorf(codes.Internal, "failed to parse image creation time: %s", sErr.Error())
}
log.DebugLog(ctx, "image %s, savedImageTime=%v, currentImageTime=%v", rbdVol, st, creationTime.AsTime())
if req.GetForce() && st.Equal(creationTime.AsTime()) {
log.DebugLog(ctx, "image %s, savedImageTime=%v, currentImageTime=%v", rbdVol, st, creationTime)
if req.GetForce() && st.Equal(*creationTime) {
err = mirror.Resync(ctx)
if err != nil {
return nil, getGRPCError(err)
Expand Down Expand Up @@ -746,8 +746,8 @@ func (rs *ReplicationServer) ResyncVolume(ctx context.Context,
}

// timestampToString converts the time.Time object to string.
func timestampToString(st *timestamppb.Timestamp) string {
return fmt.Sprintf("seconds:%d nanos:%d", st.GetSeconds(), st.GetNanos())
func timestampToString(st *time.Time) string {
return fmt.Sprintf("seconds:%d nanos:%d", st.Unix(), st.Nanosecond())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we check on this one, Does Unix is equivalent to GetSeconds()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// timestampFromString parses the timestamp string and returns the time.Time
Expand Down
10 changes: 5 additions & 5 deletions internal/csi-addons/rbd/replication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ func TestGetGRPCError(t *testing.T) {
}

func Test_timestampFromString(t *testing.T) {
tm := timestamppb.Now()
tm := time.Now()
t.Parallel()
tests := []struct {
name string
Expand All @@ -627,8 +627,8 @@ func Test_timestampFromString(t *testing.T) {
}{
{
name: "valid timestamp",
timestamp: timestampToString(tm),
want: tm.AsTime().Local(),
timestamp: timestampToString(&tm),
want: tm,
wantErr: false,
},
{
Expand Down Expand Up @@ -669,8 +669,8 @@ func Test_timestampFromString(t *testing.T) {
if (err != nil) != tt.wantErr {
t.Errorf("timestampFromString() error = %v, wantErr %v", err, tt.wantErr)
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("timestampFromString() = %v, want %v", got, tt.want)
if !tt.want.Equal(got) {
t.Errorf("timestampFromString() = %q, want %q", got, tt.want)
}
})
}
Expand Down
29 changes: 26 additions & 3 deletions internal/journal/volumegroupjournal.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"time"

"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/log"
Expand Down Expand Up @@ -75,6 +76,11 @@ type VolumeGroupJournal interface {
// VolumeGroupJournalConfig contains the configuration.
type VolumeGroupJournalConfig struct {
Config

// csiCreationTimeKey can hold the key for the time a group was
// created. At least RBD groups do not provide the creation time
// through API calls.
csiCreationTimeKey string
}

type volumeGroupJournalConnection struct {
Expand All @@ -97,6 +103,7 @@ func NewCSIVolumeGroupJournal(suffix string) VolumeGroupJournalConfig {
csiNameKey: "csi.volname",
namespace: "",
},
csiCreationTimeKey: "csi.creationtime",
}
}

Expand Down Expand Up @@ -229,6 +236,7 @@ func (vgjc *volumeGroupJournalConnection) CheckReservation(ctx context.Context,
volGroupData.VolumeGroupAttributes = &VolumeGroupAttributes{}
volGroupData.VolumeGroupAttributes.RequestName = savedVolumeGroupAttributes.RequestName
volGroupData.VolumeGroupAttributes.VolumeMap = savedVolumeGroupAttributes.VolumeMap
volGroupData.VolumeGroupAttributes.CreationTime = savedVolumeGroupAttributes.CreationTime

return volGroupData, nil
}
Expand Down Expand Up @@ -354,6 +362,12 @@ func (vgjc *volumeGroupJournalConnection) ReserveName(ctx context.Context,
omapValues[cj.csiNameKey] = reqName
omapValues[cj.csiImageKey] = groupName

t, err := time.Now().MarshalText()
if err != nil {
return "", "", err
}
omapValues[cj.csiCreationTimeKey] = string(t)

err = setOMapKeys(ctx, vgjc.connection, journalPool, cj.namespace, oid, omapValues)
if err != nil {
return "", "", err
Expand All @@ -365,9 +379,10 @@ func (vgjc *volumeGroupJournalConnection) ReserveName(ctx context.Context,
// VolumeGroupAttributes contains the request name and the volumeID's and
// the corresponding snapshotID's.
type VolumeGroupAttributes struct {
RequestName string // Contains the request name for the passed in UUID
GroupName string // Contains the group name
VolumeMap map[string]string // Contains the volumeID and the corresponding value mapping
RequestName string // Contains the request name for the passed in UUID
GroupName string // Contains the group name
CreationTime *time.Time // Contains the time of creation of the group
VolumeMap map[string]string // Contains the volumeID and the corresponding value mapping
}

func (vgjc *volumeGroupJournalConnection) GetVolumeGroupAttributes(
Expand All @@ -390,13 +405,21 @@ func (vgjc *volumeGroupJournalConnection) GetVolumeGroupAttributes(
log.WarningLog(ctx, "unable to read omap values: pool missing: %v", err)
}

t := &time.Time{}
err = t.UnmarshalText([]byte(values[cj.csiCreationTimeKey]))
if err != nil {
t = nil
}

groupAttributes.RequestName = values[cj.csiNameKey]
groupAttributes.GroupName = values[cj.csiImageKey]
groupAttributes.CreationTime = t

// Remove request name key and group name key from the omap, as we are
// looking for volumeID/snapshotID mapping
delete(values, cj.csiNameKey)
delete(values, cj.csiImageKey)
delete(values, cj.csiCreationTimeKey)
groupAttributes.VolumeMap = map[string]string{}
for k, v := range values {
groupAttributes.VolumeMap[k] = v
Expand Down
26 changes: 12 additions & 14 deletions internal/rbd/controllerserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1235,14 +1235,13 @@ func (cs *ControllerServer) CreateSnapshot(
return nil, status.Error(codes.Internal, err.Error())
}

csiSnap, err := vol.toSnapshot().ToCSI(ctx)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}

return &csi.CreateSnapshotResponse{
Snapshot: &csi.Snapshot{
SizeBytes: vol.VolSize,
SnapshotId: vol.VolID,
SourceVolumeId: req.GetSourceVolumeId(),
CreationTime: vol.CreatedAt,
ReadyToUse: true,
},
Snapshot: csiSnap,
}, nil
}

Expand Down Expand Up @@ -1295,14 +1294,13 @@ func cloneFromSnapshot(
}
}

csiSnap, err := rbdSnap.ToCSI(ctx)
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
}

return &csi.CreateSnapshotResponse{
Snapshot: &csi.Snapshot{
SizeBytes: rbdSnap.VolSize,
SnapshotId: rbdSnap.VolID,
SourceVolumeId: rbdSnap.SourceVolumeID,
CreationTime: rbdSnap.CreatedAt,
ReadyToUse: true,
},
Snapshot: csiSnap,
}, nil
}

Expand Down
9 changes: 4 additions & 5 deletions internal/rbd/rbd_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ import (
librbd "github.com/ceph/go-ceph/rbd"
"github.com/ceph/go-ceph/rbd/admin"
"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/golang/protobuf/ptypes/timestamp"
"google.golang.org/protobuf/types/known/timestamppb"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/cloud-provider/volume/helpers"
mount "k8s.io/mount-utils"
Expand Down Expand Up @@ -138,7 +136,7 @@ type rbdImage struct {
// fileEncryption provides access to optional VolumeEncryption functions (e.g fscrypt)
fileEncryption *util.VolumeEncryption

CreatedAt *timestamp.Timestamp
CreatedAt *time.Time

// conn is a connection to the Ceph cluster obtained from a ConnPool
conn *util.ClusterConnection
Expand Down Expand Up @@ -1595,7 +1593,7 @@ func (rv *rbdVolume) setImageOptions(ctx context.Context, options *librbd.ImageO

// GetCreationTime returns the creation time of the image. if the image
// creation time is not set, it queries the image info and returns the creation time.
func (ri *rbdImage) GetCreationTime() (*timestamppb.Timestamp, error) {
func (ri *rbdImage) GetCreationTime(ctx context.Context) (*time.Time, error) {
if ri.CreatedAt != nil {
return ri.CreatedAt, nil
}
Expand Down Expand Up @@ -1648,8 +1646,9 @@ func (ri *rbdImage) getImageInfo() error {
if err != nil {
return err
}

t := time.Unix(tm.Sec, tm.Nsec)
ri.CreatedAt = timestamppb.New(t)
ri.CreatedAt = &t

return nil
}
Expand Down
35 changes: 35 additions & 0 deletions internal/rbd/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import (
"errors"
"fmt"

"github.com/container-storage-interface/spec/lib/go/csi"
"google.golang.org/protobuf/types/known/timestamppb"

"github.com/ceph/ceph-csi/internal/util"
"github.com/ceph/ceph-csi/internal/util/log"
)
Expand Down Expand Up @@ -98,6 +101,27 @@ func cleanUpSnapshot(
return nil
}

func (rv *rbdVolume) toSnapshot() *rbdSnapshot {
return &rbdSnapshot{
rbdImage: rbdImage{
ClusterID: rv.ClusterID,
VolID: rv.VolID,
Monitors: rv.Monitors,
Pool: rv.Pool,
JournalPool: rv.JournalPool,
RadosNamespace: rv.RadosNamespace,
RbdImageName: rv.RbdImageName,
ImageID: rv.ImageID,
CreatedAt: rv.CreatedAt,
// copyEncryptionConfig cannot be used here because the volume and the
// snapshot will have the same volumeID which cases the panic in
// copyEncryptionConfig function.
blockEncryption: rv.blockEncryption,
fileEncryption: rv.fileEncryption,
},
}
}

func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume {
return &rbdVolume{
rbdImage: rbdImage{
Expand All @@ -109,6 +133,7 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume {
RadosNamespace: rbdSnap.RadosNamespace,
RbdImageName: rbdSnap.RbdSnapName,
ImageID: rbdSnap.ImageID,
CreatedAt: rbdSnap.CreatedAt,
// copyEncryptionConfig cannot be used here because the volume and the
// snapshot will have the same volumeID which cases the panic in
// copyEncryptionConfig function.
Expand All @@ -118,6 +143,16 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume {
}
}

func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make use of it here ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, sure, this function actually was not required to be included, it is part of my VolumeGroupSnapshot work. I'll include it here so that it all gets a little cleaner.

return &csi.Snapshot{
SizeBytes: rbdSnap.VolSize,
SnapshotId: rbdSnap.VolID,
SourceVolumeId: rbdSnap.SourceVolumeID,
CreationTime: timestamppb.New(*rbdSnap.CreatedAt),
ReadyToUse: true,
}, nil
}

func undoSnapshotCloning(
ctx context.Context,
parentVol *rbdVolume,
Expand Down
5 changes: 3 additions & 2 deletions internal/rbd/types/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ package types

import (
"context"
"time"

"github.com/container-storage-interface/spec/lib/go/csi"
"google.golang.org/protobuf/types/known/timestamppb"
)

//nolint:interfacebloat // more than 10 methods are needed for the interface
Expand All @@ -42,7 +42,8 @@ type Volume interface {
RemoveFromGroup(ctx context.Context, vg VolumeGroup) error

// GetCreationTime returns the creation time of the volume.
GetCreationTime() (*timestamppb.Timestamp, error)
GetCreationTime(ctx context.Context) (*time.Time, error)

// GetMetadata returns the value of the metadata key from the volume.
GetMetadata(key string) (string, error)
// SetMetadata sets the value of the metadata key on the volume.
Expand Down