From 0b848bee4e89d89a1bedf4c38afd9188f2bc3dec Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 13:31:43 +0800 Subject: [PATCH 1/3] pvc storage request warning for fractional byte value - create or update --- pkg/api/persistentvolumeclaim/util.go | 20 +++++++++++++++++++ .../core/persistentvolumeclaim/strategy.go | 4 ++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index e8b56e74b7d..b8cacf83a4c 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -17,6 +17,10 @@ limitations under the License. package persistentvolumeclaim import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" @@ -151,3 +155,19 @@ func allocatedResourcesInUse(oldPVC *core.PersistentVolumeClaim) bool { return false } + +func GetWarningsForPersistentVolumeClaim(pv *core.PersistentVolumeClaim) []string { + if pv == nil { + return nil + } + storageValue := pv.Spec.Resources.Requests[core.ResourceStorage] + return warningsForPersistentVolumeClaimResources(field.NewPath("spec").Child("Resources").Child("Requests").Key(core.ResourceStorage.String()), storageValue) +} + +func warningsForPersistentVolumeClaimResources(fieldPath *field.Path, storageValue resource.Quantity) []string { + var warnings []string + if storageValue.MilliValue()%int64(1000) != int64(0) { + warnings = append(warnings, fmt.Sprintf("%s: fractional byte value %q is invalid, must be an integer", fieldPath.String(), storageValue.String())) + } + return warnings +} diff --git a/pkg/registry/core/persistentvolumeclaim/strategy.go b/pkg/registry/core/persistentvolumeclaim/strategy.go index dad8db0915c..14bf61e2b33 100644 --- a/pkg/registry/core/persistentvolumeclaim/strategy.go +++ b/pkg/registry/core/persistentvolumeclaim/strategy.go @@ -86,7 +86,7 @@ func (persistentvolumeclaimStrategy) Validate(ctx context.Context, obj runtime.O // WarningsOnCreate returns warnings for the creation of the given object. func (persistentvolumeclaimStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { - return nil + return pvcutil.GetWarningsForPersistentVolumeClaim(obj.(*api.PersistentVolumeClaim)) } // Canonicalize normalizes the object after validation. @@ -128,7 +128,7 @@ func (persistentvolumeclaimStrategy) ValidateUpdate(ctx context.Context, obj, ol // WarningsOnUpdate returns warnings for the given update. func (persistentvolumeclaimStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { - return nil + return pvcutil.GetWarningsForPersistentVolumeClaim(obj.(*api.PersistentVolumeClaim)) } func (persistentvolumeclaimStrategy) AllowUnconditionalUpdate() bool { From ca94a89414618a93f3e3dcfc1e5afb5b0209bec1 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 13:43:40 +0800 Subject: [PATCH 2/3] pvc warning for storage request: add unit test --- pkg/api/persistentvolumeclaim/util.go | 10 ++-- pkg/api/persistentvolumeclaim/util_test.go | 70 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index b8cacf83a4c..519aaeb18cf 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -19,7 +19,6 @@ package persistentvolumeclaim import ( "fmt" - "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/apis/core" @@ -161,13 +160,12 @@ func GetWarningsForPersistentVolumeClaim(pv *core.PersistentVolumeClaim) []strin return nil } storageValue := pv.Spec.Resources.Requests[core.ResourceStorage] - return warningsForPersistentVolumeClaimResources(field.NewPath("spec").Child("Resources").Child("Requests").Key(core.ResourceStorage.String()), storageValue) -} - -func warningsForPersistentVolumeClaimResources(fieldPath *field.Path, storageValue resource.Quantity) []string { var warnings []string if storageValue.MilliValue()%int64(1000) != int64(0) { - warnings = append(warnings, fmt.Sprintf("%s: fractional byte value %q is invalid, must be an integer", fieldPath.String(), storageValue.String())) + warnings = append(warnings, fmt.Sprintf( + "%s: fractional byte value %q is invalid, must be an integer", + field.NewPath("spec").Child("resources").Child("requests").Key(core.ResourceStorage.String()), + storageValue.String())) } return warnings } diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go index 8462cde39f9..1b7d260b7d3 100644 --- a/pkg/api/persistentvolumeclaim/util_test.go +++ b/pkg/api/persistentvolumeclaim/util_test.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/sets" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -397,3 +398,72 @@ func withResizeStatus(status core.PersistentVolumeClaimResizeStatus) *core.Persi }, } } + +func TestWarnings(t *testing.T) { + testcases := []struct { + name string + template *core.PersistentVolumeClaim + expected []string + }{ + { + name: "null", + template: nil, + expected: nil, + }, + { + name: "200Mi no warning", + template: &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceStorage: resource.MustParse("200Mi"), + }, + }, + }, + }, + expected: nil, + }, + { + name: "200m warning", + template: &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceStorage: resource.MustParse("200m"), + }, + }, + }, + }, + expected: []string{ + `spec.resources.requests[storage]: fractional byte value "200m" is invalid, must be an integer`, + }, + }, + { + name: "integer no warning", + template: &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceStorage: resource.MustParse("200"), + }, + }, + }, + }, + expected: nil, + }, + } + + for _, tc := range testcases { + t.Run("pvcspec_"+tc.name, func(t *testing.T) { + actual := sets.NewString(GetWarningsForPersistentVolumeClaim(tc.template)...) + expected := sets.NewString(tc.expected...) + for _, missing := range expected.Difference(actual).List() { + t.Errorf("missing: %s", missing) + } + for _, extra := range actual.Difference(expected).List() { + t.Errorf("extra: %s", extra) + } + }) + + } +} From 140502af8cbe4de0b78265f4ba025a5ada805849 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Fri, 21 Oct 2022 22:31:48 +0800 Subject: [PATCH 3/3] add warning for PVC template in statefulset and in pod ephemeral volume source --- pkg/api/persistentvolumeclaim/util.go | 19 +++++++++++--- pkg/api/persistentvolumeclaim/util_test.go | 9 ++++++- pkg/api/pod/warnings.go | 4 +++ pkg/api/pod/warnings_test.go | 30 ++++++++++++++++++++++ pkg/registry/apps/statefulset/strategy.go | 11 +++++++- test/e2e/framework/.import-restrictions | 1 + 6 files changed, 68 insertions(+), 6 deletions(-) diff --git a/pkg/api/persistentvolumeclaim/util.go b/pkg/api/persistentvolumeclaim/util.go index 519aaeb18cf..2f24c645b66 100644 --- a/pkg/api/persistentvolumeclaim/util.go +++ b/pkg/api/persistentvolumeclaim/util.go @@ -159,13 +159,24 @@ func GetWarningsForPersistentVolumeClaim(pv *core.PersistentVolumeClaim) []strin if pv == nil { return nil } - storageValue := pv.Spec.Resources.Requests[core.ResourceStorage] + + return GetWarningsForPersistentVolumeClaimSpec(field.NewPath("spec"), pv.Spec) +} + +func GetWarningsForPersistentVolumeClaimSpec(fieldPath *field.Path, pvSpec core.PersistentVolumeClaimSpec) []string { + var warnings []string - if storageValue.MilliValue()%int64(1000) != int64(0) { + requestValue := pvSpec.Resources.Requests[core.ResourceStorage] + if requestValue.MilliValue()%int64(1000) != int64(0) { warnings = append(warnings, fmt.Sprintf( "%s: fractional byte value %q is invalid, must be an integer", - field.NewPath("spec").Child("resources").Child("requests").Key(core.ResourceStorage.String()), - storageValue.String())) + fieldPath.Child("resources").Child("requests").Key(core.ResourceStorage.String()), requestValue.String())) + } + limitValue := pvSpec.Resources.Limits[core.ResourceStorage] + if limitValue.MilliValue()%int64(1000) != int64(0) { + warnings = append(warnings, fmt.Sprintf( + "%s: fractional byte value %q is invalid, must be an integer", + fieldPath.Child("resources").Child("limits").Key(core.ResourceStorage.String()), limitValue.String())) } return warnings } diff --git a/pkg/api/persistentvolumeclaim/util_test.go b/pkg/api/persistentvolumeclaim/util_test.go index 1b7d260b7d3..a0e4f5fec63 100644 --- a/pkg/api/persistentvolumeclaim/util_test.go +++ b/pkg/api/persistentvolumeclaim/util_test.go @@ -411,13 +411,16 @@ func TestWarnings(t *testing.T) { expected: nil, }, { - name: "200Mi no warning", + name: "200Mi requests no warning", template: &core.PersistentVolumeClaim{ Spec: core.PersistentVolumeClaimSpec{ Resources: core.ResourceRequirements{ Requests: core.ResourceList{ core.ResourceStorage: resource.MustParse("200Mi"), }, + Limits: core.ResourceList{ + core.ResourceStorage: resource.MustParse("200Mi"), + }, }, }, }, @@ -431,11 +434,15 @@ func TestWarnings(t *testing.T) { Requests: core.ResourceList{ core.ResourceStorage: resource.MustParse("200m"), }, + Limits: core.ResourceList{ + core.ResourceStorage: resource.MustParse("100m"), + }, }, }, }, expected: []string{ `spec.resources.requests[storage]: fractional byte value "200m" is invalid, must be an integer`, + `spec.resources.limits[storage]: fractional byte value "100m" is invalid, must be an integer`, }, }, { diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 146f0cca61d..f2e5d8fa01d 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" nodeapi "k8s.io/kubernetes/pkg/api/node" + pvcutil "k8s.io/kubernetes/pkg/api/persistentvolumeclaim" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/pods" ) @@ -151,6 +152,9 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta if v.Glusterfs != nil { warnings = append(warnings, fmt.Sprintf("%s: deprecated in v1.25, this feature will be removed soon after in a subsequent release", fieldPath.Child("spec", "volumes").Index(i).Child("glusterfs"))) } + if v.Ephemeral != nil && v.Ephemeral.VolumeClaimTemplate != nil { + warnings = append(warnings, pvcutil.GetWarningsForPersistentVolumeClaimSpec(fieldPath.Child("spec", "volumes").Index(i).Child("ephemeral").Child("volumeClaimTemplate").Child("spec"), v.Ephemeral.VolumeClaimTemplate.Spec)...) + } } // duplicate hostAliases (#91670, #58477) diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index 0b9bb00c915..6bab0367c69 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -459,6 +459,36 @@ func TestWarnings(t *testing.T) { }, expected: []string{}, }, + { + name: "pod with ephemeral volume source 200Mi", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: api.PodSpec{Volumes: []api.Volume{ + {Name: "ephemeral-volume", VolumeSource: api.VolumeSource{Ephemeral: &api.EphemeralVolumeSource{ + VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{ + Spec: api.PersistentVolumeClaimSpec{Resources: api.ResourceRequirements{ + Requests: api.ResourceList{api.ResourceStorage: resource.MustParse("200Mi")}}}, + }, + }}}}}, + }, + expected: []string{}, + }, + { + name: "pod with ephemeral volume source 200m", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: api.PodSpec{Volumes: []api.Volume{ + {Name: "ephemeral-volume", VolumeSource: api.VolumeSource{Ephemeral: &api.EphemeralVolumeSource{ + VolumeClaimTemplate: &api.PersistentVolumeClaimTemplate{ + Spec: api.PersistentVolumeClaimSpec{Resources: api.ResourceRequirements{ + Requests: api.ResourceList{api.ResourceStorage: resource.MustParse("200m")}}}, + }, + }}}}}, + }, + expected: []string{ + `spec.volumes[0].ephemeral.volumeClaimTemplate.spec.resources.requests[storage]: fractional byte value "200m" is invalid, must be an integer`, + }, + }, } for _, tc := range testcases { diff --git a/pkg/registry/apps/statefulset/strategy.go b/pkg/registry/apps/statefulset/strategy.go index 6d689bc5e14..7b63faa7513 100644 --- a/pkg/registry/apps/statefulset/strategy.go +++ b/pkg/registry/apps/statefulset/strategy.go @@ -26,6 +26,7 @@ import ( "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" + pvcutil "k8s.io/kubernetes/pkg/api/persistentvolumeclaim" "k8s.io/kubernetes/pkg/api/pod" "k8s.io/kubernetes/pkg/apis/apps" "k8s.io/kubernetes/pkg/apis/apps/validation" @@ -139,7 +140,11 @@ func (statefulSetStrategy) Validate(ctx context.Context, obj runtime.Object) fie // WarningsOnCreate returns warnings for the creation of the given object. func (statefulSetStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []string { newStatefulSet := obj.(*apps.StatefulSet) - return pod.GetWarningsForPodTemplate(ctx, field.NewPath("spec", "template"), &newStatefulSet.Spec.Template, nil) + warnings := pod.GetWarningsForPodTemplate(ctx, field.NewPath("spec", "template"), &newStatefulSet.Spec.Template, nil) + for i, pvc := range newStatefulSet.Spec.VolumeClaimTemplates { + warnings = append(warnings, pvcutil.GetWarningsForPersistentVolumeClaimSpec(field.NewPath("spec", "volumeClaimTemplates").Index(i), pvc.Spec)...) + } + return warnings } // Canonicalize normalizes the object after validation. @@ -168,6 +173,10 @@ func (statefulSetStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtim if newStatefulSet.Generation != oldStatefulSet.Generation { warnings = pod.GetWarningsForPodTemplate(ctx, field.NewPath("spec", "template"), &newStatefulSet.Spec.Template, &oldStatefulSet.Spec.Template) } + for i, pvc := range newStatefulSet.Spec.VolumeClaimTemplates { + warnings = append(warnings, pvcutil.GetWarningsForPersistentVolumeClaimSpec(field.NewPath("spec", "volumeClaimTemplates").Index(i).Child("Spec"), pvc.Spec)...) + } + return warnings } diff --git a/test/e2e/framework/.import-restrictions b/test/e2e/framework/.import-restrictions index be280ae1d91..8e171cee315 100644 --- a/test/e2e/framework/.import-restrictions +++ b/test/e2e/framework/.import-restrictions @@ -8,6 +8,7 @@ rules: - k8s.io/kubernetes/pkg/api/v1/service - k8s.io/kubernetes/pkg/api/pod - k8s.io/kubernetes/pkg/api/node + - k8s.io/kubernetes/pkg/api/persistentvolumeclaim - k8s.io/kubernetes/pkg/apis/apps - k8s.io/kubernetes/pkg/apis/apps/validation - k8s.io/kubernetes/pkg/apis/autoscaling