Enable the VolumeGroupSnapshot tests #283
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
cncf-cla: yes
Signed-off-by: Penghao <[email protected]>
Phaow
force-pushed
the
enable-vgs-tests
branch
from
d0ea3ee to
9fb036e
Compare
|
/test pull-kubernetes-csi-release-tools-csi-test |
|
Hi @xing-yang @jsafrane @gnufied @msau42 , could you please help review when you get a chance? Thank you! |
|
/assign @xing-yang @jsafrane @gnufied @msau42 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Phaow The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
Phaow
force-pushed
the
enable-vgs-tests
branch
from
a1ecfa1 to
5005ced
Compare
|
Hi @jsafrane , could you help take a look again when you get a chance? Thank you! I hack some codes in master...Phaow:csi-release-tools:dev and verify it works, which needs update in job definition part, the script also needs the https://github.com/kubernetes-csi/csi-release-tools/blob/master/filter-junit.go, I will download it in prow job definition part. |
Signed-off-by: Penghao <[email protected]>
Phaow
force-pushed
the
enable-vgs-tests
branch
from
5005ced to
9160d31
Compare
|
/test pull-kubernetes-csi-release-tools-external-provisioner |
1 similar comment
|
/test pull-kubernetes-csi-release-tools-external-provisioner |
prow.sh
Outdated
| vgs) | ||
| # VGS tests are in in-tree/core tests; do NOT pass -storage.testdriver | ||
| ginkgo_target="${CSI_PROW_WORK}/e2e.test" | ||
| cd "${e2e_src_dir}" || die "cd ${e2e_src_dir} failed" | ||
| ;; |
There was a problem hiding this comment.
I admit I don't understand why there is vgs as a special value. IMHO the VGS tests don't change how junit fiels a re generated / moved or what binaries to run.
Please correct me if I am wrong. There were two kinds of tests:
local, running "e2e-local.test", if available- non-
local, running "e2e.test -storage.testdriver=test-driver.yaml" with csi-driver-hostpath with CSI sidecars either compiled from a PR or canary or whatever.
Now you want to add a new set of tests
e2e.testwith in-tree tests, that have all sidecars hardcoded in Kubernetes manifests, only with optional "--feature-gate=CSIVolumeGroupSnapshot=true" in its external-snapshotter (see Refine: VolumeGroupSnapshot tests kubernetes/kubernetes#134214). Should we call itin-tree? And focus not justFeature:volumegroupsnapshot, but all[Driver: csi-hostpath]
There was a problem hiding this comment.
Now that I am reading what I wrote, should the in-tree tests use all sidecars from the PR they run in? Or even canary? That would be definitely for a future improvement, not this PR.
There was a problem hiding this comment.
non-local, running "e2e.test -storage.testdriver=test-driver.yaml" with csi-driver-hostpath with CSI sidecars either compiled from a PR or canary or whatever.
It seems currently only snapshot controller image could used the one compile from the PR, the snapshotter and other sidecar images do not override for hostpath csi driver->https://github.com/kubernetes-csi/csi-release-tools/blob/master/prow.sh#L720-L769 .
should the in-tree tests use all sidecars from the PR they run in?
From my understanding, we'd better keep this part in the prow.sh with external CSI testsuite, which we could replace the hostpath sidecar images directly.
To run vgs test toward external hostpath csi driver we still needs hack 90b69e4#diff-c2d96beb8cf73c62f7ae059aff96e381a8d73317df3ce356f36ba6b46338959dR784-R804 and kubernetes/kubernetes#134633.
So maybe we'd better choose to use the external hostpath csi driver for sidecars tests, right?
There was a problem hiding this comment.
should the in-tree tests use all sidecars from the PR they run in?
From my understanding, we'd better keep this part in the prow.sh with external CSI testsuite, which we could replace the hostpath sidecar images directly.
Yes, that's what I think too. But this PR adds vgs tests that will test with csi-driver-hospath yaml files hardcoded in k/k. It would be better to run them with the hostpath driver installed in prow.sh.
There was a problem hiding this comment.
Change to run vgs tests from csi external and integration with k/k PR works as expected.
Signed-off-by: Penghao <[email protected]>
Signed-off-by: Penghao <[email protected]>
|
@Phaow Sorry if you answered this already, but why aren't we implementing this via - https://github.com/kubernetes-csi/external-snapshotter/blob/master/.prow.sh ? Is that not doable? |
|
Hi @gnufied , thanks for looking into this, we have a discussion try to use it also for k/k vgs testing -> |
Signed-off-by: Penghao <[email protected]>
Signed-off-by: Penghao <[email protected]>
| # Call post-install hook(s) if defined | ||
| if [ -n "${CSI_PROW_DRIVER_POSTINSTALL}" ]; then | ||
| # Iterate through space-separated list of hook functions | ||
| for hook_func in ${CSI_PROW_DRIVER_POSTINSTALL}; do |
There was a problem hiding this comment.
I'm thinking about if we want to test all the storage beta features in one k/k job use array we could combine some hooks to avoid write a new one do all the configurations even if a new hook combine some hooks inside it. Is there something I missed?
6 participants
What type of PR is this?
/kind test
What this PR does / why we need it:
Used the temporary job for vgs
pull-kubernetes-e2e-storage-kind-vgsverified it works as expected in live PR.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: