Add support for emptydir rw access and e2e testing #11705

skonto · 2021-07-21T13:12:45Z

Part of the work in #11664.

adds an e2e test that serves data in an emptyDir volume. Test run here. It will be run along with the rest when the feature is on by default.
allows rw access to emptyDir volumes only (extends logic to allow more types of volumes eg. pvc in the future).

/cc @julz @dprotaso @markusthoemmes @nak3

codecov · 2021-07-21T13:17:34Z

Codecov Report

Merging #11705 (3b17ad7) into main (11abffd) will increase coverage by 0.02%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main   #11705      +/-   ##
==========================================
+ Coverage   87.80%   87.83%   +0.02%     
==========================================
  Files         190      190              
  Lines        9275     9279       +4     
==========================================
+ Hits         8144     8150       +6     
  Misses        878      878              
+ Partials      253      251       -2

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_defaults.go	`97.05% <60.00%> (-2.95%)`	⬇️
pkg/apis/serving/k8s_validation.go	`96.12% <100.00%> (ø)`
pkg/autoscaler/statforwarder/leases.go	`78.41% <0.00%> (+1.43%)`	⬆️
pkg/reconciler/configuration/configuration.go	`86.15% <0.00%> (+1.53%)`	⬆️

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 11abffd...3b17ad7. Read the comment docs.

skonto · 2021-07-21T18:27:15Z

/retest

skonto · 2021-07-22T07:39:14Z

/assign @dprotaso

julz · 2021-07-22T10:11:42Z

pkg/apis/serving/k8s_validation.go

@@ -105,8 +106,11 @@ func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes set
 		}
 		errs = errs.Also(validateVolume(ctx, volume).ViaIndex(i))
 		volumes.Insert(volume.Name)
