add pvc and emptyDir to function_volumes

[zalsader]

Changes

• add persistentVolumeClaim and emptyDir to the allowed function volumes
• deploy persistentVolumeClaim and emptyDir volumes
• add persistentVolumeClaim and emptyDir as options in config

/kind api-change

Fixes #1647

Release Note

Allow specifying `persistentVolumeClaim` and `emptyDir` as volumes in functions.

Docs

- [knative/docs]: TODO

[knative-prow]

Welcome @zalsader! It looks like this is your first PR to knative/func 🎉

[codecov]

Codecov Report

Patch coverage: 43.67% and project coverage change: +13.12 🎉

Comparison is base (88b3634) 49.81% compared to head (f89e51c) 62.94%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1666       +/-   ##
===========================================
+ Coverage   49.81%   62.94%   +13.12%     
===========================================
  Files          81       93       +12     
  Lines       11148    12183     +1035     
===========================================
+ Hits         5553     7668     +2115     
+ Misses       5111     3818     -1293     
- Partials      484      697      +213     

Flag	Coverage Δ
e2e-test	39.01% <27.01%> (?)
e2e-test-oncluster	34.45% <3.44%> (?)
e2e-test-oncluster-runtime	29.35% <0.00%> (?)
e2e-test-runtime-go	28.09% <3.44%> (?)
e2e-test-runtime-node	29.20% <3.44%> (?)
e2e-test-runtime-python	29.20% <3.44%> (?)
e2e-test-runtime-quarkus	29.34% <3.44%> (?)
e2e-test-runtime-springboot	28.21% <3.44%> (?)
e2e-test-runtime-typescript	29.34% <3.44%> (?)
integration-tests	49.78% <16.09%> (?)
unit-tests-macos-latest	48.67% <19.54%> (?)
unit-tests-ubuntu-latest	49.55% <19.54%> (?)
unit-tests-windows-latest	48.73% <19.54%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/functions/function.go	77.92% <0.00%> (+0.51%)	⬆️
pkg/functions/invoke.go	55.39% <0.00%> (+7.19%)	⬆️
pkg/knative/deployer.go	64.47% <17.30%> (+58.52%)	⬆️
cmd/config_volumes.go	64.84% <32.55%> (+37.37%)	⬆️
pkg/k8s/persistent_volumes.go	71.79% <59.37%> (+71.79%)	⬆️
pkg/functions/function_volumes.go	78.18% <75.55%> (-5.82%)	⬇️

... and 59 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[matejvasek]

@zalsader you need to re-generate schema make schema-generate and push it.

[zalsader]

@zalsader you need to re-generate schema make schema-generate and push it.

I did that. The command for make schema-generate was broken, so I added a fix.

[zalsader]

One thing is missing: We need to check that the features are enabled. Do you have suggestions on how to do that?

[matejvasek]

One thing is missing: We need to check that the features are enabled. Do you have suggestions on how to do that?

@zalsader what features do you mean?

[zalsader]

@zalsader what features do you mean?

Sorry I should have been more clear. The PVC and emptyDir are behind a feature flag.

[lkingland]

This looks very good

Just one small change: please move the validation logic func validateVolumes out of pkg/functions since in order to execute this (much appreciated) more advanced validation requires k8s.io/api/core/v1 and k8s.io/apimachinery/pkg/api/resource. We are keeping the core package functions dependency-free, so I think perhaps pkg/k8s would be the right place for this validator; taking its dependencies with it.

[lkingland]

@zalsader what features do you mean?

Sorry I should have been more clear. The PVC and emptyDir are behind a feature flag.

This is a good point, and actually a larger question, because there are other things I would like the functions client library to be able to inspect about the state of the configuration of the cluster, and interact with the user appropriately (or at least generate helpful error messages).

For now, I am fine with this PR being as-is, leaning on documentation, and perhaps issuing a warning to the user in the CLI that if they receive error X they need to enable PVC support.

I am going to put this on our agenda for this weeks meeting, and probably open a discussion.

[zalsader]

For now, I am fine with this PR being as-is, leaning on documentation, and perhaps issuing a warning to the user in the CLI that if they receive error X they need to enable PVC support.

I am going to put this on our agenda for this weeks meeting, and probably open a discussion.

When is the meeting, and would I be able to join? Where would be an appropriate place to issue this warning or write that documentation?

[lkingland]

For now, I am fine with this PR being as-is, leaning on documentation, and perhaps issuing a warning to the user in the CLI that if they receive error X they need to enable PVC support.
I am going to put this on our agenda for this weeks meeting, and probably open a discussion.

When is the meeting, and would I be able to join? Where would be an appropriate place to issue this warning or write that documentation?

We have a Knative Community Calendar which includes an open Functions Weekly Meeting on Tuesdays from 14:00-14:30 UTC, as well as many other meetings.

Until we have a way to query the connected platform to detect things like whether or not PVCs are enabled, I would issue this warning clearly from within the CLI package when a user successfully adds a PVC volume via the CLI; perhaps even with a link to the documentation of how to enable. Perhaps someone in the meeting will have a better suggestion as a stop-gap until we have a more permanent solution.

[lkingland]

Looks good to me @zalsader

I'll /lgtm this after giving you a chance to take a look at that .PHONY declaration, and perhaps add a helpful warning to the user about this feature requiring special features being installed in the cluster; if you can find a good spot to emit that from the cmd package.
👍🏻

[lkingland]

I am pretty sure that this is not a .PHONY, because it is actually intended to create the file schema/func_yaml-schema.json. Did you mean to perhaps write .PHONY: schema-generate?

[zalsader]

make build depends on $(CODE) which includes schema/func_yaml-schema.json. Since the file exists, make does not attempt to regenerate that file. This caused an issue later in the CI pipeline which runs make schema-generate.
Since there was a .PHONY: generate/zz_filesystem_generated.go, I thought it would be ok to do the same for the schema file. What do you think we should do instead?

[lance]

I think @lkingland is correct here. Modifying pkg/functions/function.go does indeed cause the schema to be regenerated via this make target:

func/Makefile

Lines 206 to 207 in d5b4a8d

	schema/func_yaml-schema.json: pkg/functions/function.go
	go run schema/generator/main.go

The schema-generate target should have been a no-op in CI unless the schema/func_yaml-schema.json changed during the build. This has not historically been a problem, so IMO this change is not needed. Otherwise all looks good!

[lkingland]

Since there was a .PHONY: generate/zz_filesystem_generated.go, I thought it would be ok to do the same for the schema file.

I see; thanks for noticing! I believe that .PHONY: generate/zz_filesystem_generated.go should actually be removed as well, but that's probably an update for another day/issue. The way it is being generated seems to me not entirely optimal, so it's probably not the best example.

Here's how I understand this rule to work:

schema/func_yaml-schema.json: pkg/functions/function.go 
        go run schema/generator/main.go

pkg/functions/function.go is the dependency. The rule will only execute if the target file either doesn't exist or is older than the dependency file. In this case, if function.go is newer than func_yaml-schema.json (has modifications), then the rule will be executed, generating the file again via go run.... This is the intended functionality.

I would like to know the problem you saw in CI. There actually might be something over there we need to fix or clean up! Other than this, this PR looks good to me

[zalsader]

I know what the issue is now; I added a commit. schema/func_yaml-schema.json should also depend on function_*.go

[lance]

Assuming tests pass, this is good to go. Thanks for your contribution @zalsader!

/lgtm

[lance]

@zalsader I neglected to notice that @lkingland had requested this change.

Just one small change: please move the validation logic func validateVolumes out of pkg/functions since in order to execute this (much appreciated) more advanced validation requires k8s.io/api/core/v1 and k8s.io/apimachinery/pkg/api/resource. We are keeping the core package functions dependency-free, so I think perhaps pkg/k8s would be the right place for this validator; taking its dependencies with it.

Would you mind please handling this as well? Thanks.

[zalsader]

Would you mind please handling this as well? Thanks.

I think it is already done since I removed the extra dependencies.

The additional dependencies were removed as requested

[lance]

/approve
/lgtm

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance, zalsader

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:

• OWNERS [lance]

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