Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run upstream e2e tests #55

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Jul 19, 2019

Is this a bug fix or adding new feature? new feature

What is this PR about? / Why do we need it? ref: #32. We should run all the existing upstream tests for storage drivers. They cover static provisioning, RWX mode from multiple pods, and more.

hack/run-e2e-test is based on the one in EBS CSI repo: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/hack/run-e2e-test. It creates a kops cluster, creates an EFS file system, runs tests, and cleans up.

test/e2e/e2e_test.go is the tests themselves. It simply imports the upstream test suites and runs them.

depends on #48 and #51

Also depends on kubernetes/kubernetes#80375

What testing is done?
I edited the script to point to my s3 bucket instead of s3://k8s-kops-csi-e2e. Then, I ran make test-e2e as I would expect prow to.

Skipping slow tests (by manually editing hack/run-e2e-test script):
Ran 11 of 142 Specs in 330.736 seconds
SUCCESS! -- 11 Passed | 0 Failed | 0 Pending | 131 Skipped
--- PASS: TestEFSCSI (330.74s)
PASS
ok github.com/kubernetes-sigs/aws-efs-csi-driver/test/e2e 331.139s

Running all tests:
Ran 23 of 142 Specs in 1310.545 seconds
SUCCESS! -- 23 Passed | 0 Failed | 0 Pending | 119 Skipped
--- PASS: TestEFSCSI (1310.55s)
PASS
ok github.com/kubernetes-sigs/aws-efs-csi-driver/test/e2e 1310.917s

I am not sure if the prow AWS account has permissions to create EFS file systems, I ran the test using an admin account. The plan is to add the prow job and tweak it later.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 19, 2019
@k8s-ci-robot k8s-ci-robot requested review from d-nishi and justinsb July 19, 2019 23:30
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 19, 2019
@wongma7 wongma7 changed the title WIP: run upstream e2e tests Run upstream e2e tests Jul 24, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2019
@wongma7 wongma7 force-pushed the e2e-test branch 3 times, most recently from ac2fc4d to e79735a Compare August 6, 2019 21:00
@leakingtapan
Copy link
Contributor

/retest

@wongma7
Copy link
Contributor Author

wongma7 commented Aug 7, 2019

An error occurred (AccessDeniedException) when calling the CreateFileSystem operation: User: arn:aws:iam:::user/awstester is not authorized to perform: elasticfilesystem:CreateFileSystem on the specified resource

now my script is stuck in a loop trying to delete...need to add a timeout.

@wongma7
Copy link
Contributor Author

wongma7 commented Aug 7, 2019

/test pull-aws-efs-csi-driver-e2e

@wongma7 wongma7 force-pushed the e2e-test branch 3 times, most recently from 0db9529 to 030db3d Compare August 7, 2019 22:16
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 8, 2019

/test pull-aws-efs-csi-driver-e2e

@wongma7
Copy link
Contributor Author

wongma7 commented Aug 8, 2019

e2e passed! https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-efs-csi-driver/55/pull-aws-efs-csi-driver-e2e/1159269511698845697

Ran 23 of 141 Specs in 1316.602 seconds
SUCCESS! -- 23 Passed | 0 Failed | 0 Pending | 118 Skipped

However, it's not separated by testcase in testgrid, so I'd like to figure that out: https://testgrid.k8s.io/sig-aws-efs-csi-driver#e2e-test-single-az

@leakingtapan
Copy link
Contributor

However, it's not separated by testcase in testgrid, so I'd like to figure that out

see https://gcsweb.k8s.io/gcs/kubernetes-jenkins/logs/ci-gcp-compute-persistent-disk-csi-driver-latest-k8s-master-integration/1158851998234185732/artifacts/

you need to generate some junit xml files from your job

@wongma7 wongma7 force-pushed the e2e-test branch 2 times, most recently from d53a4ac to 03e5519 Compare August 9, 2019 01:07
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 9, 2019

@wongma7 wongma7 force-pushed the e2e-test branch 2 times, most recently from 1d3c15c to 3063b1e Compare August 12, 2019 19:55
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 12, 2019

/test pull-aws-efs-csi-driver-unit

hack/run-e2e-test Outdated Show resolved Hide resolved
hack/run-e2e-test Outdated Show resolved Hide resolved
kubectl apply -f $TEST_DIR/manifest.yaml

echo "Creating EFS file system"
aws efs create-file-system --creation-token $TEST_ID --tags Key=KubernetesCluster,Value=$CLUSTER_NAME.k8s.local --region $REGION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will make transition to the new test framework harder. Could we create the EFS fs in the go test code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the plan but atm it makes sense to keep this in the script to reuse the various env vars

fi

pushd ./test/e2e
$DEP_PATH ensure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is go dep needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only way to vendor k/k into the test/e2e directory. I don't want to check in huge vendor so have to download dep and do dep ensure during test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it works for EBS to consume k/k with just go mod. See here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but that is with 1.14 k/k. After 1.15, k/k moved to modules and I can't use go mod anymore. (Seems like it doesn't make sense, I know)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More details: when k/k used godeps, it just symlinked stuff in staging dir into vendor dir. Now with go.mod, it uses some weird tricks with v0.0.0 https://github.com/kubernetes/kubernetes/blob/master/go.mod#L144 and replace directives and I've not figured out a way to import it using go mod.

cancelPodLogs := testsuites.StartPodLogs(f)

/* TODO deploy the driver before the test.
cleanup, err := f.CreateFromManifests(nil, "deploy/kubernetes/manifest.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, removed; much simpler to do the build+push+deploy of driver in the script beforehand.

test/e2e/e2e_test.go Outdated Show resolved Hide resolved
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@leakingtapan
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 15, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 16, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 16, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 16, 2019
Copy link
Contributor

@leakingtapan leakingtapan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
minor comments

@@ -0,0 +1,22 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a copyright head similar to other scripts?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2019
@leakingtapan
Copy link
Contributor

seems the test reporter for prow ARTIFACT is not working. could you check on that? the e2e test failed too

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 21, 2019

Copy link
Contributor

@leakingtapan leakingtapan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

minor comment

hack/run-e2e-test Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2019
@wongma7
Copy link
Contributor Author

wongma7 commented Aug 22, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2019
@leakingtapan
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leakingtapan, wongma7

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit 7c42c57 into kubernetes-sigs:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants