Skip to content

Commit

Permalink
Fix sizeless clones for sourceRef data volumes (#2759)
Browse files Browse the repository at this point in the history 
We were hitting a panic because we're passing the original DV object to renderPvcSpec (source is nil)
instead of the mutated DV which has sourceRef converted (source not nil)

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
  • Loading branch information
@akalenyu
akalenyu committed Jun 21, 2023
1 parent 0bc6a8a commit 9ea73dc
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 19 deletions.
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/controller-base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request,
}
}

syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, dv, syncState.pvc)
syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, syncState.dvMutated, syncState.pvc)
if err != nil {
if syncErr := r.syncDataVolumeStatusPhaseWithEvent(&syncState, cdiv1.PhaseUnset, nil,
Event{corev1.EventTypeWarning, cc.ErrClaimNotValid, err.Error()}); syncErr != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func resolveVolumeSize(c client.Client, dvSpec cdiv1.DataVolumeSpec, pvcSpec *v1

if !found {
// Storage size can be empty when cloning
isClone := dvSpec.Source.PVC != nil
isClone := dvSpec.Source.PVC != nil || dvSpec.Source.Snapshot != nil
if isClone {
return &requestedSize, nil
}
Expand Down
41 changes: 33 additions & 8 deletions tests/cloner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ var _ = Describe("all clone tests", func() {
})

DescribeTable("Should clone with empty volume size without using size-detection pod",
func(sourceVolumeMode, targetVolumeMode v1.PersistentVolumeMode) {
func(sourceVolumeMode, targetVolumeMode v1.PersistentVolumeMode, sourceRef bool) {
// When cloning without defining the target's storage size, the source's size can be attainable
// by different means depending on the clone type and the volume mode used.
// Either if "block" is used as volume mode or smart/csi cloning is used as clone strategy,
Expand Down Expand Up @@ -772,6 +772,11 @@ var _ = Describe("all clone tests", func() {
targetDiskImagePath = testBaseDir
}

if cloneType == "snapshot" && sourceRef {
// TODO: remove this when we no longer have smart clone controller
Skip("Smart clone controller doesn't play nice with sourceRef and is being removed soon")
}

// Create the source DV
dataVolume := utils.NewDataVolumeWithHTTPImportAndStorageSpec(dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs))
dataVolume.Spec.Storage.VolumeMode = &sourceVolumeMode
Expand All @@ -788,9 +793,20 @@ var _ = Describe("all clone tests", func() {
By("Wait for source DV Succeeded phase")
err = utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, dataVolumeName, cloneCompleteTimeout)
Expect(err).ToNot(HaveOccurred())
var ds *cdiv1.DataSource
if sourceRef {
ds = utils.NewPvcDataSource("test-datasource", sourcePvc.Namespace, sourcePvc.Name, sourcePvc.Namespace)
By(fmt.Sprintf("Create new datasource %s", ds.Name))
ds, err = f.CdiClient.CdiV1beta1().DataSources(sourcePvc.Namespace).Create(context.TODO(), ds, metav1.CreateOptions{})
Expect(err).ToNot(HaveOccurred())
}

// We attempt to create the sizeless DV
targetDV := utils.NewDataVolumeForCloningWithEmptySize("target-dv", sourcePvc.Namespace, sourcePvc.Name, nil, &targetVolumeMode)
if sourceRef {
targetDV = utils.NewDataVolumeWithSourceRefAndStorageAPI("target-dv", nil, ds.Namespace, ds.Name)
targetDV.Spec.Storage.VolumeMode = &targetVolumeMode
}
if targetSCName != "" {
targetDV.Spec.Storage.StorageClassName = &targetSCName
}
Expand Down Expand Up @@ -827,9 +843,10 @@ var _ = Describe("all clone tests", func() {
By("Checksum comparison")
Expect(sourceMD5).To(Equal(targetMD5))
},
Entry("[test_id:8492]Block to block (empty storage size)", v1.PersistentVolumeBlock, v1.PersistentVolumeBlock),
Entry("[test_id:8491]Block to filesystem (empty storage size)", v1.PersistentVolumeBlock, v1.PersistentVolumeFilesystem),
Entry("[test_id:8490]Filesystem to filesystem(empty storage size)", v1.PersistentVolumeFilesystem, v1.PersistentVolumeFilesystem),
Entry("[test_id:8492]Block to block (empty storage size)", v1.PersistentVolumeBlock, v1.PersistentVolumeBlock, false),
Entry("[test_id:8491]Block to filesystem (empty storage size)", v1.PersistentVolumeBlock, v1.PersistentVolumeFilesystem, false),
Entry("[test_id:8490]Filesystem to filesystem(empty storage size)", v1.PersistentVolumeFilesystem, v1.PersistentVolumeFilesystem, false),
Entry("[test_id:8490]Filesystem to filesystem(empty storage size) with sourceRef", v1.PersistentVolumeFilesystem, v1.PersistentVolumeFilesystem, true),
)

Context("WaitForFirstConsumer with advanced cloning methods", func() {
Expand Down Expand Up @@ -2900,7 +2917,7 @@ var _ = Describe("all clone tests", func() {
})

Context("sourceRef support", func() {
It("[test_id:9758] Should clone data from SourceRef snapshot DataSource", func() {
DescribeTable("[test_id:9758] Should clone data from SourceRef snapshot DataSource", func(sizeless bool) {
if !f.IsSnapshotStorageClassAvailable() {
Skip("Clone from volumesnapshot does not work without snapshot capable storage")
}
Expand All @@ -2914,8 +2931,13 @@ var _ = Describe("all clone tests", func() {
targetDataSource, err := f.CdiClient.CdiV1beta1().DataSources(snapshot.Namespace).Create(context.TODO(), targetDS, metav1.CreateOptions{})
Expect(err).ToNot(HaveOccurred())

targetDV := utils.NewDataVolumeWithSourceRef("target-dv", size, targetDataSource.Namespace, targetDataSource.Name)
targetDV.Spec.PVC.StorageClassName = &f.SnapshotSCName
var targetSizePtr *string
if !sizeless {
targetSizePtr = &size
}
targetDV := utils.NewDataVolumeWithSourceRefAndStorageAPI("target-dv", targetSizePtr, targetDataSource.Namespace, targetDataSource.Name)
targetDV.Spec.Storage.StorageClassName = &f.SnapshotSCName
targetDV.Spec.Storage.VolumeMode = &volumeMode
By(fmt.Sprintf("Create new target datavolume %s", targetDV.Name))
targetDataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDV)
Expect(err).ToNot(HaveOccurred())
Expand All @@ -2929,7 +2951,10 @@ var _ = Describe("all clone tests", func() {
same, err := f.VerifyTargetPVCContentMD5(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDV), path, utils.UploadFileMD5, utils.UploadFileSize)
Expect(err).ToNot(HaveOccurred())
Expect(same).To(BeTrue())
})
},
Entry("with size specified on target", false),
Entry("with size omitted on target", true),
)
})
})
})
Expand Down
3 changes: 2 additions & 1 deletion tests/dataimportcron_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,8 @@ var _ = Describe("DataImportCron", func() {
}
Expect(dataSource.Spec.Source).To(Equal(expectedSource))
// Verify content
targetDV := utils.NewDataVolumeWithSourceRefAndStorageAPI("target-dv", "1Gi", dataSource.Namespace, dataSource.Name)
size := "1Gi"
targetDV := utils.NewDataVolumeWithSourceRefAndStorageAPI("target-dv", &size, dataSource.Namespace, dataSource.Name)
By(fmt.Sprintf("Create new target datavolume %s", targetDV.Name))
targetDataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, ns, targetDV)
Expect(err).ToNot(HaveOccurred())
Expand Down
19 changes: 11 additions & 8 deletions tests/utils/datavolume.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,16 @@ func NewDataVolumeWithSourceRef(dataVolumeName string, size, sourceRefNamespace,
}

// NewDataVolumeWithSourceRefAndStorageAPI initializes a DataVolume struct with DataSource SourceRef and storage defaults-inferring API
func NewDataVolumeWithSourceRefAndStorageAPI(dataVolumeName string, size, sourceRefNamespace, sourceRefName string) *cdiv1.DataVolume {
func NewDataVolumeWithSourceRefAndStorageAPI(dataVolumeName string, size *string, sourceRefNamespace, sourceRefName string) *cdiv1.DataVolume {
spec := &cdiv1.StorageSpec{}
if size != nil {
spec.Resources = k8sv1.ResourceRequirements{
Requests: k8sv1.ResourceList{
k8sv1.ResourceStorage: resource.MustParse(*size),
},
}
}

return &cdiv1.DataVolume{
ObjectMeta: metav1.ObjectMeta{
Name: dataVolumeName,
Expand All @@ -197,13 +206,7 @@ func NewDataVolumeWithSourceRefAndStorageAPI(dataVolumeName string, size, source
Namespace: &sourceRefNamespace,
Name: sourceRefName,
},
Storage: &cdiv1.StorageSpec{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceStorage: resource.MustParse(size),
},
},
},
Storage: spec,
},
}
}
Expand Down

0 comments on commit 9ea73dc

Please sign in to comment.