Skip to content

Commit

Permalink
Allow creating PVC if there is a satisfying PV
Browse files Browse the repository at this point in the history 
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. SC/PV is not 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>
  • Loading branch information
@arnongilboa
arnongilboa committed Apr 19, 2023
1 parent 8cb11f5 commit e2203a3
Show file tree
Hide file tree
Showing 11 changed files with 304 additions and 47 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
2 changes: 1 addition & 1 deletion pkg/controller/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func GetStorageClassByName(client client.Client, name *string) (*storagev1.Stora
storageClass := &storagev1.StorageClass{}
if err := client.Get(context.TODO(), types.NamespacedName{Name: *name}, storageClass); err != 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.New(fmt.Sprintf("unable to retrieve storage class %s", *name))
}
return storageClass, 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
18 changes: 9 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 @@ -160,7 +160,7 @@ var _ = Describe("updateBoundCondition", 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 +173,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 +186,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 +204,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 +222,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 +241,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 +259,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 e2203a3

Please sign in to comment.