Skip to content

Commit

Permalink
CR fixes
Browse files Browse the repository at this point in the history 
Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
  • Loading branch information
@arnongilboa
arnongilboa committed Apr 30, 2023
1 parent ea9bc21 commit 137ec17
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 90 deletions.
27 changes: 16 additions & 11 deletions pkg/controller/datavolume/controller-base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,16 @@ func getIndexArgs() []indexArgs {
return nil
},
},
{
obj: &corev1.PersistentVolume{},
field: claimStorageClassNameField,
extractValue: func(obj client.Object) []string {
if pv, ok := obj.(*corev1.PersistentVolume); ok && pv.Status.Phase == corev1.VolumeAvailable {
return []string{pv.Spec.StorageClassName}
}
return nil
},
},
}
}

Expand All @@ -203,14 +213,6 @@ func CreateCommonIndexes(mgr manager.Manager) error {
return err
}
}
if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &corev1.PersistentVolume{}, claimStorageClassNameField, func(obj client.Object) []string {
if pv, ok := obj.(*corev1.PersistentVolume); ok {
return []string{pv.Spec.StorageClassName}
}
return nil
}); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -319,6 +321,7 @@ func addDataVolumeControllerCommonWatches(mgr manager.Manager, dataVolumeControl
if storage != nil &&
storage.StorageClassName != nil &&
*storage.StorageClassName == pv.Spec.StorageClassName &&
pv.Status.Phase == corev1.VolumeAvailable &&
getDataVolumeOp(mgr.GetLogger(), &dv, mgr.GetClient()) == op {
reqs = append(reqs, reconcile.Request{NamespacedName: types.NamespacedName{Name: dv.Name, Namespace: dv.Namespace}})
}
Expand Down Expand Up @@ -443,10 +446,12 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request,
}
}

syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, dv)
syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, dv, syncState.pvc)
if err != nil {
r.syncDataVolumeStatusPhaseWithEvent(&syncState, cdiv1.PhaseUnset, nil,
Event{corev1.EventTypeWarning, cc.ErrClaimNotValid, err.Error()})
if syncErr := r.syncDataVolumeStatusPhaseWithEvent(&syncState, cdiv1.PhaseUnset, nil,
Event{corev1.EventTypeWarning, cc.ErrClaimNotValid, err.Error()}); syncErr != nil {
log.Error(syncErr, "failed to sync DataVolume status with event")
}
return syncState, err
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/pvc-clone-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ func (r *PvcCloneReconciler) isSourcePVCPopulated(dv *cdiv1.DataVolume) (bool, e
}

func (r *PvcCloneReconciler) sourceInUse(dv *cdiv1.DataVolume, eventReason string) (bool, error) {
pods, err := cc.GetPodsUsingPVCs(r.client, dv.Spec.Source.PVC.Namespace, sets.New[string](dv.Spec.Source.PVC.Name), false)
pods, err := cc.GetPodsUsingPVCs(r.client, dv.Spec.Source.PVC.Namespace, sets.New(dv.Spec.Source.PVC.Name), false)
if err != nil {
return false, err
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/controller/datavolume/pvc-clone-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ var _ = Describe("All DataVolume Tests", func() {
{AccessModes: accessMode, VolumeMode: &BlockMode}}, &cloneStrategy)

reconciler := createCloneReconciler(dv, storageProfile, sc)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, nil)
Expect(err).ToNot(HaveOccurred())
done, err := reconciler.detectCloneSize(syncState(dv, targetPvc, pvcSpec), HostAssistedClone)
Expect(err).To(HaveOccurred())
Expand All @@ -747,7 +747,7 @@ var _ = Describe("All DataVolume Tests", func() {
pvc := CreatePvcInStorageClass("test", metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimBound)
reconciler := createCloneReconciler(dv, pvc, storageProfile, sc)

pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
done, err := reconciler.detectCloneSize(syncState(dv, pvc, pvcSpec), HostAssistedClone)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -775,7 +775,7 @@ var _ = Describe("All DataVolume Tests", func() {
pvc.GetAnnotations()[AnnPodPhase] = string(corev1.PodSucceeded)
reconciler := createCloneReconciler(dv, pvc, storageProfile, sc)

pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
done, err := reconciler.detectCloneSize(syncState(dv, pvc, pvcSpec), HostAssistedClone)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -811,7 +811,7 @@ var _ = Describe("All DataVolume Tests", func() {
Expect(err).ToNot(HaveOccurred())

// Checks
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
done, err := reconciler.detectCloneSize(syncState(dv, pvc, pvcSpec), HostAssistedClone)
Expect(err).To(HaveOccurred())
Expand Down Expand Up @@ -852,7 +852,7 @@ var _ = Describe("All DataVolume Tests", func() {
Expect(err).ToNot(HaveOccurred())

// Get the expected value
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
expectedSize, err := cc.InflateSizeWithOverhead(reconciler.client, int64(100), pvcSpec)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -883,7 +883,7 @@ var _ = Describe("All DataVolume Tests", func() {
reconciler := createCloneReconciler(dv, pvc, storageProfile, sc)

// Get the expected value
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
expectedSize, err := cc.InflateSizeWithOverhead(reconciler.client, int64(100), pvcSpec)
Expect(err).ToNot(HaveOccurred())
Expand All @@ -910,7 +910,7 @@ var _ = Describe("All DataVolume Tests", func() {
pvc.Spec.VolumeMode = &volumeMode
reconciler := createCloneReconciler(dv, pvc, storageProfile, sc)

pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv)
pvcSpec, err := renderPvcSpec(reconciler.client, reconciler.recorder, reconciler.log, dv, pvc)
Expect(err).ToNot(HaveOccurred())
expectedSize := *pvc.Status.Capacity.Storage()
done, err := reconciler.detectCloneSize(syncState(dv, pvc, pvcSpec), selectedCloneStrategy)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/datavolume/smart-clone-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func (r *SmartCloneReconciler) reconcileSnapshot(log logr.Logger, snapshot *snap
return reconcile.Result{}, nil
}

targetPvcSpec, err := renderPvcSpec(r.client, r.recorder, r.log, dataVolume)
targetPvcSpec, err := renderPvcSpec(r.client, r.recorder, r.log, dataVolume, nil)
if err != nil {
return reconcile.Result{}, err
}
Expand Down
143 changes: 75 additions & 68 deletions pkg/controller/datavolume/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/go-logr/logr"
"github.com/pkg/errors"

corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand All @@ -45,93 +44,105 @@ const (
)

// renderPvcSpec creates a new PVC Spec based on either the dv.spec.pvc or dv.spec.storage section
func renderPvcSpec(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume) (*v1.PersistentVolumeClaimSpec, error) {
func renderPvcSpec(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume, pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaimSpec, error) {
if dv.Spec.PVC != nil {
return dv.Spec.PVC.DeepCopy(), nil
} else if dv.Spec.Storage != nil {
return pvcFromStorage(client, recorder, log, dv)
return pvcFromStorage(client, recorder, log, dv, pvc)
}

return nil, errors.Errorf("datavolume one of {pvc, storage} field is required")
}

func pvcFromStorage(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume) (*v1.PersistentVolumeClaimSpec, error) {
storage := dv.Spec.Storage
pvcSpec := copyStorageAsPvc(log, storage)
func pvcFromStorage(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume, pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaimSpec, error) {
var pvcSpec *v1.PersistentVolumeClaimSpec

if pvc == nil {
pvcSpec = copyStorageAsPvc(log, dv.Spec.Storage)
if err := renderPvcSpecVolumeModeAndAccessModes(client, recorder, log, dv, pvcSpec); err != nil {
return nil, err
}
} else {
pvcSpec = pvc.Spec.DeepCopy()
}

if err := renderPvcSpecVolumeSize(client, dv.Spec, pvcSpec); err != nil {
return nil, err
}

return pvcSpec, nil
}

func renderPvcSpecVolumeModeAndAccessModes(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume, pvcSpec *v1.PersistentVolumeClaimSpec) error {
if dv.Spec.ContentType == cdiv1.DataVolumeArchive {
if pvcSpec.VolumeMode != nil && *pvcSpec.VolumeMode == v1.PersistentVolumeBlock {
log.V(1).Info("DataVolume with ContentType Archive cannot have block volumeMode", "namespace", dv.Namespace, "name", dv.Name)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, "DataVolume with ContentType Archive cannot have block volumeMode")
return nil, errors.Errorf("DataVolume with ContentType Archive cannot have block volumeMode")
return errors.Errorf("DataVolume with ContentType Archive cannot have block volumeMode")
}
volumeMode := v1.PersistentVolumeFilesystem
pvcSpec.VolumeMode = &volumeMode
}

storageClass, err := cc.GetStorageClassByName(client, storage.StorageClassName)
storageClass, err := cc.GetStorageClassByName(client, dv.Spec.Storage.StorageClassName)
if err != nil {
return nil, err
return err
}
if storageClass == nil {
// If SC was not found, look for PV with this SC name
pv, err := getPV(client, pvcSpec, dv.Name, dv.Namespace)
if err != nil {
return nil, err
}
if pv != nil {
pvcSpec.VolumeMode = pv.Spec.VolumeMode
if len(pvcSpec.AccessModes) == 0 {
pvcSpec.AccessModes = pv.Spec.AccessModes
}
if err := renderPvcSpecFromAvailablePv(client, pvcSpec); err != nil {
return err
}
// Not even default storageClass on the cluster, cannot apply the defaults, verify spec is ok
if len(pvcSpec.AccessModes) == 0 {
log.V(1).Info("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, "DataVolume.storage spec is missing accessMode and no storageClass to choose profile")
return nil, errors.Errorf("DataVolume spec is missing accessMode")
return errors.Errorf("DataVolume spec is missing accessMode")
}
} else {
pvcSpec.StorageClassName = &storageClass.Name
// given storageClass we can apply defaults if needed
if (pvcSpec.VolumeMode == nil || *pvcSpec.VolumeMode == "") && (len(pvcSpec.AccessModes) == 0) {
accessModes, volumeMode, err := getDefaultVolumeAndAccessMode(client, storageClass)
if err != nil {
log.V(1).Info("Cannot set accessMode and volumeMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid,
fmt.Sprintf("DataVolume.storage spec is missing accessMode and volumeMode, cannot get access mode from StorageProfile %s", getName(storageClass)))
return nil, err
}
pvcSpec.AccessModes = append(pvcSpec.AccessModes, accessModes...)
pvcSpec.VolumeMode = volumeMode
} else if len(pvcSpec.AccessModes) == 0 {
accessModes, err := getDefaultAccessModes(client, storageClass, pvcSpec.VolumeMode)
if err != nil {
log.V(1).Info("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid,
fmt.Sprintf("DataVolume.storage spec is missing accessMode and cannot get access mode from StorageProfile %s", getName(storageClass)))
return nil, err
}
pvcSpec.AccessModes = append(pvcSpec.AccessModes, accessModes...)
} else if pvcSpec.VolumeMode == nil || *pvcSpec.VolumeMode == "" {
volumeMode, err := getDefaultVolumeMode(client, storageClass, pvcSpec.AccessModes)
if err != nil {
return nil, err
}
pvcSpec.VolumeMode = volumeMode
return nil
}

pvcSpec.StorageClassName = &storageClass.Name
// given storageClass we can apply defaults if needed
if (pvcSpec.VolumeMode == nil || *pvcSpec.VolumeMode == "") && (len(pvcSpec.AccessModes) == 0) {
accessModes, volumeMode, err := getDefaultVolumeAndAccessMode(client, storageClass)
if err != nil {
log.V(1).Info("Cannot set accessMode and volumeMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid,
fmt.Sprintf("DataVolume.storage spec is missing accessMode and volumeMode, cannot get access mode from StorageProfile %s", getName(storageClass)))
return err
}
pvcSpec.AccessModes = append(pvcSpec.AccessModes, accessModes...)
pvcSpec.VolumeMode = volumeMode
} else if len(pvcSpec.AccessModes) == 0 {
accessModes, err := getDefaultAccessModes(client, storageClass, pvcSpec.VolumeMode)
if err != nil {
log.V(1).Info("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err)
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid,
fmt.Sprintf("DataVolume.storage spec is missing accessMode and cannot get access mode from StorageProfile %s", getName(storageClass)))
return err
}
pvcSpec.AccessModes = append(pvcSpec.AccessModes, accessModes...)
} else if pvcSpec.VolumeMode == nil || *pvcSpec.VolumeMode == "" {
volumeMode, err := getDefaultVolumeMode(client, storageClass, pvcSpec.AccessModes)
if err != nil {
return err
}
pvcSpec.VolumeMode = volumeMode
}

requestedVolumeSize, err := resolveVolumeSize(client, dv.Spec, pvcSpec)
return nil
}

func renderPvcSpecVolumeSize(client client.Client, dvSpec cdiv1.DataVolumeSpec, pvcSpec *v1.PersistentVolumeClaimSpec) error {
requestedVolumeSize, err := resolveVolumeSize(client, dvSpec, pvcSpec)
if err != nil {
return nil, err
return err
}
if pvcSpec.Resources.Requests == nil {
pvcSpec.Resources.Requests = v1.ResourceList{}
}
pvcSpec.Resources.Requests[v1.ResourceStorage] = *requestedVolumeSize

return pvcSpec, nil
return nil
}

func getName(storageClass *storagev1.StorageClass) string {
Expand All @@ -157,35 +168,31 @@ func copyStorageAsPvc(log logr.Logger, storage *cdiv1.StorageSpec) *v1.Persisten
return pvcSpec
}

// Gets either the bound PV or an available satisfying PV
func getPV(c client.Client, pvcSpec *v1.PersistentVolumeClaimSpec, pvcName, pvcNamespace string) (*corev1.PersistentVolume, error) {
// Renders the PVC spec VolumeMode and AccessModes from an available satisfying PV
func renderPvcSpecFromAvailablePv(c client.Client, pvcSpec *v1.PersistentVolumeClaimSpec) error {
if pvcSpec.StorageClassName == nil {
return nil, nil
return nil
}

pvList := &v1.PersistentVolumeList{}
fields := client.MatchingFields{claimStorageClassNameField: *pvcSpec.StorageClassName}
if err := c.List(context.TODO(), pvList, fields); err != nil {
return nil, err
}
for _, pv := range pvList.Items {
if pv.Status.Phase == corev1.VolumeBound &&
pv.Spec.ClaimRef != nil &&
pv.Spec.ClaimRef.Name == pvcName &&
pv.Spec.ClaimRef.Namespace == pvcNamespace {
return &pv, nil
}
return err
}
for _, pv := range pvList.Items {
if pv.Status.Phase == corev1.VolumeAvailable {
pvc := &corev1.PersistentVolumeClaim{Spec: *pvcSpec}
if pv.Status.Phase == v1.VolumeAvailable {
pvc := &v1.PersistentVolumeClaim{Spec: *pvcSpec}
if err := checkVolumeSatisfyClaim(&pv, pvc); err == nil {
return &pv, nil
pvcSpec.VolumeMode = pv.Spec.VolumeMode
if len(pvcSpec.AccessModes) == 0 {
pvcSpec.AccessModes = pv.Spec.AccessModes
}
return nil
}
}
}

return nil, nil
return nil
}

func getDefaultVolumeAndAccessMode(c client.Client, storageClass *storagev1.StorageClass) ([]v1.PersistentVolumeAccessMode, *v1.PersistentVolumeMode, error) {
Expand Down
4 changes: 2 additions & 2 deletions tests/datavolume_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2421,7 +2421,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, dataVolume.Namespace, v1.ClaimBound, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())

By("waiting for dv succeeeded")
By("waiting for dv succeeded")
err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())
},
Expand Down Expand Up @@ -2479,7 +2479,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, dataVolume.Namespace, v1.ClaimBound, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())

By("waiting for dv succeeeded")
By("waiting for dv succeeded")
err = utils.WaitForDataVolumePhase(f, dataVolume.Namespace, cdiv1.Succeeded, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())
},
Expand Down

0 comments on commit 137ec17

Please sign in to comment.