Fix volumes e2e test to check fsType #70823

mkimuram · 2018-11-08T21:08:09Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Volumes test doesn't check fsType, as a result, e2e test which requires particular fsType could pass even when driver doesn't provide the right fsType. Also, current gcePDCSIDriver.GetDynamicProvisionStorageClass in e2e test code is broken, so this PR will also fix it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #70822

Special notes for your reviewer:
/sig storage

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2018-11-08T21:08:17Z

Hi @mkimuram. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mkimuram · 2018-11-08T21:11:23Z

@msau42 @davidz627 @pohly

This PR will make fsType check as below:

STEP: Checking fsType is correct.
Nov  8 20:46:32.670: INFO: Running '/root/go/src/k8s.io/kubernetes/_output/bin/kubectl --server=https://localhost:6443/ --kubeconfig=/var/run/kubernetes/admin.
kubeconfig exec iscsi-client --namespace=e2e-tests-volumes-r8ljx -- grep  /opt/0 ext4 /proc/mounts'
Nov  8 20:46:33.002: INFO: stderr: ""
Nov  8 20:46:33.002: INFO: stdout: "/dev/sda /opt/0 ext4 rw,seclabel,relatime,block_validity,delalloc,barrier,user_xattr,acl 0 0\n"

I'm not sure csi gce actually works well, so I think that it would better to test [Testpattern: Dynamic PV (ext4)] volumes should be mountable with csi gce driver, before merging this PR.

davidz627 · 2018-11-08T21:43:26Z

/assign
/ok-to-test

davidz627 · 2018-11-08T22:27:08Z

test/e2e/common/volumes.go

@@ -91,7 +91,7 @@ var _ = Describe("[sig-storage] GCP Volumes", func() {
 			}

 			// Must match content of test/images/volumes-tester/nfs/index.html
-			framework.TestVolumeClient(c, config, nil, tests)


nit, add comment a la framework.TestVolumeClient(c, config, nil, "" /* fsType */, tests) for unnamed variables

Thank you for your comment. I will fix all of them as suggested.

davidz627 · 2018-11-08T22:37:44Z

test/e2e/storage/drivers/csi.go

@@ -186,6 +186,9 @@ func (g *gcePDCSIDriver) GetDynamicProvisionStorageClass(fsType string) *storage
 	suffix := fmt.Sprintf("%s-sc", g.driverInfo.Name)

 	parameters := map[string]string{"type": "pd-standard"}
+	if fsType != "" {
+		parameters["fsType"] = fsType


I don't think we specify fsType in the storage class for GCE PD, it's actually passed in on NodeStageVolume through the VolumeCapability field that comes from the PVSource.

IIUC, CSI provisioner will add fsType to PVC if fsType is set in StorageClass on provision. So, fsType should also be passed to PV, as a result, it will be passed as a VolumeCapability on NodeStageVolume when attach is requested.

oh cool, I didn't know that, thanks for the explanation!

@davidz627 this will still cause pd csi driver to fail right? Should we be using the new namespaced fstype parameter?

/hold
My understanding is PD csi tests in serial will start failing as is

davidz627 · 2018-11-09T19:17:22Z

@mkimuram I am testing this on the GCE PD CSI Driver now

oomichi · 2018-11-15T18:36:35Z

/test pull-kubernetes-e2e-kops-aws

mkimuram · 2018-11-29T20:34:47Z

@davidz627

Update this PR with two commits:

Improve debug message to debug failure for aws with ext3
Skip XFS test for csi gce pd on COS

I will check 1 to see what is happening.
Meanwhile, I would like you to check if this PR works well for csi gce pd as well, for it is still tagged with [Serial] and is not tested in CI automatically triggered on every commit.

davidz627 · 2018-11-29T21:40:28Z

test/e2e/storage/drivers/csi.go

@@ -348,6 +354,9 @@ func (g *gcePDExternalCSIDriver) GetDriverInfo() *DriverInfo {
 func (g *gcePDExternalCSIDriver) SkipUnsupportedTest(pattern testpatterns.TestPattern) {
 	framework.SkipUnlessProviderIs("gce", "gke")
 	framework.SkipIfMultizone(g.driverInfo.Framework.ClientSet)
+	if pattern.FsType == "xfs" {


is there a way to put this check in the "testsuite" skip checks instead of the driver skip tests? This skip "holds" for any driver that runs this test suite.

@davidz627

is there a way to put this check in the "testsuite" skip checks instead of the driver skip tests? This skip "holds" for any driver that runs this test suite.

I think that moving this logic to skipUnsupportedTest will make xfs tests skip for any drivers and any testsuites. However, to do so, we should blacklist unsupported distros instead of whitelisting just "ubuntu" and "custom", because we are not sure which distro will be passed here. (In current code, it assumes that this code path in gce driver's SkipUnsupportedTest reaches by only gce tests, so just whitelisting them is working fine.)

So, could you let me know which distro to blacklist?
(I guess that "gci" is needed here and how about "debian" and "coreos"?)

@davidz627

commit 07a456d will do above, assuming that only "gci" should be blacklisted. If this isn't true, please point it out.
(Currently, xfs test seems running only on gce CI, so we can go ahead once we confirm that it is true in gce CI, and add blacklist when we need to add some more latter.)

@msau42 @lookuptable do either of you know the answer to this? I am only aware of gci not having xfs support but there could be more

For gce CI, blacklisting "gci" is fine, although we need to find a way to deal with "custom", which can be both ubuntu or gci. Can we check for the presence of xfs tools on the node?

mkimuram · 2018-11-29T21:57:17Z

Improve debug message to debug failure for aws with ext3

Below log indicates that in-tree aws driver seems to fail in formatting or mounting ext3.

test/e2e/storage/testsuites/volumes.go:153
failed: getting the right fsType ext3
Expected error:
    <*errors.errorString | 0xc00021c250>: {
        s: "Failed to find \"ext3\", last result: \"/dev/xvdbu /opt/0 ext4 rw,relatime,data=ordered 0 0\n\"",
    }
    Failed to find "ext3", last result: "/dev/xvdbu /opt/0 ext4 rw,relatime,data=ordered 0 0
    "
not to have occurred
test/e2e/framework/volume_util.go:483

In-tree gce driver passes for both dynamic provisioning and pre-provisioned PV cases, so I suspect that this is in-tree aws driver issue. Note that in-tree aws driver doesn't run pre-provisioned PV case, because it doesn't define necessary methods like GetPersistentVolumeSource for this driver.

(I believe that #67195 has fixed aws drivers to at least not to fail when fsType is specified in provisioining, so this issue should happen after it.)

davidz627 · 2018-11-29T23:34:02Z

GCE PD CSI driver will fail because fstype parameter is not supported on CreateVolume.

This is part of a larger problem, I have a PR out here to fix. @mkimuram please keep an eye on it :)

kubernetes-csi/external-provisioner#178

mkimuram · 2019-01-23T07:58:28Z

/retest

mkimuram · 2019-01-23T08:33:00Z

@davidz627

I rebased the commits and updated fsType parameter for csi to csi.storage.k8s.io/fstype.
PTAL

davidz627 · 2019-01-24T20:08:25Z

tested and working with GCE PD CSI Driver now! Thanks @mkimuram
/lgtm

davidz627 · 2019-01-24T20:09:03Z

/assign @saad-ali
for final approval

davidz627 · 2019-01-25T22:38:33Z

/hold cancel

saad-ali

/approve

k8s-ci-robot · 2019-01-28T22:33:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mkimuram, saad-ali

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/volume/awsebs/OWNERS~~ [jsafrane,saad-ali]
~~test/OWNERS~~ [saad-ali]

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

mkimuram · 2019-01-28T23:49:49Z

/retest

mkimuram · 2019-01-29T00:55:21Z

/retest

mkimuram · 2019-01-29T03:01:23Z

/retest

k8s-ci-robot · 2019-01-29T03:42:35Z

@mkimuram: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`b152d43`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot requested review from copejon and krousey November 8, 2018 21:08

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 8, 2018

mkimuram mentioned this pull request Nov 8, 2018

Make csi drivers and in-tree drivers share e2e tests #68025

Merged

k8s-ci-robot assigned davidz627 Nov 8, 2018

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2018

davidz627 reviewed Nov 8, 2018

View reviewed changes

mkimuram force-pushed the issue/70822 branch from 828d682 to 07cf3fc Compare November 9, 2018 14:02

mkimuram mentioned this pull request Nov 28, 2018

Storage XFS volumes should be mountable test does not verify actual formatted filesystem #71528

Closed

mkimuram force-pushed the issue/70822 branch from 07cf3fc to 2e8acd5 Compare November 29, 2018 20:28

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2018

davidz627 reviewed Nov 29, 2018

View reviewed changes

mkimuram force-pushed the issue/70822 branch from 77784a6 to 5594b38 Compare November 30, 2018 20:23

mkimuram force-pushed the issue/70822 branch from e9bbab3 to b152d43 Compare January 23, 2019 05:35

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 23, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2019

k8s-ci-robot assigned saad-ali Jan 24, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2019

pohly mentioned this pull request Jan 28, 2019

E2E storage: more tests for different pod/node combinations #72002

Merged

saad-ali approved these changes Jan 28, 2019

View reviewed changes

saad-ali added the kind/bug Categorizes issue or PR as related to a bug. label Jan 28, 2019

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jan 28, 2019

saad-ali added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jan 28, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 28, 2019

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

k8s-ci-robot merged commit b670115 into kubernetes:master Jan 29, 2019

davidz627 mentioned this pull request Mar 8, 2019

XFS test is passing when it shouldn't be kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#192

Closed

mkimuram mentioned this pull request May 29, 2019

Skip GCE PD in-tree plugin tests if fstype is xfs and node distro is not ubuntu or custom #78459

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix volumes e2e test to check fsType #70823

Fix volumes e2e test to check fsType #70823

mkimuram commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

mkimuram commented Nov 8, 2018

davidz627 commented Nov 8, 2018

davidz627 Nov 8, 2018

mkimuram Nov 9, 2018

davidz627 Nov 8, 2018

mkimuram Nov 9, 2018

davidz627 Nov 9, 2018 •

edited

msau42 Dec 4, 2018

msau42 Dec 4, 2018

davidz627 commented Nov 9, 2018

oomichi commented Nov 15, 2018

mkimuram commented Nov 29, 2018

davidz627 Nov 29, 2018

mkimuram Nov 29, 2018

mkimuram Nov 29, 2018 •

edited

davidz627 Dec 4, 2018

msau42 Dec 4, 2018

mkimuram commented Nov 29, 2018

davidz627 commented Nov 29, 2018

mkimuram commented Jan 23, 2019

mkimuram commented Jan 23, 2019

davidz627 commented Jan 24, 2019

davidz627 commented Jan 24, 2019

davidz627 commented Jan 25, 2019

saad-ali left a comment

k8s-ci-robot commented Jan 28, 2019

mkimuram commented Jan 28, 2019

mkimuram commented Jan 29, 2019

mkimuram commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

Fix volumes e2e test to check fsType #70823

Fix volumes e2e test to check fsType #70823

Conversation

mkimuram commented Nov 8, 2018

k8s-ci-robot commented Nov 8, 2018

mkimuram commented Nov 8, 2018

davidz627 commented Nov 8, 2018

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

davidz627 Nov 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Nov 9, 2018

oomichi commented Nov 15, 2018

mkimuram commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram Nov 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Nov 29, 2018

davidz627 commented Nov 29, 2018

mkimuram commented Jan 23, 2019

mkimuram commented Jan 23, 2019

davidz627 commented Jan 24, 2019

davidz627 commented Jan 24, 2019

davidz627 commented Jan 25, 2019

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 28, 2019

mkimuram commented Jan 28, 2019

mkimuram commented Jan 29, 2019

mkimuram commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

davidz627 Nov 9, 2018 •

edited

mkimuram Nov 29, 2018 •

edited