Skip to content

Commit

Permalink
Create PVC if possible even if the StorageClass is missing (#2683)
Browse files Browse the repository at this point in the history 
* Create PVC if possible even if SC is missing

When PVC is created with storageClassName and the SC is not found,
k8s looks for PV with the storageClassName for satisfying this claim.
In this case k8s supports also a blank (“”, not the nil default one)
storageClassName. To support this behavior we added:
-DV controller support for this flow (for both “” and non-existing SC)
-Condition update and event when StorageSpec PVC rendering errors and
 PVC is not created (e.g. missing both AccessModes and SC/PV)
-PVC is created even if a satisfying SC/PV doesn't exist if pvc/storage
 AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase
 will be Pending until a satisfying SC/PV is found
-PV watch to reconcile DVs waiting for the PV storageClassName
-PV storageClassName indexer, so we can list the relevant PVs

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

* CR fixes

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

---------

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
  • Loading branch information
@arnongilboa
arnongilboa committed May 2, 2023
1 parent 163f77a commit 5d78da3
Show file tree
Hide file tree
Showing 14 changed files with 447 additions and 100 deletions.
10 changes: 6 additions & 4 deletions pkg/apiserver/webhooks/dataimportcron-validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ type dataImportCronValidatingWebhook struct {
}

func (wh *dataImportCronValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
var causes []metav1.StatusCause

if ar.Request.Resource.Group != cdiv1.CDIGroupVersionKind.Group || ar.Request.Resource.Resource != "dataimportcrons" {
klog.V(3).Infof("Got unexpected resource type %s", ar.Request.Resource.Resource)
return toAdmissionResponseError(fmt.Errorf("unexpected resource: %s", ar.Request.Resource.Resource))
Expand All @@ -52,8 +54,8 @@ func (wh *dataImportCronValidatingWebhook) Admit(ar admissionv1.AdmissionReview)
return toAdmissionResponseError(err)
}

causes := validateNameLength(cron.Name, validation.DNS1035LabelMaxLength)
if len(causes) > 0 {
if cause := validateNameLength(cron.Name, validation.DNS1035LabelMaxLength); cause != nil {
causes = append(causes, *cause)
return toRejectedAdmissionResponse(causes)
}

Expand Down Expand Up @@ -152,8 +154,8 @@ func (wh *dataImportCronValidatingWebhook) validateDataImportCronSpec(request *a
return causes
}

causes = validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength)
if len(causes) > 0 {
if cause := validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength); cause != nil {
causes = append(causes, *cause)
return causes
}

Expand Down
35 changes: 28 additions & 7 deletions pkg/apiserver/webhooks/datavolume-validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,15 @@ func validateSourceURL(sourceURL string) string {
return ""
}

func validateNameLength(name string, maxLen int) []metav1.StatusCause {
var causes []metav1.StatusCause
func validateNameLength(name string, maxLen int) *metav1.StatusCause {
if len(name) > maxLen {
causes = append(causes, metav1.StatusCause{
return &metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: fmt.Sprintf("Name cannot be longer than %d characters", maxLen),
Field: "",
})
}
}
return causes
return nil
}

func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admissionv1.AdmissionRequest, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec, namespace *string) []metav1.StatusCause {
Expand Down Expand Up @@ -104,6 +103,10 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission
causes = append(causes, *cause)
return causes
}
if cause := validateStorageClassName(spec, field); cause != nil {
causes = append(causes, *cause)
return causes
}

if spec.PVC != nil {
dataSourceRef = spec.PVC.DataSourceRef
Expand Down Expand Up @@ -564,6 +567,22 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourceSnapshot(snapshot
return nil
}

func validateStorageClassName(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) *metav1.StatusCause {
var sc *string

if spec.PVC != nil {
sc = spec.PVC.StorageClassName
} else if spec.Storage != nil {
sc = spec.Storage.StorageClassName
}

if sc == nil || *sc == "" {
return nil
}

return validateNameLength(*sc, kvalidation.DNS1123SubdomainMaxLength)
}

func validateStorageSize(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) (*metav1.StatusCause, bool) {
var name string
var resources v1.ResourceRequirements
Expand Down Expand Up @@ -633,6 +652,8 @@ func validateExternalPopulation(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path
}

func (wh *dataVolumeValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
var causes []metav1.StatusCause

klog.V(3).Infof("Got AdmissionReview %+v", ar)

if err := validateDataVolumeResource(ar); err != nil {
Expand Down Expand Up @@ -682,9 +703,9 @@ func (wh *dataVolumeValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *ad
}
}

causes := validateNameLength(dv.Name, kvalidation.DNS1123SubdomainMaxLength)
if len(causes) > 0 {
if cause := validateNameLength(dv.Name, kvalidation.DNS1123SubdomainMaxLength); cause != nil {
klog.Infof("rejected DataVolume admission")
causes = append(causes, *cause)
return toRejectedAdmissionResponse(causes)
}

Expand Down
44 changes: 37 additions & 7 deletions pkg/apiserver/webhooks/datavolume-validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strings"

. "github.com/onsi/ginkgo"
. "github.com/onsi/ginkgo/extensions/table"
Expand All @@ -38,6 +39,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
fakeclient "k8s.io/client-go/kubernetes/fake"
"k8s.io/utils/pointer"

snapclientfake "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake"
cdiclientfake "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned/fake"
Expand All @@ -51,6 +53,8 @@ var (
)

var _ = Describe("Validating Webhook", func() {
longName := "name-is-longer-than-253-" + strings.Repeat("0123456789", 23)

Context("with DataVolume admission review", func() {
It("should accept DataVolume with HTTP source on create", func() {
dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
Expand Down Expand Up @@ -215,16 +219,43 @@ var _ = Describe("Validating Webhook", func() {
})

It("should reject DataVolume with name length greater than 253 characters", func() {
longName := "the-name-length-of-this-datavolume-is-greater-then-253-characters" +
"123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" +
"123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789"
dataVolume := newHTTPDataVolume(
longName,
"http://www.example.com")
dataVolume := newHTTPDataVolume(longName, "http://www.example.com")
resp := validateDataVolumeCreate(dataVolume)
Expect(resp.Allowed).To(Equal(false))
})

DescribeTable("should", func(scName *string, expected bool) {
httpSource := &cdiv1.DataVolumeSource{
HTTP: &cdiv1.DataVolumeSourceHTTP{URL: "http://www.example.com"},
}
storage := &cdiv1.StorageSpec{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceStorage: resource.MustParse("500Mi"),
},
},
StorageClassName: scName,
}
dv := newDataVolumeWithStorageSpec("testDV", httpSource, nil, storage)
resp := validateDataVolumeCreate(dv)
Expect(resp.Allowed).To(Equal(expected))
},
Entry("should accept DataVolume with Storage spec blank StorageClassName", pointer.String(""), true),
Entry("should reject DataVolume with Storage spec too long StorageClassName", pointer.String(longName), false),
Entry("should accept DataVolume with Storage spec empty StorageClassName", nil, true),
)

DescribeTable("should", func(scName *string, expected bool) {
dv := newHTTPDataVolume("testDV", "http://www.example.com")
dv.Spec.PVC.StorageClassName = scName
resp := validateDataVolumeCreate(dv)
Expect(resp.Allowed).To(Equal(expected))
},
Entry("should accept DataVolume with PVC spec blank StorageClassName", pointer.String(""), true),
Entry("should reject DataVolume with PVC spec too long StorageClassName", pointer.String(longName), false),
Entry("should accept DataVolume with PVC spec empty StorageClassName", nil, true),
)

It("should reject DataVolume source with invalid URL on create", func() {
dataVolume := newHTTPDataVolume("testDV", "invalidurl")
resp := validateDataVolumeCreate(dataVolume)
Expand Down Expand Up @@ -277,7 +308,6 @@ var _ = Describe("Validating Webhook", func() {
dataVolume.Spec.ContentType = "invalid"
resp := validateDataVolumeCreate(dataVolume)
Expect(resp.Allowed).To(Equal(false))

})

It("should accept DataVolume with archive contentType", func() {
Expand Down
7 changes: 5 additions & 2 deletions pkg/controller/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,11 @@ func GetStorageClassByName(client client.Client, name *string) (*storagev1.Stora
if name != nil {
storageClass := &storagev1.StorageClass{}
if err := client.Get(context.TODO(), types.NamespacedName{Name: *name}, storageClass); err != nil {
if k8serrors.IsNotFound(err) {
return nil, nil
}
klog.V(3).Info("Unable to retrieve storage class", "storage class name", *name)
return nil, errors.New("unable to retrieve storage class")
return nil, errors.Errorf("unable to retrieve storage class %s", *name)
}
return storageClass, nil
}
Expand Down Expand Up @@ -392,7 +395,7 @@ func GetFilesystemOverheadForStorageClass(client client.Client, storageClassName
}

targetStorageClass, err := GetStorageClassByName(client, storageClassName)
if err != nil {
if err != nil || targetStorageClass == nil {
klog.V(3).Info("Storage class", storageClassName, "not found, trying default storage class")
targetStorageClass, err = GetStorageClassByName(client, nil)
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions pkg/controller/datavolume/conditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func UpdateReadyCondition(conditions []cdiv1.DataVolumeCondition, status corev1.
return updateCondition(conditions, cdiv1.DataVolumeReady, status, message, reason)
}

func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, reason string) []cdiv1.DataVolumeCondition {
func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, message, reason string) []cdiv1.DataVolumeCondition {
if pvc != nil {
pvcCondition := getPVCCondition(pvc.GetAnnotations())
switch pvc.Status.Phase {
Expand All @@ -149,10 +149,13 @@ func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.Pe
conditions = UpdateReadyCondition(conditions, corev1.ConditionFalse, "", "")
}
} else {
if message == "" {
message = "No PVC found"
}
if reason == "" {
reason = cc.NotFound
}
conditions = updateCondition(conditions, cdiv1.DataVolumeBound, corev1.ConditionUnknown, "No PVC found", reason)
conditions = updateCondition(conditions, cdiv1.DataVolumeBound, corev1.ConditionUnknown, message, reason)
conditions = UpdateReadyCondition(conditions, corev1.ConditionFalse, "", "")
}
return conditions
Expand Down
30 changes: 21 additions & 9 deletions pkg/controller/datavolume/conditions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ var _ = Describe("updateReadyCondition", func() {
var _ = Describe("updateBoundCondition", func() {
It("should create condition if it doesn't exist", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
conditions = updateBoundCondition(conditions, nil, "")
conditions = updateBoundCondition(conditions, nil, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -147,7 +147,7 @@ var _ = Describe("updateBoundCondition", func() {
It("should create condition with reason if it doesn't exist", func() {
reason := "exceeded quota"
conditions := make([]cdiv1.DataVolumeCondition, 0)
conditions = updateBoundCondition(conditions, nil, reason)
conditions = updateBoundCondition(conditions, nil, "", reason)
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -156,11 +156,23 @@ var _ = Describe("updateBoundCondition", func() {
Expect(condition.Status).To(Equal(corev1.ConditionUnknown))
})

It("should create condition with message if one passed", func() {
message := "message"
conditions := make([]cdiv1.DataVolumeCondition, 0)
conditions = updateBoundCondition(conditions, nil, message, "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expect(condition.Message).To(Equal(message))
Expect(condition.Reason).To(Equal(NotFound))
Expect(condition.Status).To(Equal(corev1.ConditionUnknown))
})

It("should be bound if PVC bound", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
pvc.Status.Phase = corev1.ClaimBound
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(1))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -173,7 +185,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "true"}, nil)
pvc.Status.Phase = corev1.ClaimBound
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(1))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -186,7 +198,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "false", AnnBoundConditionReason: "not bound", AnnBoundConditionMessage: "scratch PVC not bound"}, nil)
pvc.Status.Phase = corev1.ClaimBound
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -204,7 +216,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
pvc.Status.Phase = corev1.ClaimPending
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -222,7 +234,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "true"}, nil)
pvc.Status.Phase = corev1.ClaimPending
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -241,7 +253,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "false", AnnBoundConditionReason: reason, AnnBoundConditionMessage: "scratch PVC not bound"}, nil)
pvc.Status.Phase = corev1.ClaimPending
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand All @@ -259,7 +271,7 @@ var _ = Describe("updateBoundCondition", func() {
conditions := make([]cdiv1.DataVolumeCondition, 0)
pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
pvc.Status.Phase = corev1.ClaimLost
conditions = updateBoundCondition(conditions, pvc, "")
conditions = updateBoundCondition(conditions, pvc, "", "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expand Down

0 comments on commit 5d78da3

Please sign in to comment.