+		if volume.EmptyDir != nil {
+			nonReadOnlyVolumes.Insert(volume.Name)


the variable is called nonReadOnlyVolumes, but it seems like it actually contains nonEmptyDirVolumes. When we actually use it we end up negating it again (!nonReadOnlyValumes.Has(..)) which is a lot of negations for my mind to follow and confused me a bit: maybe this could be emptyDirVolumes or allowedReadWriteVolumes or something like that?

(see suggested alternative approach in 3rd comment though which makes this comment and the next redundant if it works!)

I can change the name but the var contains only emptyDirs as they are the only ones which are supported and can have rw access. At least this is the intention here.

but it seems like it actually contains nonEmptyDirVolumes

Don't parse this part. Could you explain a bit more?

julz · 2021-07-22T10:12:30Z

pkg/apis/serving/k8s_validation.go

@@ -82,8 +82,9 @@ var (
 )

 // ValidateVolumes validates the Volumes of a PodSpec.
-func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes sets.String) (sets.String, *apis.FieldError) {
+func ValidateVolumes(ctx context.Context, vs []corev1.Volume, mountedVolumes sets.String) (sets.String, sets.String, *apis.FieldError) {


since we have two sets.String in the return section now I'd probably add variable names for them and maybe add to the method comment to indicate what each is

julz · 2021-07-22T10:31:04Z

pkg/apis/serving/k8s_validation.go

 	volumes := make(sets.String, len(vs))
+	nonReadOnlyVolumes := make(sets.String)


thinking out loud: a nice alternative approach might be to make the return value of this method a map[string]corev1.Volume (rather than two sets.Strings each with different semantics, which is maybe a bit unwieldy and slightly hard to document). The volume.EmptyDir != nil in this method would then not be needed at all, and in validateVolumeMounts you could do: if volumes[vm.Name].EmptyDir != nil && !vm.ReadOnly (which means the logic would only be in only one place).

Yeah I also thought of returning volumes instead or some data structure but didnt want to change the signature for existing code (less changes).

Right but we have to change the signature anyway since we pass two sets.Strings around everywhere now?

Yes but didnt touch some other parts though eg. accessing the first string set stayed the same. Anyway I can take a look and refactor it ;)

Heheheh yeah, worth a go anyway I think since it'd move all the logic to one place and avoid having two pass two things up and down the stack

julz · 2021-07-22T10:44:47Z

pkg/apis/serving/k8s_validation_test.go

@@ -1509,7 +1509,7 @@ func TestContainerValidation(t *testing.T) {
 				ctx = config.ToContext(ctx, cfg)
 			}

-			got := ValidateContainer(ctx, test.c, test.volumes)
+			got := ValidateContainer(ctx, test.c, test.volumes, make(sets.String))


probably worth a unit test for the new behaviour here too I think

Yes we can cover more cases there by adding a test case field.

julz

nice! lgtm bar a formatting nit in the tests and I think it's probably worth logging the errors in the test image in this case

julz · 2021-07-23T10:46:32Z

pkg/apis/serving/k8s_validation_test.go

+		volumes: map[string]corev1.Volume{
+			"the-name": {
+				Name: "the-name",
+				VolumeSource: corev1.VolumeSource{
+					ConfigMap: &corev1.ConfigMapVolumeSource{
+						LocalObjectReference: corev1.LocalObjectReference{
+							Name: "test-cm",
+						}},
+				},
+			}},


total nit, but I think for consistency the formatting here should be:

Suggested change

volumes: map[string]corev1.Volume{

"the-name": {

Name: "the-name",

VolumeSource: corev1.VolumeSource{

ConfigMap: &corev1.ConfigMapVolumeSource{

LocalObjectReference: corev1.LocalObjectReference{

Name: "test-cm",

}},

},

}},

volumes: map[string]corev1.Volume{

"the-name": {

Name: "the-name",

VolumeSource: corev1.VolumeSource{

ConfigMap: &corev1.ConfigMapVolumeSource{

LocalObjectReference: corev1.LocalObjectReference{

Name: "test-cm",

},

},

},

},

},

(basically line the braces up under the first column of where the opening brace is)

julz · 2021-07-23T10:50:11Z

test/e2e/emptydir_test.go

+	})
+
+	withVolume2 := WithVolume("cache", "/cache", corev1.VolumeSource{
+		EmptyDir: &corev1.EmptyDirVolumeSource{},


I dont think we actually try reading/writing to this, but I guess we just want to test the validation allows this since we tested read/write on the other volume?

Yes to show that we can have multiple ones.

julz · 2021-07-23T10:51:30Z

test/test_images/emptydir/emptydir.go

+	if path == "" {
+		path = "/data"
+	}
+	f, _ := os.OpenFile(path+"/testfile", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)


worth logging the error here, since it may help with debugging if e.g. the volume was mounted read-only?

Ok let me add that

skonto · 2021-07-23T11:35:16Z

/assign @julz

julz

/lgtm
/approve

nice, thanks @skonto!

knative-prow-robot · 2021-07-23T11:40:40Z

[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:

~~pkg/apis/OWNERS~~ [julz]
~~test/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 support for emptydir rw access, e2e * fix header * fix quantity * set to alpha feature * toggle feature * fixes * fix fmt * fix nit

* Add support for emptyDir volume type (knative#11669) * support emptyDir volume * fixes * reorder feature * Add support for emptydir rw access and e2e testing (knative#11705) * add support for emptydir rw access, e2e * fix header * fix quantity * set to alpha feature * toggle feature * fixes * fix fmt * fix nit Co-authored-by: Stavros Kontopoulos <[email protected]>

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 21, 2021

skonto changed the title ~~Add support for emptydir rw access and e2e testing~~ [wip] Add support for emptydir rw access and e2e testing Jul 21, 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 Jul 21, 2021

skonto added 2 commits July 21, 2021 16:20

add support for emptydir rw access, e2e

c262cac

fix header

adfe004

skonto force-pushed the add_e2e_empty_dir branch from 6b30d5d to adfe004 Compare July 21, 2021 13:21

skonto added 3 commits July 21, 2021 16:48

fix quantity

694cf32

set to alpha feature

0b4ceaf

toggle feature

5a058f6

skonto changed the title ~~[wip] Add support for emptydir rw access and e2e testing~~ Add support for emptydir rw access and e2e testing Jul 21, 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 Jul 21, 2021

skonto changed the title ~~Add support for emptydir rw access and e2e testing~~ [WIP] Add support for emptydir rw access and e2e testing Jul 21, 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 Jul 21, 2021

skonto changed the title ~~[WIP] Add support for emptydir rw access and e2e testing~~ Add support for emptydir rw access and e2e testing Jul 22, 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 Jul 22, 2021

knative-prow-robot assigned dprotaso Jul 22, 2021

julz reviewed Jul 22, 2021

View reviewed changes

skonto added 2 commits July 23, 2021 13:12

fixes

c04dfe6

fix fmt

7f285c6

julz reviewed Jul 23, 2021

View reviewed changes

fix nit

3b17ad7

knative-prow-robot assigned julz Jul 23, 2021

julz approved these changes Jul 23, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2021

knative-prow-robot merged commit 6447982 into knative:main Jul 23, 2021

skonto mentioned this pull request Oct 13, 2021

Add init containers e2e test #12123

Merged

skonto mentioned this pull request Jan 3, 2022

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

Closed

boudo mentioned this pull request Nov 8, 2022

Feature Request: Relax volume constraint to support more volume types #4417

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for emptydir rw access and e2e testing #11705

Add support for emptydir rw access and e2e testing #11705

skonto commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 21, 2021 •

edited

Loading

skonto commented Jul 21, 2021

skonto commented Jul 22, 2021

julz Jul 22, 2021

julz Jul 22, 2021

skonto Jul 22, 2021 •

edited

Loading

julz Jul 22, 2021

skonto Jul 22, 2021

julz Jul 22, 2021 •

edited

Loading

skonto Jul 22, 2021 •

edited

Loading

julz Jul 22, 2021

skonto Jul 22, 2021 •

edited

Loading

julz Jul 22, 2021

julz Jul 22, 2021

skonto Jul 22, 2021

julz left a comment

julz Jul 23, 2021

skonto Jul 23, 2021

julz Jul 23, 2021

skonto Jul 23, 2021

julz Jul 23, 2021

skonto Jul 23, 2021

skonto commented Jul 23, 2021

julz left a comment

knative-prow-robot commented Jul 23, 2021

		volumes := make(sets.String, len(vs))
		nonReadOnlyVolumes := make(sets.String)

Add support for emptydir rw access and e2e testing #11705

Add support for emptydir rw access and e2e testing #11705

Conversation

skonto commented Jul 21, 2021 • edited Loading

codecov bot commented Jul 21, 2021 • edited Loading

Codecov Report

skonto commented Jul 21, 2021

skonto commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julz Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

skonto Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Jul 22, 2021 • 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

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

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 Jul 23, 2021

julz left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 23, 2021

skonto commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 21, 2021 •

edited

Loading

skonto Jul 22, 2021 •

edited

Loading

julz Jul 22, 2021 •

edited

Loading

skonto Jul 22, 2021 •

edited

Loading

skonto Jul 22, 2021 •

edited

Loading