feat: add support for generic ephemeral volumes#16590
Conversation
|
|
|
Welcome @jbunting! It looks like this is your first PR to knative/serving 🎉 |
|
Hi @jbunting. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Thanks for this PR, looks great from my side but a maintainer has to give it another look :) /ok-to-test |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16590 +/- ##
==========================================
- Coverage 80.21% 80.20% -0.01%
==========================================
Files 217 217
Lines 13545 13556 +11
==========================================
+ Hits 10865 10873 +8
Misses 2318 2318
- Partials 362 365 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| FeaturePodSpecShareProcessNamespace = "kubernetes.podspec-shareprocessnamespace" | ||
| FeaturePodSpecTolerations = "kubernetes.podspec-tolerations" | ||
| FeaturePodSpecTopologySpreadConstraints = "kubernetes.podspec-topologyspreadconstraints" | ||
| FeaturePodSpecVolumesEphemeral = "kubernetes.podspec-volumes-ephemeral" |
There was a problem hiding this comment.
We should probably also add a test for this new feature as there are tests in features_test.go for the other Volumes
There was a problem hiding this comment.
Good point, I'll add that.
| ephemeral: | ||
| description: |- | ||
| This is accessible behind a feature flag - kubernetes.podspec-volumes-ephemeral | ||
| type: object | ||
| x-kubernetes-preserve-unknown-fields: true |
There was a problem hiding this comment.
I just learned that we should extend the following code to get these changes instead of manually applying them:
serving/cmd/schema-tweak/overrides.go
Lines 251 to 257 in fb213e0
(CI failed for this reason)
There was a problem hiding this comment.
Ah, I missed that. I'll get that updated.
There was a problem hiding this comment.
I've added this and the related test is passing now.
7f2b2c7 to
d0ccf6c
Compare
|
/retest |
Add a new feature flag `kubernetes.podspec-volumes-ephemeral` to allow generic ephemeral volumes (volumeClaimTemplate) in Knative services. This enables workloads that need per-pod dynamically provisioned storage, such as local NVMe scratch space on cloud VMs. Also exempt ephemeral volumes from the forced readOnly default, matching the existing behavior for emptyDir and PersistentVolumeClaim volumes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d0ccf6c to
1295f8e
Compare
|
/test all |
linkvt
left a comment
There was a problem hiding this comment.
/lgtm but I'm not a maintainer who still has to review and approve
Thanks for looking at it! Is there anything else I can do to make it easier for a maintainer to look at? |
|
Not right now, it's not a completely new topic (similar logic exists for the other volume types already) so it's just about when he's finding time. |
|
/lgtm thanks for the contribution What are your thoughts about allowing ephemeral volumes without the feature flag? Generally we were cautious of volumes in the past to avoid people shooting themselves in the foot - eg. NFS volumes not handling load well when things scale up. But it's not obvious to me if that's possible with this ephemeral volumes - seems akin to emptyDir which we allow. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, jbunting, linkvt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I'm far from an expert but I'm pretty sure that's accurate -- ephemeral volumes are pretty much akin to That being said, I think not having it behind a feature flag makes plenty of sense. I included the feature flag because I figured following the existing pattern would be the smoothest way to contribute. |
|
Thanks for the reply
I say let's use the feature flag for a release or two and revisit the decision |
Relates to #4417
Description
This feature introduces support for generic ephemeral volumes (volumeClaimTemplate) in Knative Serving, enabling users to dynamically provision per-pod storage through any Kubernetes StorageClass. This is particularly useful for workloads that need local high-performance scratch space, such as NVMe storage on cloud VMs provisioned via a local StorageClass, while retaining Knative's autoscaling capabilities.
Proposed Changes
Release Note