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.

Sign up for GitHub

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

Jump to bottom

Fix sizeless clones for sourceRef data volumes #2759

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Fix sizeless clones for sourceRef data volumes #2759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/controller-base.go
View file
Open in desktop
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
Open in desktop
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't do regular size-detection when cloning from a snapshot, are we sure this works? For example, what would happen if the target DV size is empty and the snapshot restoreSize is zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

covered by

containerized-data-importer/pkg/controller/common/util.go

Lines 790 to 792 in 5f85c42

} else if isSizelessClone && !restoreSizeAvailable {
return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore")
}

if isClone {
return &requestedSize, nil
}
Expand Down
41 changes: 33 additions & 8 deletions tests/cloner_test.go
View file
Open in desktop
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 @@ -2904,7 +2921,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 @@ -2918,8 +2935,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 @@ -2933,7 +2955,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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,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 @@ -194,13 +203,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