Add pvc support #12458

skonto · 2021-12-23T15:43:05Z

Part of the work in #12438

Adds support for PVCs.
Introduces two feature flags as described in the related feature doc:

 kubernetes.podspec-persistent-volume-claim
 kubernetes.podspec-persistent-volume-write

Tested on minikube with minio with this trivial setup.

Release note

We're experimenting with PVC support behind the feature flags `kubernetes.podspec-persistent-volume-claim` `kubernetes.podspec-persistent-volume-write`

codecov · 2021-12-23T15:48:15Z

Codecov Report

Merging #12458 (da63dcb) into main (ee55bd0) will decrease coverage by 0.03%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main   #12458      +/-   ##
==========================================
- Coverage   87.46%   87.42%   -0.04%     
==========================================
  Files         195      195              
  Lines        9686     9720      +34     
==========================================
+ Hits         8472     8498      +26     
- Misses        930      937       +7     
- Partials      284      285       +1

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_defaults.go	`97.22% <0.00%> (ø)`
pkg/apis/serving/k8s_validation.go	`93.61% <84.84%> (-0.08%)`	⬇️
pkg/apis/config/features.go	`95.83% <100.00%> (+0.37%)`	⬆️
pkg/apis/serving/fieldmask.go	`95.13% <100.00%> (+0.06%)`	⬆️
pkg/autoscaler/statforwarder/forwarder.go	`88.88% <0.00%> (-7.41%)`	⬇️
pkg/autoscaler/scaling/multiscaler.go	`87.27% <0.00%> (-1.82%)`	⬇️
pkg/activator/net/revision_backends.go	`91.73% <0.00%> (-0.87%)`	⬇️
pkg/autoscaler/statforwarder/leases.go	`76.97% <0.00%> (+1.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee55bd0...da63dcb. Read the comment docs.

skonto · 2021-12-23T18:12:16Z

pkg/apis/serving/helpers/contexts.go

+limitations under the License.
+*/
+
+package helpers


need this otherwise we end up with a cycle deps issue.

skonto · 2021-12-27T09:06:41Z

/cc @julz @dprotaso

skonto · 2022-01-03T08:25:27Z

@julz @dprotaso happy new year! Gentle ping for review :)

julz · 2022-01-04T12:29:33Z

config/core/configmaps/features.yaml

+    # Controls whether persistent volumes support is enabled or not.
+    # 1. Enabled: enabling persistent volumes support
+    # 2. Disabled: disabling persistent volumes support
+    kubernetes.podspec-persistent-volumes-claims: "disabled"


Suggested change

kubernetes.podspec-persistent-volumes-claims: "disabled"

kubernetes.podspec-persistent-volume-claims: "disabled"

think this is singular in the spec

.. though then it wouldn't match podspec-persistent-volumes-write so maybe your way is better even if inconsistent. hm :(.

Yeah I thought too about the naming inconsistency. I dont have a better option.

I switched to:

kubernetes.podspec-persistent-volume-claim kubernetes.podspec-persistent-volume-write

julz · 2022-01-04T12:30:47Z

pkg/apis/serving/helpers/contexts.go

@@ -0,0 +1,42 @@
+/*
+Copyright 2021 The Knative Authors


Guess this is 2022 now!

Suggested change

Copyright 2021 The Knative Authors

Copyright 2022 The Knative Authors

julz · 2022-01-04T12:32:36Z

pkg/apis/serving/k8s_validation.go

-	minGroupID, maxGroupID = 0, math.MaxInt32
+	minUserID, maxUserID                     = 0, math.MaxInt32
+	minGroupID, maxGroupID                   = 0, math.MaxInt32
+	PodSpecPersistenceVolumesWriteAnnotation = "features.knative.dev/podspec-persistence-volumes-write"


I think this was "persistent" above

Suggested change

PodSpecPersistenceVolumesWriteAnnotation = "features.knative.dev/podspec-persistence-volumes-write"

PodSpecPersistentVolumesWriteAnnotation = "features.knative.dev/podspec-persistent-volumes-write"

Correct, will change.

julz · 2022-01-04T12:33:10Z

pkg/apis/serving/k8s_validation.go

@@ -109,6 +112,32 @@ func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes set
 	return volumes, errs
 }

+func shouldAllowPersistenVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features *config.Features) *apis.FieldError {


Suggested change

func shouldAllowPersistenVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features *config.Features) *apis.FieldError {

func shouldAllowPersistentVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features *config.Features) *apis.FieldError {

julz · 2022-01-04T12:34:37Z

pkg/apis/serving/k8s_validation.go

@@ -109,6 +112,32 @@ func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes set
 	return volumes, errs
 }

+func shouldAllowPersistenVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features *config.Features) *apis.FieldError {
+	var errs *apis.FieldError
+	if volume.PersistentVolumeClaim != nil {


could turn this in to a guard clause and exit immediately if nil, then the rest can be outdented

julz · 2022-01-04T12:36:03Z

pkg/apis/serving/k8s_validation.go

+		var isWriteAllowed bool
+		if features.PodSpecPersistentVolumesWrite == config.Enabled {
+			isWriteAllowed = true
+		}


Suggested change

var isWriteAllowed bool

if features.PodSpecPersistentVolumesWrite == config.Enabled {

isWriteAllowed = true

}

isWriteAllowed := features.PodSpecPersistentVolumesWrite == config.Enabled

julz · 2022-01-04T12:50:54Z

pkg/apis/serving/v1/configuration_validation.go

@@ -36,7 +37,7 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {
 		errs = errs.ViaField("metadata")

 		ctx = apis.WithinParent(ctx, c.ObjectMeta)
-		errs = errs.Also(c.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
+		errs = errs.Also(c.Spec.Validate(helpers.WithAnnotations(apis.WithinSpec(ctx), c.Annotations)).ViaField("spec"))


wonder if we could use apis.ParentMeta to avoid the extra helper pkg here

Let me try it, should work I think.

julz · 2022-01-04T12:55:25Z

pkg/apis/serving/k8s_validation_test.go

+			}},
+		cfgOpts: []configOption{withPodSpecPersistentVolumesClaimsEnabled(), withPodSpecPersistentVolumesWriteAllowed()},
+		want: &apis.FieldError{
+			Message: `Persistent volumes write support is off, but found Persistent Volume claim myclaim that is not read-only`,


fwiw I'm not sure I understand the point of having an annotation for this. IIUC the only case where a user should be able to set it to true is if the feature is "allowed", but if the feature is allowed why don't we just directly let them set the ReadOnly field to false without an annotation? Why would someone add ReadOnly: false and add the annotation as disabled (or, equivalently, not add it)?

My rationale is that from an admin perspective when deploying many apps you might want to have explicit control on what apps have write access and have an easy way to mark/list them so and restrict them per case. This is due to the implications that volume write access has for the serverless model. Of course you can always implement this externally to Knative via custom tooling, annotations etc so I agree using ReadOnly could be enough in that case. I can remove it. wdyt?

I removed the annotation if we find it useful can be added later on.

pkg/apis/serving/k8s_validation.go

rhuss · 2022-01-11T15:57:28Z

pkg/apis/serving/helpers/contexts.go

+import "context"
+
+type (
+	revCtxKey struct{}


wouldn't it be better to call it annoCtxKey ? Not sure how this helper is related to revisions only.

Removed that part.

rhuss · 2022-01-11T15:58:14Z

pkg/apis/serving/helpers/contexts.go

+)
+
+type revCtx struct {
+	anns map[string]string


would prefer annotations (or annos) but as its a private I leave it up to you

I removed that part as suggested by @julz .

skonto · 2022-01-11T16:37:51Z

@julz I removed the annotation and have now simpler name for the feature attributes, no plural.

rhuss · 2022-01-12T08:29:30Z

pkg/apis/serving/k8s_validation.go

+		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volumes claims support is disabled, "+
+			"but found Persistent Volume Claim %s", volume.PersistentVolumeClaim.ClaimName)})


Another nit pick, sorry ...

Suggested change

errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volumes claims support is disabled, "+

"but found Persistent Volume Claim %s", volume.PersistentVolumeClaim.ClaimName)})

errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volume claim support is disabled, "+

"but found persistent volume claim %s", volume.PersistentVolumeClaim.ClaimName)})

(the capitalization should be consistent, I would prefer to user lower case for 'persistent volume claim' and camel-case for PersistenceVolumeClaim)

@rhuss Here you suggested the opposite no? I changed it due to that comment. I guess this is an update to that comment?

Btw Persistent Volume Claim refers to the K8s api attribute. Persistent volumes claims support refers to the feature of supporting this type of volumes. Anyway I can change. Btw there is no PersistenceVolumeClaim.

@rhuss Here you suggested the opposite no? I changed it due to that comment. I guess this is an update to that comment?

Yep, for me, it's important to be consistent (it doesn't make sense to reference the K8s API attribute (which btw IMO is PersistentVolumeClaim and the feature in the error message (actually the feature is disabled, not the API attribute).

Also (as @julz mentioned) persistent volumes claims sounds incorrect with the double plural (regardless of the arguments above)

skonto · 2022-01-12T10:25:35Z

   image_pull_error_test.go:86: Failed to validate revision state: the Revision image-pull-error-cerabxvl-00001 ReadyCondition = (Reason="Unschedulable", Message="0/2 nodes are available: 1 Insufficient cpu, 1 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate."), wantReasons: [ImagePullBackOff ErrImagePull]

skonto · 2022-01-12T10:25:50Z

/retest

skonto · 2022-01-13T11:31:14Z

@dprotaso @julz gentle ping :)

julz

nits

julz · 2022-01-13T13:56:24Z

pkg/apis/serving/k8s_validation.go

@@ -109,6 +110,24 @@ func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes set
 	return volumes, errs
 }

+func shouldAllowPersistentVolumeClaims(volume corev1.VolumeSource, features *config.Features) *apis.FieldError {


bit of a nit, but given I think this should probably be validatePersistentVolumeClaims since it's not a boolean check / returns apis.FieldError

julz · 2022-01-13T13:57:37Z

pkg/apis/serving/k8s_validation.go

 	if len(specified) == 0 {
 		fieldPaths := []string{"secret", "configMap", "projected"}
 		cfg := config.FromContextOrDefaults(ctx)
 		if cfg.Features.PodSpecVolumesEmptyDir == config.Enabled {
 			fieldPaths = append(fieldPaths, "emptyDir")
 		}
+		if cfg.Features.PodSpecPersistentVolumeClaim == config.Enabled {
+			fieldPaths = append(fieldPaths, "persistenVolumeClaim")


Suggested change

fieldPaths = append(fieldPaths, "persistenVolumeClaim")

fieldPaths = append(fieldPaths, "persistentVolumeClaim")

Persistent typo:) My bad will fix.

julz · 2022-01-13T14:01:45Z

pkg/apis/serving/k8s_validation.go

@@ -142,12 +161,20 @@ func validateVolume(ctx context.Context, volume corev1.Volume) *apis.FieldError
 		specified = append(specified, "emptyDir")
 		errs = errs.Also(validateEmptyDirFields(vs.EmptyDir).ViaField("emptyDir"))
 	}
+
+	if vs.PersistentVolumeClaim != nil {
+		specified = append(specified, "persistentVolumeClaim")


I'm wondering if we should call shouldAllowPersistentVolumeClaims->validatePersistentVolumeClaims here (rather than in ValidateVolumes) to be consistent with the other volume types above

Yeah makes sense will switch to that.

did you miss this one?

@julz this part is about validating the volume itself wrt to K8s reqs among others. On the other hand, validatePersistentVolumeClaims is related to whether we should allow the specific type of volume wrt to the related Serving feature. That is why I used the name shouldAllowPersistentVolumeClaims similarly to what we have for emptydir.
However in general we could move this code part

serving/pkg/apis/serving/k8s_validation.go

Lines 90 to 106 in 165d516

if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {

errs = errs.Also((&apis.FieldError{Message: fmt.Sprintf("EmptyDir volume support is disabled, "+

"but found EmptyDir volume %s", volume.Name)}).ViaIndex(i))

}

errs = errs.Also(validatePersistentVolumeClaims(volume.VolumeSource, features).ViaIndex(i))

if _, ok := volumes[volume.Name]; ok {

errs = errs.Also((&apis.FieldError{

Message: fmt.Sprintf("duplicate volume name %q", volume.Name),

Paths: []string{"name"},

}).ViaIndex(i))

}

if !mountedVolumes.Has(volume.Name) {

errs = errs.Also((&apis.FieldError{

Message: fmt.Sprintf("volume with name %q not mounted", volume.Name),

Paths: []string{"name"},

}).ViaIndex(i))

}

in validateVolume. It does not make sense to do it only for pvcs because the same concept applies for emptydir. Personally I find this ok as is and the name validatePersistentVolumeClaims a bit misleading in terms of what we are validating (k8s specs va something else). Should I move the whole part?

moved emptyDir & pvc related checks to the volumeValidate func

skonto · 2022-01-13T18:58:03Z

    image_pull_error_test.go:86: Failed to validate revision state: the Revision image-pull-error-jiiucylu-00001 ReadyCondition = (Reason="Unschedulable", Message="0/2 nodes are available: 1 Insufficient cpu, 1 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate."), wantReasons: [ImagePullBackOff ErrImagePull] ```

skonto · 2022-01-13T18:58:12Z

/retest

skonto · 2022-01-14T09:09:02Z

@julz ready. :)

skonto · 2022-01-17T09:50:30Z

@julz gentle ping.

skonto · 2022-01-18T08:55:13Z

@dprotaso @julz gentle ping.

skonto · 2022-01-18T22:35:48Z

@julz I addressed the last pending issue, pls review.

skonto · 2022-01-19T08:53:44Z

/retest

julz

/lgtm
/approve

knative-prow-robot · 2022-01-19T12:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: julz, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~config/OWNERS~~ [julz]
~~pkg/apis/OWNERS~~ [julz]
~~pkg/reconciler/OWNERS~~ [julz]
~~pkg/testing/OWNERS~~ [julz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* add pvc support * fix header * fix fmt * simplify implementation * fixes * fix msg * improve test names * nits * more fixes

* Add pvc support (knative#12458) * add pvc support * fix header * fix fmt * simplify implementation * fixes * fix msg * improve test names * nits * more fixes * Add pvc e2e test (knative#12547) * add pvc e2e test * fmt * fix comment * fix pvc ns * add fsGroup * add flag * fix typo * fixes * fix style * skip pvc setup in upgrade tests * Rename test image to volumes from emptydir (#1052) * fix volumes path & dockerfile * run pvc test * enable alpha Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>

knative-prow-robot added area/API API objects and controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Dec 23, 2021

skonto changed the title ~~Add pvc support~~ [wip] Add pvc support Dec 23, 2021

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2021

skonto commented Dec 23, 2021

View reviewed changes

skonto changed the title ~~[wip] Add pvc support~~ Add pvc support Dec 27, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2021

knative-prow-robot requested review from dprotaso and julz December 27, 2021 09:06

Phelan164 mentioned this pull request Dec 30, 2021

Set shm-size for Triton in yaml kserve/kserve#1937

Closed

julz reviewed Jan 4, 2022

View reviewed changes

rhuss reviewed Jan 11, 2022

View reviewed changes

pkg/apis/serving/k8s_validation.go Outdated Show resolved Hide resolved

rhuss reviewed Jan 11, 2022

View reviewed changes

skonto force-pushed the add_support_pvc branch from 21091ce to 4e96597 Compare January 11, 2022 16:36

rhuss reviewed Jan 12, 2022

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2022

skonto added 5 commits January 13, 2022 11:43

add pvc support

35a34d5

fix header

a1e3220

fix fmt

aac83f3

simplify implementation

ec4128a

fixes

d298e67

skonto added 2 commits January 13, 2022 11:46

fix msg

beea962

improve test names

772a48c

skonto force-pushed the add_support_pvc branch from 238248f to 772a48c Compare January 13, 2022 09:56

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2022

julz reviewed Jan 13, 2022

View reviewed changes

nits

165d516

more fixes

da63dcb

julz approved these changes Jan 19, 2022

View reviewed changes

knative-prow-robot assigned julz Jan 19, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2022

knative-prow-robot merged commit 0682e94 into knative:main Jan 19, 2022

dprotaso added this to the v1.2.0 milestone Jan 26, 2022

skonto added a commit to skonto/serving that referenced this pull request Mar 10, 2022

Add pvc support (knative#12458)

3a8cb21

* add pvc support * fix header * fix fmt * simplify implementation * fixes * fix msg * improve test names * nits * more fixes

skonto added a commit to skonto/serving that referenced this pull request Mar 11, 2022

Add pvc support (knative#12458)

16047e2

* add pvc support * fix header * fix fmt * simplify implementation * fixes * fix msg * improve test names * nits * more fixes

	kubernetes.podspec-persistent-volumes-claims: "disabled"
	kubernetes.podspec-persistent-volume-claims: "disabled"

	Copyright 2021 The Knative Authors
	Copyright 2022 The Knative Authors

	PodSpecPersistenceVolumesWriteAnnotation = "features.knative.dev/podspec-persistence-volumes-write"
	PodSpecPersistentVolumesWriteAnnotation = "features.knative.dev/podspec-persistent-volumes-write"

	func shouldAllowPersistenVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features config.Features) apis.FieldError {
	func shouldAllowPersistentVolumeClaims(ctx context.Context, volume corev1.VolumeSource, features config.Features) apis.FieldError {

		errs = errs.Also(&apis.FieldError{Message: fmt.Sprintf("Persistent volumes claims support is disabled, "+
		"but found Persistent Volume Claim %s", volume.PersistentVolumeClaim.ClaimName)})

	fieldPaths = append(fieldPaths, "persistenVolumeClaim")
	fieldPaths = append(fieldPaths, "persistentVolumeClaim")

	if volume.EmptyDir != nil && features.PodSpecVolumesEmptyDir != config.Enabled {
	errs = errs.Also((&apis.FieldError{Message: fmt.Sprintf("EmptyDir volume support is disabled, "+
	"but found EmptyDir volume %s", volume.Name)}).ViaIndex(i))
	}
	errs = errs.Also(validatePersistentVolumeClaims(volume.VolumeSource, features).ViaIndex(i))
	if _, ok := volumes[volume.Name]; ok {
	errs = errs.Also((&apis.FieldError{
	Message: fmt.Sprintf("duplicate volume name %q", volume.Name),
	Paths: []string{"name"},
	}).ViaIndex(i))
	}
	if !mountedVolumes.Has(volume.Name) {
	errs = errs.Also((&apis.FieldError{
	Message: fmt.Sprintf("volume with name %q not mounted", volume.Name),
	Paths: []string{"name"},
	}).ViaIndex(i))
	}

Add pvc support #12458

Add pvc support #12458

Conversation

skonto commented Dec 23, 2021 • edited by dprotaso Loading

Release note

codecov bot commented Dec 23, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

skonto commented Dec 27, 2021

skonto commented Jan 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Jan 11, 2022

Choose a reason for hiding this comment

skonto Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

skonto Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Jan 12, 2022

skonto commented Jan 12, 2022

skonto commented Jan 13, 2022

julz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

skonto Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

skonto commented Jan 13, 2022

skonto commented Jan 13, 2022

skonto commented Jan 14, 2022 • edited Loading

skonto commented Jan 17, 2022

skonto commented Jan 18, 2022 • edited Loading

skonto commented Jan 18, 2022

skonto commented Jan 19, 2022

julz left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 19, 2022

skonto commented Dec 23, 2021 •

edited by dprotaso

Loading

codecov bot commented Dec 23, 2021 •

edited

Loading

skonto Jan 11, 2022 •

edited

Loading

skonto Jan 11, 2022 •

edited

Loading

skonto Jan 12, 2022 •

edited

Loading

skonto Jan 12, 2022 •

edited

Loading

skonto Jan 13, 2022 •

edited

Loading

skonto Jan 18, 2022 •

edited

Loading

skonto Jan 18, 2022 •

edited

Loading

skonto commented Jan 14, 2022 •

edited

Loading

skonto commented Jan 18, 2022 •

edited

Loading