From 4a46ffd2b7b868dde622f3714863e7ca96e9c2ee Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Wed, 7 Aug 2024 09:54:25 +0200 Subject: [PATCH 1/6] ci: detect `[skip ci]` in PR descriptions and prevent breakage When Jenkins jobs are started by a comment, and if the PR contains `[skip ci]` in the description, Jenkins does not run the requested job, nor set a status for the job in the PR. This causes Mergify to add the `ok-to-test` label again, instructing a GitHub Action to add comments to start jobs in Jenkins. Once all comments have been posted, the `ok-to-test` label is removed. Mergify then notices that the jobs were not run, and adds the `ok-to-test` label again.... Endlessly looping of adding the label, commenting and removing the label as a result. By reporting the brokenness of the PR description and marking the PR as Draft, the looping is prevented. Signed-off-by: Niels de Vos --- .mergify.yml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.mergify.yml b/.mergify.yml index 74c1d49b096..02625e7d2cc 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -464,3 +464,17 @@ pull_request_rules: - Repo activity - ci/skip/e2e - ci/skip/multi-arch-build + + # A [skip ci] in the PR description prevents Jenkins jobs from running, mark + # the PR as Draft so that CI jobs do not automatically run anymore. + - name: detect [skip ci] in the PR description + conditions: + - "body-raw~=[skip ci]" + actions: + edit: + draft: true + comment: + # yamllint disable-line rule:truthy + message: "The PR description contains the unsupported `[skip ci]` + command, please update the description and mark the PR ready for review + again." From d06497cf329db11309f966821b8727944b452d28 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Wed, 7 Aug 2024 13:59:16 +0200 Subject: [PATCH 2/6] doc: add description of Mergify commands for merging a PR Signed-off-by: Niels de Vos --- docs/development-guide.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/development-guide.md b/docs/development-guide.md index e9eca403c5a..2d67eaf37a3 100644 --- a/docs/development-guide.md +++ b/docs/development-guide.md @@ -278,8 +278,11 @@ need to be met before it will be merged: on the PR. The bot will merge the PR if it's having one approval and the label `ready-to-merge`. -When the criteria are met, a project maintainer can merge your changes into -the project's devel branch. +When the criteria are met, a project maintainer can instruct the Mergify bot to +queue the PR for merging. This usually is done by leaving two comments: + +* `@mergifyio rebase` to rebase on the latest HEAD of the branch +* `@mergifyio queue` once the rebasing is done, to add the PR to the merge queue ### Backport a Fix to a Release Branch From 2fd92573f4519cb443c4f66442a49dbe9840e213 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Fri, 9 Aug 2024 12:22:34 +0200 Subject: [PATCH 3/6] ci: prevent incorrect pattern matching for `skipping ci` command Mergify matches `body-raw` as regular expression, but that makes it difficult (if not impossible) to match the `[` in a string. Fixes: #4751 Signed-off-by: Niels de Vos --- .mergify.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.mergify.yml b/.mergify.yml index 02625e7d2cc..dd9fc38db5c 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -469,7 +469,8 @@ pull_request_rules: # the PR as Draft so that CI jobs do not automatically run anymore. - name: detect [skip ci] in the PR description conditions: - - "body-raw~=[skip ci]" + # this should be \[skip\W+ci\], but Mergify does not accept that + - "body-raw~=skip ci" actions: edit: draft: true From 6d1ab1b8d92e400814bd6a822212a02a8feeabe9 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Mon, 5 Aug 2024 18:27:15 +0200 Subject: [PATCH 4/6] rbd: have GetCreationTime() return a time.Time struct Do not use protobuf types when there is no need. Just use the standard time.Time format instead. Signed-off-by: Niels de Vos --- internal/csi-addons/rbd/replication.go | 12 ++++++------ internal/csi-addons/rbd/replication_test.go | 10 +++++----- internal/rbd/controllerserver.go | 5 +++-- internal/rbd/rbd_util.go | 9 ++++----- internal/rbd/snapshot.go | 13 +++++++++++++ internal/rbd/types/volume.go | 5 +++-- 6 files changed, 34 insertions(+), 20 deletions(-) diff --git a/internal/csi-addons/rbd/replication.go b/internal/csi-addons/rbd/replication.go index f58750892f1..f87a2440161 100644 --- a/internal/csi-addons/rbd/replication.go +++ b/internal/csi-addons/rbd/replication.go @@ -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()) @@ -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()) } @@ -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) @@ -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()) } // timestampFromString parses the timestamp string and returns the time.Time diff --git a/internal/csi-addons/rbd/replication_test.go b/internal/csi-addons/rbd/replication_test.go index b4e81bf9519..ba7212483eb 100644 --- a/internal/csi-addons/rbd/replication_test.go +++ b/internal/csi-addons/rbd/replication_test.go @@ -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 @@ -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, }, { @@ -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) } }) } diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index 56de60c58d0..b020bdef018 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -32,6 +32,7 @@ import ( "github.com/kubernetes-csi/csi-lib-utils/protosanitizer" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/timestamppb" ) const ( @@ -1240,7 +1241,7 @@ func (cs *ControllerServer) CreateSnapshot( SizeBytes: vol.VolSize, SnapshotId: vol.VolID, SourceVolumeId: req.GetSourceVolumeId(), - CreationTime: vol.CreatedAt, + CreationTime: timestamppb.New(*vol.CreatedAt), ReadyToUse: true, }, }, nil @@ -1300,7 +1301,7 @@ func cloneFromSnapshot( SizeBytes: rbdSnap.VolSize, SnapshotId: rbdSnap.VolID, SourceVolumeId: rbdSnap.SourceVolumeID, - CreationTime: rbdSnap.CreatedAt, + CreationTime: timestamppb.New(*rbdSnap.CreatedAt), ReadyToUse: true, }, }, nil diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index 6f9120719e9..a69b0e63279 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -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" @@ -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 @@ -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 } @@ -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 } diff --git a/internal/rbd/snapshot.go b/internal/rbd/snapshot.go index fc3fa242783..15f1db5cb97 100644 --- a/internal/rbd/snapshot.go +++ b/internal/rbd/snapshot.go @@ -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" ) @@ -118,6 +121,16 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume { } } +func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) { + 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, diff --git a/internal/rbd/types/volume.go b/internal/rbd/types/volume.go index 388676cf566..fb2b6627546 100644 --- a/internal/rbd/types/volume.go +++ b/internal/rbd/types/volume.go @@ -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 @@ -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. From 503d10eb1a90ec5f96156fa9d9e4401511d58ba4 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Tue, 6 Aug 2024 18:02:34 +0200 Subject: [PATCH 5/6] journal: store CreationTime for VolumeGroups Signed-off-by: Niels de Vos --- internal/journal/volumegroupjournal.go | 29 +++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/internal/journal/volumegroupjournal.go b/internal/journal/volumegroupjournal.go index 6d36c9d9af2..8f81c0b4b07 100644 --- a/internal/journal/volumegroupjournal.go +++ b/internal/journal/volumegroupjournal.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "time" "github.com/ceph/ceph-csi/internal/util" "github.com/ceph/ceph-csi/internal/util/log" @@ -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 { @@ -97,6 +103,7 @@ func NewCSIVolumeGroupJournal(suffix string) VolumeGroupJournalConfig { csiNameKey: "csi.volname", namespace: "", }, + csiCreationTimeKey: "csi.creationtime", } } @@ -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 } @@ -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 @@ -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( @@ -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 From 869aaced7d76a110421314e2b09a9e0d9f49223e Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 8 Aug 2024 11:03:29 +0200 Subject: [PATCH 6/6] rbd: convert rbdVolume to rbdSnapshot After cloning the RBD snapshot, an rbdVolume is returned for the CSI.Snapshot object. In order to use the rbdSnapshot.ToCSI() function, the rbdVolume needs to be converted (back) to an rbdSnaphot. Signed-off-by: Niels de Vos --- internal/rbd/controllerserver.go | 27 ++++++++++++--------------- internal/rbd/snapshot.go | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go index b020bdef018..f0ac0a47956 100644 --- a/internal/rbd/controllerserver.go +++ b/internal/rbd/controllerserver.go @@ -32,7 +32,6 @@ import ( "github.com/kubernetes-csi/csi-lib-utils/protosanitizer" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "google.golang.org/protobuf/types/known/timestamppb" ) const ( @@ -1236,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: timestamppb.New(*vol.CreatedAt), - ReadyToUse: true, - }, + Snapshot: csiSnap, }, nil } @@ -1296,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: timestamppb.New(*rbdSnap.CreatedAt), - ReadyToUse: true, - }, + Snapshot: csiSnap, }, nil } diff --git a/internal/rbd/snapshot.go b/internal/rbd/snapshot.go index 15f1db5cb97..fd7badb41e6 100644 --- a/internal/rbd/snapshot.go +++ b/internal/rbd/snapshot.go @@ -101,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{ @@ -112,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.