Add k8s e2e test support & script #196
Conversation
|
I used it to test vpa, helpful. 👍 |
test/e2e/testdriver_template.yaml
Outdated
| controllerExpansion: true | ||
| nodeExpansion: true | ||
| onlineExpansion: false | ||
| volumeLimits: false |
There was a problem hiding this comment.
why volumeLimits is disabled?
There was a problem hiding this comment.
This test will create more than max_volumes_per_node of PVCs to check the functioning of CSI Volume Limit, which is a very slow test, and which, in my opinion, is more of a K8s CSI logic rather than the CSIDriver implementation.
But we can add it back if it's considered necessary, what's your opinion?
There was a problem hiding this comment.
I don't understand. This test will create more than max_volumes_per_node of PVCs , for qingcloud-csi, the max volumes is 9, so the test will create 10 or 10+ volumes, correct? 10+ volumes will be very slow? why?
There was a problem hiding this comment.
It's the description in the e2e test pkg. I've checked the test case, I think the " very slow" warning is for some drivers with more than 100 max volumes.
There're 2 test cases in the volume limits, which means in the case of qingcloud-csi, a total of 20 PVCs will be created, which is acceptable.
I'll change this test back to enabled.
test/e2e/chart/Chart.yaml
Outdated
| @@ -0,0 +1,34 @@ | |||
| # Copyright (C) 2018 Yunify, Inc. | |||
There was a problem hiding this comment.
are you adding the qingcloud-csi chart here? Why not using the existing chart?
There was a problem hiding this comment.
In the test environment, there are likely already a deployed qingcloud-csi, with some of the cluster-level components like ClusterRole and ClusterRoleBinding created in a static name, which results in a conflict when deploying the qingcloud-csi driver of the e2e test.
So I'm adding a local chart with the names modified to a configurable template, to avoid conflicts.
There was a problem hiding this comment.
okay, I understand. But adding a bunch of source files from another repo to this repo is strongly discouraged. They work now, but maintaining them in the future will be a nightmare.
Try to use the existing qingcloud-csi chart ( or even modify the existing qingcloud-csi chart to support the e2e test), but don't add the chart source codes to this repo.
The simplest and quickest way to me may be adding a prerequisite: this e2e test need a clean cluster without qingcloud-csi deployed. if it's deployed, uninstall it before proceed.
There was a problem hiding this comment.
Yeah, I agree on that, will just leave it to the user in the prerequisites doc section.
test/e2e/README.md
Outdated
| tar -xf e2e-tests.tar.gz --directory=./qingcloud-csi/test/e2e && rm e2e-tests.tar.gz | ||
| ``` | ||
|
|
||
| 2. Multiple nodes are preferred, if that's possible, as some tests will check against volume drifting from one node to another. |
There was a problem hiding this comment.
will the test detect the number of nodes? If I run the test on a single-node cluster, will some test cases fail or skip?
There was a problem hiding this comment.
No, the test won't skip tests according to the number of nodes, and will time out if there is only one node.
I'll figure out a way to get around this. I think I'll check the number of nodes in the script and skip the multi-node tests if there is only one node.
There was a problem hiding this comment.
yes, try to auto-detect the cluster nodes and enable the proper tests. If that's is impossible, add a argument to your test script or write a guide for user to self-check the cluster nodes and disable/enable the proper tests.
test/e2e/README.md
Outdated
|
|
||
| ## Run | ||
|
|
||
| Execute `./run_e2e_test.sh` to run the e2e tests. |
There was a problem hiding this comment.
do I need use root or regular user to run this script?
There was a problem hiding this comment.
root access in Linux is not necessary, but admin access to the K8s cluster is needed. I'll update the usage doc to clarify that.
|
/hold do not merge until I've verified the new commit |
dkeven
changed the title
dkeven
force-pushed
the
supporte2etest
branch
5 times, most recently
from
678679b to
511aa5e
Compare
Signed-off-by: dkeven <[email protected]>
dkeven
force-pushed
the
supporte2etest
branch
from
511aa5e to
aabf941
Compare
dkeven
changed the title
|
/unhold I've finished updating the commit, plz take a look @stoneshi-yunify @f10atin9 There appears to be a bug when I'm testing the e2e functions. I'll create another PR to separate the commit. |
|
It looks like the ci-bot is hanging again |
Signed-off-by: dkeven [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds a script, along with some config templates, to conveniently run the Kubernetes storage external e2e test, including:
Which issue(s) this PR fixes:
Fixes #189
Special notes for your reviewer:
This PR adds a slightly modified version of the csi-qingcloud helm chart, mainly for avoiding conficts with existing
csi-qingcloud's staically-named cluster-level resources likeClusterRoleandClusterRoleBinding, with each of them changed to a template-rendered name.Does this PR introduce a user-facing change?:
Additional documentation e.g., usage docs, etc.: