From a82a0a7a59d29e56060f4f21ba24e3e2024a5c3a Mon Sep 17 00:00:00 2001 From: Bartosz Rybacki Date: Wed, 22 Sep 2021 18:06:18 +0200 Subject: [PATCH] Correct the cloneStrategy on StorageProfile (#1951) Moves the cloneStrategy up one level from claimPropertySet to StorageProfile.spec and StorageProfile.status. Signed-off-by: Bartosz Rybacki --- doc/storageprofile.md | 9 +++++--- pkg/apis/core/v1beta1/openapi_generated.go | 21 ++++++++++++------- pkg/apis/core/v1beta1/types.go | 6 ++++-- .../core/v1beta1/types_swagger_generated.go | 9 ++++---- .../core/v1beta1/zz_generated.deepcopy.go | 15 ++++++++----- pkg/controller/datavolume-controller.go | 6 +----- pkg/controller/storageprofile-controller.go | 2 ++ pkg/controller/util_test.go | 6 +++--- pkg/operator/resources/crds_generated.go | 12 +++++------ tests/cloner_test.go | 4 ++-- tests/utils/storageprofile.go | 12 ++--------- 11 files changed, 55 insertions(+), 47 deletions(-) diff --git a/doc/storageprofile.md b/doc/storageprofile.md index 3d381dc0e..99ddb287b 100644 --- a/doc/storageprofile.md +++ b/doc/storageprofile.md @@ -19,12 +19,13 @@ apiVersion: cdi.kubevirt.io/v1beta1 kind: StorageProfile metadata: name: hostpath-provisioner -spec: +spec: claimPropertySets: - accessModes: - ReadWriteOnce volumeMode: Filesystem + cloneStrategy: snapshot status: storageClass: hostpath-provisioner provisioner: kubevirt.io/hostpath-provisioner @@ -32,12 +33,14 @@ status: - accessModes: - ReadWriteOnce volumeMode: Filesystem + cloneStrategy: snapshot ``` Current version supports the following parameters: -- `accessMode` - contains the desired access modes the volume should have -- `volumeMode` - defines what type of volume is required by the claim - `cloneStrategy` - defines the preferred method for performing a CDI clone +- `claimPropertySets` contains a list of `claimPropertySet` (currently only one supported) + - `accessMode` - contains the desired access modes the volume should have + - `volumeMode` - defines what type of volume is required by the claim Values for accessModes and volumeMode are exactly the same as for PVC: `accessModes` is a list of `[ReadWriteMany|ReadWriteOnce|ReadOnlyMany]` and `volumeMode` is a single value `Filesystem` or `Block`. The value for `cloneStrategy` ca be one of: `copy`,`snapshot`,`csi-clone`. diff --git a/pkg/apis/core/v1beta1/openapi_generated.go b/pkg/apis/core/v1beta1/openapi_generated.go index 0db9c9ed1..a0847d2d7 100644 --- a/pkg/apis/core/v1beta1/openapi_generated.go +++ b/pkg/apis/core/v1beta1/openapi_generated.go @@ -14809,13 +14809,6 @@ func schema_pkg_apis_core_v1beta1_ClaimPropertySet(ref common.ReferenceCallback) Format: "", }, }, - "cloneStrategy": { - SchemaProps: spec.SchemaProps{ - Description: "CloneStrategy defines the preferred method for performing a CDI clone", - Type: []string{"string"}, - Format: "", - }, - }, }, }, }, @@ -16396,6 +16389,13 @@ func schema_pkg_apis_core_v1beta1_StorageProfileSpec(ref common.ReferenceCallbac Description: "StorageProfileSpec defines specification for StorageProfile", Type: []string{"object"}, Properties: map[string]spec.Schema{ + "cloneStrategy": { + SchemaProps: spec.SchemaProps{ + Description: "CloneStrategy defines the preferred method for performing a CDI clone", + Type: []string{"string"}, + Format: "", + }, + }, "claimPropertySets": { SchemaProps: spec.SchemaProps{ Description: "ClaimPropertySets is a provided set of properties applicable to PVC", @@ -16439,6 +16439,13 @@ func schema_pkg_apis_core_v1beta1_StorageProfileStatus(ref common.ReferenceCallb Format: "", }, }, + "cloneStrategy": { + SchemaProps: spec.SchemaProps{ + Description: "CloneStrategy defines the preferred method for performing a CDI clone", + Type: []string{"string"}, + Format: "", + }, + }, "claimPropertySets": { SchemaProps: spec.SchemaProps{ Description: "ClaimPropertySets computed from the spec and detected in the system", diff --git a/pkg/apis/core/v1beta1/types.go b/pkg/apis/core/v1beta1/types.go index 801c55cd7..c212b5c2c 100644 --- a/pkg/apis/core/v1beta1/types.go +++ b/pkg/apis/core/v1beta1/types.go @@ -364,6 +364,8 @@ type StorageProfile struct { //StorageProfileSpec defines specification for StorageProfile type StorageProfileSpec struct { + // CloneStrategy defines the preferred method for performing a CDI clone + CloneStrategy *CDICloneStrategy `json:"cloneStrategy,omitempty"` // ClaimPropertySets is a provided set of properties applicable to PVC ClaimPropertySets []ClaimPropertySet `json:"claimPropertySets,omitempty"` } @@ -374,6 +376,8 @@ type StorageProfileStatus struct { StorageClass *string `json:"storageClass,omitempty"` // The Storage class provisioner plugin name Provisioner *string `json:"provisioner,omitempty"` + // CloneStrategy defines the preferred method for performing a CDI clone + CloneStrategy *CDICloneStrategy `json:"cloneStrategy,omitempty"` // ClaimPropertySets computed from the spec and detected in the system ClaimPropertySets []ClaimPropertySet `json:"claimPropertySets,omitempty"` } @@ -388,8 +392,6 @@ type ClaimPropertySet struct { // Value of Filesystem is implied when not included in claim spec. // +optional VolumeMode *corev1.PersistentVolumeMode `json:"volumeMode,omitempty" protobuf:"bytes,6,opt,name=volumeMode,casttype=PersistentVolumeMode"` - // CloneStrategy defines the preferred method for performing a CDI clone - CloneStrategy *CDICloneStrategy `json:"cloneStrategy,omitempty"` } //StorageProfileList provides the needed parameters to request a list of StorageProfile from the system diff --git a/pkg/apis/core/v1beta1/types_swagger_generated.go b/pkg/apis/core/v1beta1/types_swagger_generated.go index 559a5597c..c7e074f8c 100644 --- a/pkg/apis/core/v1beta1/types_swagger_generated.go +++ b/pkg/apis/core/v1beta1/types_swagger_generated.go @@ -159,6 +159,7 @@ func (StorageProfile) SwaggerDoc() map[string]string { func (StorageProfileSpec) SwaggerDoc() map[string]string { return map[string]string{ "": "StorageProfileSpec defines specification for StorageProfile", + "cloneStrategy": "CloneStrategy defines the preferred method for performing a CDI clone", "claimPropertySets": "ClaimPropertySets is a provided set of properties applicable to PVC", } } @@ -168,16 +169,16 @@ func (StorageProfileStatus) SwaggerDoc() map[string]string { "": "StorageProfileStatus provides the most recently observed status of the StorageProfile", "storageClass": "The StorageClass name for which capabilities are defined", "provisioner": "The Storage class provisioner plugin name", + "cloneStrategy": "CloneStrategy defines the preferred method for performing a CDI clone", "claimPropertySets": "ClaimPropertySets computed from the spec and detected in the system", } } func (ClaimPropertySet) SwaggerDoc() map[string]string { return map[string]string{ - "": "ClaimPropertySet is a set of properties applicable to PVC", - "accessModes": "AccessModes contains the desired access modes the volume should have.\nMore info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#access-modes-1\n+optional", - "volumeMode": "VolumeMode defines what type of volume is required by the claim.\nValue of Filesystem is implied when not included in claim spec.\n+optional", - "cloneStrategy": "CloneStrategy defines the preferred method for performing a CDI clone", + "": "ClaimPropertySet is a set of properties applicable to PVC", + "accessModes": "AccessModes contains the desired access modes the volume should have.\nMore info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#access-modes-1\n+optional", + "volumeMode": "VolumeMode defines what type of volume is required by the claim.\nValue of Filesystem is implied when not included in claim spec.\n+optional", } } diff --git a/pkg/apis/core/v1beta1/zz_generated.deepcopy.go b/pkg/apis/core/v1beta1/zz_generated.deepcopy.go index 91c9e2218..16cd2550e 100644 --- a/pkg/apis/core/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/core/v1beta1/zz_generated.deepcopy.go @@ -365,11 +365,6 @@ func (in *ClaimPropertySet) DeepCopyInto(out *ClaimPropertySet) { *out = new(v1.PersistentVolumeMode) **out = **in } - if in.CloneStrategy != nil { - in, out := &in.CloneStrategy, &out.CloneStrategy - *out = new(CDICloneStrategy) - **out = **in - } return } @@ -1330,6 +1325,11 @@ func (in *StorageProfileList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *StorageProfileSpec) DeepCopyInto(out *StorageProfileSpec) { *out = *in + if in.CloneStrategy != nil { + in, out := &in.CloneStrategy, &out.CloneStrategy + *out = new(CDICloneStrategy) + **out = **in + } if in.ClaimPropertySets != nil { in, out := &in.ClaimPropertySets, &out.ClaimPropertySets *out = make([]ClaimPropertySet, len(*in)) @@ -1363,6 +1363,11 @@ func (in *StorageProfileStatus) DeepCopyInto(out *StorageProfileStatus) { *out = new(string) **out = **in } + if in.CloneStrategy != nil { + in, out := &in.CloneStrategy, &out.CloneStrategy + *out = new(CDICloneStrategy) + **out = **in + } if in.ClaimPropertySets != nil { in, out := &in.ClaimPropertySets, &out.ClaimPropertySets *out = make([]ClaimPropertySet, len(*in)) diff --git a/pkg/controller/datavolume-controller.go b/pkg/controller/datavolume-controller.go index ef72fa722..b9e317631 100644 --- a/pkg/controller/datavolume-controller.go +++ b/pkg/controller/datavolume-controller.go @@ -2526,12 +2526,8 @@ func (r *DatavolumeReconciler) getPreferredCloneStrategyForStorageClass(storageC if err != nil { return nil, errors.Wrap(err, "cannot get StorageProfile") } - if len(storageProfile.Status.ClaimPropertySets) > 0 { - cloneStrategy := storageProfile.Status.ClaimPropertySets[0].CloneStrategy - return cloneStrategy, nil - } - return nil, nil + return storageProfile.Status.CloneStrategy, nil } func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass) (*corev1.PersistentVolumeMode, error) { diff --git a/pkg/controller/storageprofile-controller.go b/pkg/controller/storageprofile-controller.go index 4ae05d332..5f78bd9cd 100644 --- a/pkg/controller/storageprofile-controller.go +++ b/pkg/controller/storageprofile-controller.go @@ -58,6 +58,8 @@ func (r *StorageProfileReconciler) reconcileStorageProfile(sc *storagev1.Storage storageProfile.Status.StorageClass = &sc.Name storageProfile.Status.Provisioner = &sc.Provisioner + storageProfile.Status.CloneStrategy = storageProfile.Spec.CloneStrategy + storageProfile.Status.ClaimPropertySets = storageProfile.Spec.ClaimPropertySets var claimPropertySet *cdiv1.ClaimPropertySet diff --git a/pkg/controller/util_test.go b/pkg/controller/util_test.go index bd4d174a7..80a3e6ecc 100644 --- a/pkg/controller/util_test.go +++ b/pkg/controller/util_test.go @@ -670,10 +670,10 @@ func createStorageProfileWithCloneStrategy(name string, Status: cdiv1.StorageProfileStatus{ StorageClass: &name, ClaimPropertySets: []cdiv1.ClaimPropertySet{{ - AccessModes: accessModes, - VolumeMode: &volumeMode, - CloneStrategy: cloneStrategy, + AccessModes: accessModes, + VolumeMode: &volumeMode, }}, + CloneStrategy: cloneStrategy, }, } } diff --git a/pkg/operator/resources/crds_generated.go b/pkg/operator/resources/crds_generated.go index 752450ac3..c16566c0d 100644 --- a/pkg/operator/resources/crds_generated.go +++ b/pkg/operator/resources/crds_generated.go @@ -3461,14 +3461,14 @@ spec: items: type: string type: array - cloneStrategy: - description: CloneStrategy defines the preferred method for performing a CDI clone - type: string volumeMode: description: VolumeMode defines what type of volume is required by the claim. Value of Filesystem is implied when not included in claim spec. type: string type: object type: array + cloneStrategy: + description: CloneStrategy defines the preferred method for performing a CDI clone + type: string type: object status: description: StorageProfileStatus provides the most recently observed status of the StorageProfile @@ -3483,14 +3483,14 @@ spec: items: type: string type: array - cloneStrategy: - description: CloneStrategy defines the preferred method for performing a CDI clone - type: string volumeMode: description: VolumeMode defines what type of volume is required by the claim. Value of Filesystem is implied when not included in claim spec. type: string type: object type: array + cloneStrategy: + description: CloneStrategy defines the preferred method for performing a CDI clone + type: string provisioner: description: The Storage class provisioner plugin name type: string diff --git a/tests/cloner_test.go b/tests/cloner_test.go index 01c228308..f5d6a2b02 100644 --- a/tests/cloner_test.go +++ b/tests/cloner_test.go @@ -1770,8 +1770,8 @@ func validateCloneType(f *framework.Framework, dv *cdiv1.DataVolume) { defaultCloneStrategy := cdiv1.CDICloneStrategy(cdiv1.CloneStrategySnapshot) cloneStrategy := &defaultCloneStrategy - if len(spec.ClaimPropertySets) > 0 { - cloneStrategy = spec.ClaimPropertySets[0].CloneStrategy + if spec.CloneStrategy != nil { + cloneStrategy = spec.CloneStrategy } sc, err := f.K8sClient.StorageV1().StorageClasses().Get(context.TODO(), f.SnapshotSCName, metav1.GetOptions{}) diff --git a/tests/utils/storageprofile.go b/tests/utils/storageprofile.go index 7b3574712..be084be6b 100644 --- a/tests/utils/storageprofile.go +++ b/tests/utils/storageprofile.go @@ -52,10 +52,7 @@ func ConfigureCloneStrategy(client client.Client, gomega.Eventually(func() *cdiv1.CDICloneStrategy { profile, err := clientSet.CdiV1beta1().StorageProfiles().Get(context.TODO(), storageClassName, metav1.GetOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - if len(profile.Status.ClaimPropertySets) > 0 { - return profile.Status.ClaimPropertySets[0].CloneStrategy - } - return nil + return profile.Status.CloneStrategy }, time.Second*30, time.Second).Should(gomega.Equal(&cloneStrategy)) return nil @@ -63,12 +60,7 @@ func ConfigureCloneStrategy(client client.Client, func updateCloneStrategy(originalProfileSpec *cdiv1.StorageProfileSpec, cloneStrategy cdiv1.CDICloneStrategy) *cdiv1.StorageProfileSpec { newProfileSpec := originalProfileSpec.DeepCopy() - - if len(newProfileSpec.ClaimPropertySets) == 0 { - newProfileSpec.ClaimPropertySets = []cdiv1.ClaimPropertySet{{CloneStrategy: &cloneStrategy}} - } else { - newProfileSpec.ClaimPropertySets[0].CloneStrategy = &cloneStrategy - } + newProfileSpec.CloneStrategy = &cloneStrategy return newProfileSpec }