storage validation: accept generic ephemeral volumes as volume device #105682

pohly · 2021-10-14T16:15:42Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Raw block devices are possible with generic ephemeral volumes, so rejecting a
pod with that combination is wrong.

Special notes for your reviewer:

Found while working on E2E tests. Here's an example of a failures from those tests: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/105659/pull-kubernetes-e2e-gce-csi-serial/1448711002941034496/

Does this PR introduce a user-facing change?

Generic ephemeral volumes can be used also as raw block devices, but the Pod validation was refusing to create pods with that combination.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1698-generic-ephemeral-volumes

k8s-ci-robot · 2021-10-14T16:15:49Z

@pohly: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions 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/test-infra repository.

pohly · 2021-10-14T16:16:08Z

/sig storage
/priority important-soon

jsafrane · 2021-10-14T17:37:33Z

/lgtm
from sig-storage POV.

pohly · 2021-10-14T17:52:35Z

/retest

test flake

pohly · 2021-10-14T17:52:58Z

/assign @smarterclayton

For approval.

pohly · 2021-10-15T11:50:20Z

After considering potential merge conflicts (now and during backporting, should we decide to do that) my conclusion is that it should be safe to merge the E2E modifications in this PR. Then #105659 can be merged independently.

I also added unit test cases.

/cc @mauriciopoppe

pohly · 2021-10-15T14:38:41Z

/retest

mauriciopoppe · 2021-10-18T20:39:58Z

pkg/apis/core/validation/validation.go

