Skip to content

Commit

Permalink
rbd: add validation to ToCSI() for rbdVolume and rbdSnapshot
Browse files Browse the repository at this point in the history
After an unfortnate timed restart of the provisioner, a volume that got
cloned did not get a `rbdVolume.VolID` set. The `.VolID` is used as the
CSI Volume Handle, and is a required attribute.

The `rbdVolume` and `rbdSnapshot` structs have a `.ToCSI()` function
that can do the validation of required attributes. This is now added,
including unit-tests.

Signed-off-by: Niels de Vos <ndevos@ibm.com>
  • Loading branch information
nixpanic committed Feb 17, 2025
1 parent e4d41c4 commit 5cd00c2
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 43 deletions.
11 changes: 11 additions & 0 deletions internal/rbd/controllerserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,17 @@ func (cs *ControllerServer) parseVolCreateRequest(
}

func (rbdVol *rbdVolume) ToCSI(ctx context.Context) (*csi.Volume, error) {
switch {
case rbdVol.VolID == "":
return nil, fmt.Errorf("%q does not have a volume-id set", rbdVol)
case rbdVol.Pool == "":
return nil, fmt.Errorf("%q does not have a pool set", rbdVol)
case rbdVol.JournalPool == "":
return nil, fmt.Errorf("%q does not have a journal-pool set", rbdVol)
case rbdVol.RbdImageName == "":
return nil, fmt.Errorf("%q does not have an image-name set", rbdVol)
}

vol := &csi.Volume{
VolumeId: rbdVol.VolID,
CapacityBytes: rbdVol.VolSize,
Expand Down
85 changes: 42 additions & 43 deletions internal/rbd/controllerserver_test.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2022 The Ceph-CSI Authors.
Copyright 2025 The Ceph-CSI Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -16,71 +16,70 @@ limitations under the License.

package rbd

import "testing"
import (
"context"
"testing"
"time"
)

func TestValidateStriping(t *testing.T) {
func TestToCSISnapshot(t *testing.T) {
t.Parallel()
now := time.Now()
tests := []struct {
name string
parameters map[string]string
wantErr bool
name string
rs *rbdSnapshot
wantErr bool
}{
{
name: "when stripeUnit is not specified",
parameters: map[string]string{
"stripeUnit": "",
"stripeCount": "10",
"objectSize": "2",
name: "all attributes set",
rs: &rbdSnapshot{
rbdImage: rbdImage{
VolID: "0001-unique-snapshot-id",
CreatedAt: &now,
},
SourceVolumeID: "0001-unique-volume-id",
},
wantErr: true,
wantErr: false,
},
{
name: "when stripeCount is not specified",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "",
"objectSize": "2",
name: "missing volume-id",
rs: &rbdSnapshot{
rbdImage: rbdImage{
VolID: "",
CreatedAt: &now,
},
SourceVolumeID: "0001-unique-volume-id",
},
wantErr: true,
},
{
name: "when objectSize is not power of 2",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "3",
name: "missing source-volume-id",
rs: &rbdSnapshot{
rbdImage: rbdImage{
VolID: "0001-unique-snapshot-id",
CreatedAt: &now,
},
SourceVolumeID: "",
},
wantErr: true,
},
{
name: "when objectSize is 0",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "0",
name: "missing creation-time",
rs: &rbdSnapshot{
rbdImage: rbdImage{
VolID: "0001-unique-snapshot-id",
CreatedAt: nil,
},
SourceVolumeID: "0001-unique-volume-id",
},
wantErr: true,
},
{
name: "when valid stripe parameters are specified",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "131072",
},
wantErr: false,
},
{
name: "when no stripe parameters are specified",
parameters: map[string]string{},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if err := validateStriping(tt.parameters); (err != nil) != tt.wantErr {
t.Errorf("validateStriping() error = %v, wantErr %v", err, tt.wantErr)
if _, err := tt.rs.ToCSI(context.TODO()); (err != nil) != tt.wantErr {
t.Errorf("ToCSI() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
Expand Down
7 changes: 7 additions & 0 deletions internal/rbd/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@ func (rbdSnap *rbdSnapshot) toVolume() *rbdVolume {
}

func (rbdSnap *rbdSnapshot) ToCSI(ctx context.Context) (*csi.Snapshot, error) {
switch {
case rbdSnap.VolID == "":
return nil, fmt.Errorf("%q does not have a volume-id set", rbdSnap)
case rbdSnap.SourceVolumeID == "":
return nil, fmt.Errorf("%q does not have a source-volume-id set", rbdSnap)
}

created, err := rbdSnap.GetCreationTime(ctx)
if err != nil {
return nil, err
Expand Down
168 changes: 168 additions & 0 deletions internal/rbd/snapshot_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/*
Copyright 2022 The Ceph-CSI Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package rbd

import (
"context"
"testing"
)

func TestValidateStriping(t *testing.T) {
t.Parallel()
tests := []struct {
name string
parameters map[string]string
wantErr bool
}{
{
name: "when stripeUnit is not specified",
parameters: map[string]string{
"stripeUnit": "",
"stripeCount": "10",
"objectSize": "2",
},
wantErr: true,
},
{
name: "when stripeCount is not specified",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "",
"objectSize": "2",
},
wantErr: true,
},
{
name: "when objectSize is not power of 2",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "3",
},
wantErr: true,
},
{
name: "when objectSize is 0",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "0",
},
wantErr: true,
},
{
name: "when valid stripe parameters are specified",
parameters: map[string]string{
"stripeUnit": "4096",
"stripeCount": "8",
"objectSize": "131072",
},
wantErr: false,
},
{
name: "when no stripe parameters are specified",
parameters: map[string]string{},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if err := validateStriping(tt.parameters); (err != nil) != tt.wantErr {
t.Errorf("validateStriping() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}

func TestToCSIVolume(t *testing.T) {
t.Parallel()
tests := []struct {
name string
rv *rbdVolume
wantErr bool
}{
{
name: "all attributes set",
rv: &rbdVolume{
rbdImage: rbdImage{
VolID: "0001-unique-volume-id",
Pool: "ecpool",
JournalPool: "replicapool",
RbdImageName: "csi-vol-01234-5678-90abc",
},
},
wantErr: false,
},
{
name: "missing volume-id",
rv: &rbdVolume{
rbdImage: rbdImage{
VolID: "",
Pool: "ecpool",
JournalPool: "replicapool",
RbdImageName: "csi-vol-01234-5678-90abc",
},
},
wantErr: true,
},
{
name: "missing pool",
rv: &rbdVolume{
rbdImage: rbdImage{
VolID: "0001-unique-volume-id",
Pool: "",
JournalPool: "replicapool",
RbdImageName: "csi-vol-01234-5678-90abc",
},
},
wantErr: true,
},
{
name: "missing journal-pool",
rv: &rbdVolume{
rbdImage: rbdImage{
VolID: "0001-unique-volume-id",
Pool: "ecpool",
JournalPool: "",
RbdImageName: "csi-vol-01234-5678-90abc",
},
},
wantErr: true,
},
{
name: "missing image-name",
rv: &rbdVolume{
rbdImage: rbdImage{
VolID: "0001-unique-volume-id",
Pool: "ecpool",
JournalPool: "",
RbdImageName: "",
},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if _, err := tt.rv.ToCSI(context.TODO()); (err != nil) != tt.wantErr {
t.Errorf("ToCSI() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}

0 comments on commit 5cd00c2

Please sign in to comment.