generic ephemeral volume GA #105609

pohly · 2021-10-11T15:29:21Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Generic ephemeral volume has met the GA criteria and is meant to graduate to GA in 1.23.

Special notes for your reviewer:

This contains two other minor PRs that were still pending when creating this PR:

Some other locations where the helper functions could be used were found when removing the feature gate.

Does this PR introduce a user-facing change?

The "Generic Ephemeral Volume" feature graduates to GA. It is now enabled unconditionally.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/e669a1483064f4a3f248e78c9b8399858ca75f66/keps/sig-storage/1698-generic-ephemeral-volumes/README.md

The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them.

k8s-triage-robot · 2021-10-11T16:22:32Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

xing-yang · 2021-10-11T16:35:09Z

/assign @xing-yang

sftim · 2021-10-20T08:24:58Z

@pohly in the PR description, I think the hyperlink to the KEP is incorrect.

pohly · 2021-10-20T09:53:37Z

in the PR description, I think the hyperlink to the KEP is incorrect.

@sftim: What is the correct link? The template only says [KEP]: <link>, without going into details whether that should be the readme, the directory, or the enhancement issue.

pohly · 2021-10-22T16:20:09Z

The repo was wrong (pohly vs, kubernetes). Fixed.

mauriciopoppe · 2021-10-27T17:31:30Z

pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go

-				pvcSource.ClaimName,
-			)
+		if isEphemeral {
+			if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil {


The condition to throw an error was changed from:

ephemeral && !metav1.IsControlledBy(pvc, pod)

to

ephemeral && (pvc.Namespace != pod.Namespace || !metav1.IsControlledBy(pvc, pod))

I guess this is ok because the caller shouldn't pass a PVC that isn't from the same namespace as the pod as you pointed out

Correct. ephemeral.VolumeIsForPod is more thorough because it's a general-purpose function that doesn't know whether the namespaces match. The semantic is the same.

mauriciopoppe · 2021-10-27T17:34:41Z

pkg/registry/policy/podsecuritypolicy/strategy.go

-		AllowEphemeralVolumeType: utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) ||
-			volumeInUse(old.(*policy.PodSecurityPolicy), policy.Ephemeral),
-	}
+	opts := validation.PodSecurityPolicyValidationOptions{}


This became:

AllowEphemeralVolumeType: true || volumeInUse(old.(*policy.PodSecurityPolicy), policy.Ephemeral)

So it makes sense that AllowEphemeralVolumeType is removed

xing-yang · 2021-10-28T00:11:57Z

pkg/apis/policy/validation/validation.go

@@ -94,9 +94,6 @@ var ValidatePodSecurityPolicyName = apimachineryvalidation.NameIsDNSSubdomain

 // PodSecurityPolicyValidationOptions contains additional parameters for ValidatePodSecurityPolicy.
 type PodSecurityPolicyValidationOptions struct {
-	// AllowEphemeralVolumeType determines whether Ephemeral is a valid entry
-	// in PodSecurityPolicySpec.Volumes.
-	AllowEphemeralVolumeType bool
 }


If PodSecurityPolicyValidationOptions is always empty, is this still needed?

Not now, but it may be needed again in the future. Therefore I kept it. Not removing it also avoids additional churn due to function prototype changes.

I've pushed a commit which removes the struct. It's intentionally a separate commit to support reverting.

xing-yang · 2021-10-28T14:57:13Z

/milestone v1.23

Since the only member of that struct is gone, the struct itself can also be removed. If for whatever reason the struct is needed again, then this commit can be reverted to bring it back.

xing-yang · 2021-10-28T17:57:34Z

/lgtm

xing-yang · 2021-10-28T17:58:09Z

/assign @thockin

pohly · 2021-10-28T19:39:43Z

Can we also verify that all our storage_operation metrics are handling pvc ephemeral volumes appropriately?
...
We have e2e tests here: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/volume_metrics.go.

I updated those tests. They now run as "Volume Metrics Ephemeral" in https://testgrid.kubernetes.io/sig-storage-kubernetes#gce-serial. The recent test failures in that job are unrelated (#105965).

thockin

/approve

k8s-ci-robot · 2021-10-28T22:52:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, thockin

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:

~~api/OWNERS~~ [thockin]
~~cmd/kube-controller-manager/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/controller/volume/OWNERS~~ [thockin]
~~pkg/features/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [thockin]
~~pkg/registry/OWNERS~~ [thockin]
~~pkg/scheduler/OWNERS~~ [thockin]
~~pkg/security/podsecuritypolicy/OWNERS~~ [thockin]
~~plugin/pkg/admission/security/podsecuritypolicy/OWNERS~~ [thockin]
~~plugin/pkg/auth/authorizer/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~test/e2e/storage/OWNERS~~ [pohly,thockin]

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

k8s-ci-robot requested review from andrewsykim and deads2k October 11, 2021 15:30

pohly added 2 commits October 11, 2021 17:33

auth: use generic ephemeral volume helper functions

466dcdf

The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them.

scheduler: use generic ephemeral volume helper functions

bc263f3

The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them.

pohly force-pushed the generic-ephemeral-volume-ga branch from 1dc06f8 to 23f29fe Compare October 11, 2021 15:34

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 11, 2021

pohly changed the title ~~generic ephemeral volume GA~~ WIP: generic ephemeral volume GA Oct 11, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2021

k8s-ci-robot assigned xing-yang Oct 11, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 14, 2021

mauriciopoppe reviewed Oct 27, 2021

View reviewed changes

xing-yang reviewed Oct 28, 2021

View reviewed changes

k8s-ci-robot added this to the v1.23 milestone Oct 28, 2021

psp: remove unused PodSecurityPolicyValidationOptions

d55f7c8

Since the only member of that struct is gone, the struct itself can also be removed. If for whatever reason the struct is needed again, then this commit can be reverted to bring it back.

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2021

k8s-ci-robot assigned thockin Oct 28, 2021

thockin reviewed Oct 28, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021

k8s-ci-robot merged commit c592bd4 into kubernetes:master Oct 29, 2021

torredil mentioned this pull request Oct 21, 2022

EBS Usage Metrics are not being emitted for some pods kubernetes-sigs/aws-ebs-csi-driver#1432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic ephemeral volume GA #105609

generic ephemeral volume GA #105609

pohly commented Oct 11, 2021 •

edited

Loading

k8s-triage-robot commented Oct 11, 2021

xing-yang commented Oct 11, 2021

sftim commented Oct 20, 2021

pohly commented Oct 20, 2021 •

edited

Loading

pohly commented Oct 22, 2021

mauriciopoppe Oct 27, 2021 •

edited

Loading

pohly Oct 27, 2021

mauriciopoppe Oct 27, 2021

xing-yang Oct 28, 2021

pohly Oct 28, 2021 •

edited

Loading

pohly Oct 28, 2021

xing-yang commented Oct 28, 2021

xing-yang commented Oct 28, 2021

xing-yang commented Oct 28, 2021

pohly commented Oct 28, 2021

thockin left a comment

k8s-ci-robot commented Oct 28, 2021

generic ephemeral volume GA #105609

generic ephemeral volume GA #105609

Conversation

pohly commented Oct 11, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-triage-robot commented Oct 11, 2021

xing-yang commented Oct 11, 2021

sftim commented Oct 20, 2021

pohly commented Oct 20, 2021 • edited Loading

pohly commented Oct 22, 2021

mauriciopoppe Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

pohly Oct 27, 2021

Choose a reason for hiding this comment

mauriciopoppe Oct 27, 2021

Choose a reason for hiding this comment

xing-yang Oct 28, 2021

Choose a reason for hiding this comment

pohly Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

pohly Oct 28, 2021

Choose a reason for hiding this comment

xing-yang commented Oct 28, 2021

xing-yang commented Oct 28, 2021

xing-yang commented Oct 28, 2021

pohly commented Oct 28, 2021

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 28, 2021

pohly commented Oct 11, 2021 •

edited

Loading

pohly commented Oct 20, 2021 •

edited

Loading

mauriciopoppe Oct 27, 2021 •

edited

Loading

pohly Oct 28, 2021 •

edited

Loading