@@ -418,7 +418,8 @@ func IsMatchedVolume(name string, volumes map[string]core.VolumeSource) bool {

 func isMatchedDevice(name string, volumes map[string]core.VolumeSource) (bool, bool) {


it's kinda hard to understand the return value without any comments :(, I read that it's used as didMatch, isPVC := isMatchedDevice(devName, volumes)

That's how it was before, but I can of course enhance this while I am touching the code.

Done. Please take another look.

mauriciopoppe · 2021-10-18T20:55:11Z

test/e2e/storage/framework/testpattern.go

@@ -297,6 +297,13 @@ var (
 		SnapshotType:           DynamicCreatedSnapshot,
 		SnapshotDeletionPolicy: DeleteSnapshot,
 	}
+	// BlockVolModeGenericEphemeralVolume is for generic ephemeral inline volumes in raw block mode.
+	BlockVolModeGenericEphemeralVolume = TestPattern{
+		Name:        "Generic Ephemeral-volume (block volmode) (late-binding)",


I'm not sure about what's our rule to add tags, e.g. [Feature:x] vs (block volmode) or (late-binding)

Round brackets are just comments. They have no special meaning.

mauriciopoppe · 2021-10-18T21:40:43Z

test/e2e/storage/testsuites/ephemeral.go

-	if framework.NodeOSDistroIs("windows") {
-		command = "ls /mnt/test* && sleep 10000"
-	}
+	command := "sleep 10000"


Is there a reason we're not checking in the Pod command that the paths were mounted? I'm not sure why this command would need to change for block volumes

Block volumes don't get mounted.

The code here could have been made more intelligent, but it was already redundant (the same mount check is done also in individual tests), so I decided to simplify here.

mauriciopoppe

A few comments about the pattern name and the TestEphemeral command change

Raw block devices are possible with generic ephemeral volumes, so rejecting a pod with that combination is wrong.

This adds a new test pattern and uses it for the inline volume tests. Because the kind of volume now varies more, validation of the mount or block device is always done by the caller of TestEphemeral.

mauriciopoppe · 2021-10-19T17:24:27Z

Thanks for addressing the comments
/lgtm

pohly · 2021-10-19T18:24:26Z

@smarterclayton: PR is ready for approval...

thockin · 2021-10-22T21:52:21Z

/approve

k8s-ci-robot · 2021-10-22T21:53:04Z

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

~~pkg/apis/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-triage-robot · 2021-10-23T00:05:45Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 14, 2021

k8s-ci-robot requested review from wojtek-t and yujuhong October 14, 2021 16:16

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Oct 14, 2021

k8s-ci-robot assigned jsafrane Oct 14, 2021

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

k8s-ci-robot assigned smarterclayton Oct 14, 2021

pohly force-pushed the generic-ephemeral-volume-raw-block branch from 01977a0 to 119725c Compare October 15, 2021 11:49

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 15, 2021

k8s-ci-robot requested a review from mauriciopoppe October 15, 2021 11:50

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 15, 2021

pohly force-pushed the generic-ephemeral-volume-raw-block branch from 119725c to f75913e Compare October 15, 2021 13:08

mauriciopoppe reviewed Oct 18, 2021

View reviewed changes

pohly added 2 commits October 19, 2021 13:30

storage validation: accept generic ephemeral volumes as volume device

a40d2eb

Raw block devices are possible with generic ephemeral volumes, so rejecting a pod with that combination is wrong.

storage e2e: check raw block support for generic ephemeral volumes

a90a3c6

This adds a new test pattern and uses it for the inline volume tests. Because the kind of volume now varies more, validation of the mount or block device is always done by the caller of TestEphemeral.

pohly force-pushed the generic-ephemeral-volume-raw-block branch from f75913e to a90a3c6 Compare October 19, 2021 11:31

k8s-ci-robot assigned mauriciopoppe Oct 19, 2021

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

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

k8s-ci-robot merged commit 7fbb384 into kubernetes:master Oct 23, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage validation: accept generic ephemeral volumes as volume device #105682

storage validation: accept generic ephemeral volumes as volume device #105682

pohly commented Oct 14, 2021 •

edited

k8s-ci-robot commented Oct 14, 2021

pohly commented Oct 14, 2021

jsafrane commented Oct 14, 2021

pohly commented Oct 14, 2021

pohly commented Oct 14, 2021

pohly commented Oct 15, 2021

pohly commented Oct 15, 2021

mauriciopoppe Oct 18, 2021

pohly Oct 19, 2021

pohly Oct 19, 2021

mauriciopoppe Oct 18, 2021

pohly Oct 19, 2021

mauriciopoppe Oct 18, 2021 •

edited

pohly Oct 19, 2021

mauriciopoppe left a comment

mauriciopoppe commented Oct 19, 2021

pohly commented Oct 19, 2021

thockin commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

k8s-triage-robot commented Oct 23, 2021

		@@ -418,7 +418,8 @@ func IsMatchedVolume(name string, volumes map[string]core.VolumeSource) bool {

		func isMatchedDevice(name string, volumes map[string]core.VolumeSource) (bool, bool) {

storage validation: accept generic ephemeral volumes as volume device #105682

storage validation: accept generic ephemeral volumes as volume device #105682

Conversation

pohly commented Oct 14, 2021 • edited

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-ci-robot commented Oct 14, 2021

pohly commented Oct 14, 2021

jsafrane commented Oct 14, 2021

pohly commented Oct 14, 2021

pohly commented Oct 14, 2021

pohly commented Oct 15, 2021

pohly commented Oct 15, 2021

mauriciopoppe Oct 18, 2021

Choose a reason for hiding this comment

pohly Oct 19, 2021

Choose a reason for hiding this comment

pohly Oct 19, 2021

Choose a reason for hiding this comment

mauriciopoppe Oct 18, 2021

Choose a reason for hiding this comment

pohly Oct 19, 2021

Choose a reason for hiding this comment

mauriciopoppe Oct 18, 2021 • edited

Choose a reason for hiding this comment

pohly Oct 19, 2021

Choose a reason for hiding this comment

mauriciopoppe left a comment

Choose a reason for hiding this comment

mauriciopoppe commented Oct 19, 2021

pohly commented Oct 19, 2021

thockin commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

k8s-triage-robot commented Oct 23, 2021

pohly commented Oct 14, 2021 •

edited

mauriciopoppe Oct 18, 2021 •

edited