-
Notifications
You must be signed in to change notification settings - Fork 217
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
Create snapshotclass based on snapshotter sidecar version #126
Create snapshotclass based on snapshotter sidecar version #126
Conversation
/test pull-kubernetes-csi-csi-driver-host-path-1-17-on-kubernetes-master |
/test pull-kubernetes-csi-csi-driver-host-path-1-15-on-kubernetes-master |
/assign @msau42 |
deploy/util/deploy-hostpath.sh
Outdated
kubectl apply -f $SNAPSHOTCLASS_PATH | ||
echo "deploying snapshotclass based on snapshotter version" | ||
snapshotter_version="$(rbac_version "${BASE_DIR}/hostpath/csi-hostpath-snapshotter.yaml" csi-snapshotter false)" | ||
kubectl apply -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${snapshotter_version}/examples/kubernetes/snapshotclass.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is sort of strange because the snapshotclass should live with the driver like storageclass does.
Another option I was thinking is to disable snapshots when the deployment version doesn't match the kubernetes version (before 1.17). Because it was an alpha feature so there's no guarantees for cross version compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ok, I'm unfamiliar with how to skip the snapshot tests. If we just don't apply a snapshotclass, will that cause the tests to be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while it was alpha, we only run the snapshot tests when the cluster and deployment version matched, and also only for the latest version. So I think for logic < 1.17, we can make this assumption.
7bb4d65
to
2bcd316
Compare
2bcd316
to
776a090
Compare
1.17 test flaked I believe:
/test pull-kubernetes-csi-csi-driver-host-path-1-17-on-kubernetes-1-17 |
776a090
to
d1dac1a
Compare
Signed-off-by: Grant Griffiths <grant@portworx.com>
d1dac1a
to
f946d8c
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggriffiths, msau42 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 |
Signed-off-by: Grant Griffiths grant@portworx.com
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
We were deploying the latest snapshotclass for
on-kubernetes-master
tests, regardless of the csi-snapshotter version. Now we'll apply the snapshotclass depending on the sidecar version.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: