e2e: defend on k8s version for snap-controller install #1534

pkalever · 2020-09-28T15:54:50Z

Describe what this PR does

Currently the scripts/install-snapshot.sh script needs to be called
depending on the Kubernetes version. It would be much easier to use the
script if it is intelligent enough to decide itself whether k8s snapshot
controller needs to be installed or not.

Fixes: #1139
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

pkalever · 2020-09-28T15:57:43Z

@nixpanic as mentioned this will only fix a part of issue #1139.

The second part is to detect 'what version of the snapshot components to install' right?
Where can I find the k8s server version => snap comp version mapping?

Thanks!

pkalever · 2020-09-28T16:24:44Z

I have come across this reference but again this doesn't have any mapping for the latest releases > v2.1.0

pkalever · 2020-09-28T17:40:35Z

Went through each release notes at https://github.com/kubernetes-csi/external-snapshotter/tags

If I understand it right, for our ceph-csi we need atleast k8s >= v1.14.0,

So the map should be:

---------------------------------------------------------------------------
|   Kube versions                |      External snapshotter version      |
---------------------------------------------------------------------------
|      v1.14.0 - v1.16.0         |                 v1.2.2                 |
---------------------------------------------------------------------------
|      v1.17.0 - Latest          |              v2.1.0 or v3.0.0          |
---------------------------------------------------------------------------

Does it look good?

nixpanic

Looks mostly good to me. Note that with the Partially Fixes: #... comment, the issue will be closed because GitHub detects Fixes: #...

scripts/install-snapshot.sh

Madhu-1 · 2020-09-29T07:40:49Z

Went through each release notes at https://github.com/kubernetes-csi/external-snapshotter/tags

If I understand it right, for our ceph-csi we need atleast k8s >= v1.14.0,

So the map should be:

---------------------------------------------------------------------------
|   Kube versions                |      External snapshotter version      |
---------------------------------------------------------------------------
|      v1.14.0 - v1.16.0         |                 v1.2.2                 |
---------------------------------------------------------------------------
|      v1.17.0 - Latest          |              v2.1.0 or v3.0.0          |
---------------------------------------------------------------------------

Does it look good?

we don't need to support snapshots if the kubernetes version is <1.17 as snapshotter beta is not backward compatible with alpha version

Madhu-1 · 2020-09-29T07:41:40Z

scripts/install-snapshot.sh

+
+KUBE_VERSION=$(kubectl version --short 2>/dev/null | grep "^Server Version" | cut -d' ' -f3)
+if [[ -z ${KUBE_VERSION} ]]; then
+    echo "could not get kube server version"


could not get Kubernetes server version

scripts/install-snapshot.sh

pkalever · 2020-09-29T07:44:41Z

Looks mostly good to me. Note that with the Partially Fixes: #... comment, the issue will be closed because GitHub detects Fixes: #...

Yep, I'm aware of this, my intention was to fix the other part of the issue as part of this PR.

@nixpanic @Madhu-1 @humblec can I have at least one ack on the table of mapping at #1534 (comment)

Logically:

if v1.14.0 <= KUBE_VERSION <= v1.16.0; then
    SNAPSHOT_VERSION=v1.2.2
elif KUBE_VERSION >= v1.17.0; then
   SNAPSHOT_VERSION=v3.0.0
fi

We also need to fix in the current patch, like we need to skip the snapshot controller install if k8s version < 1.14.0, rather than 1.17.0

Does this all sound good?

Thanks!

Madhu-1 · 2020-09-29T07:47:12Z

Looks mostly good to me. Note that with the Partially Fixes: #... comment, the issue will be closed because GitHub detects Fixes: #...

Yep, I'm aware of this, my intention was to fix the other part of the issue as part of this PR.

@nixpanic @Madhu-1 @humblec can I have at least one ack on the table of mapping at #1534 (comment)

Logically:
if v1.14.0 <= KUBE_VERSION <= v1.16.0; then
    SNAPSHOT_VERSION=v1.2.2
elif KUBE_VERSION >= v1.17.0; then
   SNAPSHOT_VERSION=v3.0.0
fi
We also need to fix in the current patch, like we need to skip the snapshot controller install if k8s version < 1.14.0, rather than 1.17.0

Does this all sound good?

Thanks!

already added here #1534 (comment)

pkalever · 2020-09-29T07:50:16Z

we don't need to support snapshots if the kubernetes version is <1.17 as snapshotter beta is not backward compatible with alpha version

Thanks Madhu, this means the current patch is good enough to fix #1139

@Madhu-1 we got a new release out for External snapshotter v3.0.0 a couple of weeks ago.
I hope we are good to update the SNAPSHOT_VERSION to v3.0.0?

Madhu-1 · 2020-09-29T07:53:01Z

we don't need to support snapshots if the kubernetes version is <1.17 as snapshotter beta is not backward compatible with alpha version

Thanks Madhu, this means the current patch is good enough to fix #1139

@Madhu-1 we got a new release out for External snapshotter v3.0.0 a couple of weeks ago.
I hope we are good to update the SNAPSHOT_VERSION to v3.0.0?

Yes, @agarwal-mudit is already working on updating sidecar containers. and also can you please update the version from v2.1.1 to 3.0.0 here

ceph-csi/scripts/install-snapshot.sh

Line 5 in d7802eb

SNAPSHOT_VERSION=${SNAPSHOT_VERSION:-"v2.1.1"}

Madhu-1 · 2020-09-29T08:17:37Z

/test ci/centos/mini-e2e

Madhu-1 · 2020-09-29T08:17:55Z

/test ci/centos/mini-e2e/k8s-1.18

pkalever · 2020-09-29T12:34:55Z

/test ci/centos/mini-e2e/k8s-1.19

pkalever · 2020-09-29T12:35:15Z

/test ci/centos/mini-e2e/k8s-1.18

pkalever · 2020-09-29T12:35:24Z

/test ci/centos/mini-e2e-helm/k8s-1.19

pkalever · 2020-09-29T13:02:02Z

CI fails with:

error: unable to read URL "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml", server reported 404 Not Found, status code=404

Maybe we need to wait until Mudit patches land and rebase this on top of them.

agarwal-mudit · 2020-09-29T13:23:17Z

CI fails with:

error: unable to read URL "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml", server reported 404 Not Found, status code=404

Maybe we need to wait until Mudit patches land and rebase this on top of them.

Will try to raise a PR by tomorrow.

nixpanic

The failed tests complain that the v3.0.0 VolumeSnapshotClass CRD can not be found:

error: unable to read URL "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml", server reported 404 Not Found, status code=404

Please check where the CRD needs to come from with newer versions.

scripts/install-snapshot.sh

pkalever · 2020-10-01T08:35:37Z

Please check where the CRD needs to come from with newer versions.

Indeed, as dicussed in the above comments, this should be rebased on Mudit's changes, once the changes are merged. 👍

agarwal-mudit · 2020-10-01T08:39:03Z

Please check where the CRD needs to come from with newer versions.

Indeed, as dicussed in the above comments, this should be rebased on Mudit's changes, once the changes are merged.

On a second look, is this path hard coded in the script because the crd path is changed in v3.0.0.
This is the new path: https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/client/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

Madhu-1 · 2020-10-01T08:41:25Z

@pkalever Mudit PR won't fix this one I think. we can go and make the below changes, please double-check once again

For 2.1 below we need to use "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/config/crd
For 3.0 onwards we need to use "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v3.0.0/client/config/crd

pkalever · 2020-10-05T10:13:54Z

@Madhu-1 you are right, starting from external snapshotter v2.0 they are at external-snapshotter/config/crd/, at v3.0 they moved to external-snapshotter/config/crd/client/.

Also came across kubernetes-csi/external-snapshotter#365 :-)

Based on Neils's comments, I'm inclined to move the SNAPSHOT_VERSION variable to build.env.

Do we need to still support v2.1.1? Meaning do we need to defend on the versions and select the appropriate link/path in this script? I'm not sure if we have a strong reason to support <3.0 anymore.

Thanks!

Pull request has been modified.

scripts/install-snapshot.sh

Pull request has been modified.

Madhu-1 · 2020-10-07T04:40:17Z

/test all

pkalever · 2020-10-12T09:03:48Z

@Madhu-1 ready for merge?

Madhu-1 · 2020-10-12T09:16:01Z

@Madhu-1 ready for merge?

will wait for @nixpanic to review it.

build.env

scripts/install-snapshot.sh

nixpanic · 2020-10-12T09:46:30Z

scripts/install-snapshot.sh

+    echo "${KUBE_VERSION}" | sed 's/^v//' | cut -d'.' -f"${1}"
+}
+
+KUBE_VERSION=$(kubectl version --short 2>/dev/null | grep "^Server Version" | cut -d' ' -f3)


Redirecting stderr to /dev/null might make debugging more difficult in case an error occurs. Is there a good reason to not display potential errors here?

Because we do not want the next line (condition) to return false in case of error.

But the output of stderr is not captured in KUBE_VERSION if you do not use 2>&1 or a similar redirection?

fixed now. Thanks Neils.

Pull request has been modified.

pkalever · 2020-10-14T07:55:12Z

/test all

Currently the scripts/install-snapshot.sh script needs to be called depending on the Kubernetes version. It would be much easier to use the script if it is intelligent enough to decide itself whether k8s snapshot controller needs to be installed or not. Fixes: ceph#1139 Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

For external-snapshotter v3.0.1, Supported CSI Spec Versions: 1.0.0, 1.1.0, 1.2.0 Minimum Kubernetes version: 1.17 Recommended Kubernetes version: 1.17 Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

nixpanic added component/testing Additional test cases or CI work enhancement New feature or request labels Sep 29, 2020

nixpanic reviewed Sep 29, 2020

View reviewed changes

scripts/install-snapshot.sh Outdated Show resolved Hide resolved

Madhu-1 reviewed Sep 29, 2020

View reviewed changes

scripts/install-snapshot.sh Outdated Show resolved Hide resolved

pkalever force-pushed the e2e-fixes branch from d7802eb to f48f730 Compare September 29, 2020 08:09

pkalever changed the title ~~e2e: defend on kube version for snap-controller install~~ e2e: defend on k8s version for snap-controller install Sep 29, 2020

Madhu-1 previously approved these changes Sep 29, 2020

View reviewed changes

nixpanic requested changes Oct 1, 2020

View reviewed changes

scripts/install-snapshot.sh Outdated Show resolved Hide resolved

pkalever force-pushed the e2e-fixes branch from f48f730 to b65d245 Compare October 5, 2020 10:42

Madhu-1 approved these changes Oct 5, 2020

View reviewed changes

humblec previously approved these changes Oct 5, 2020

View reviewed changes

Madhu-1 requested changes Oct 6, 2020

View reviewed changes

scripts/install-snapshot.sh Outdated Show resolved Hide resolved

pkalever force-pushed the e2e-fixes branch from bba307f to 60f7c0a Compare October 6, 2020 14:43

pkalever requested review from Madhu-1 and nixpanic October 12, 2020 09:02

Madhu-1 previously approved these changes Oct 12, 2020

View reviewed changes

humblec reviewed Oct 12, 2020

View reviewed changes

build.env Show resolved Hide resolved

humblec reviewed Oct 12, 2020

View reviewed changes

scripts/install-snapshot.sh Outdated Show resolved Hide resolved

nixpanic reviewed Oct 12, 2020

View reviewed changes

pkalever force-pushed the e2e-fixes branch from 60f7c0a to ac32650 Compare October 12, 2020 11:17

pkalever requested review from nixpanic and humblec October 12, 2020 11:17

humblec previously approved these changes Oct 12, 2020

View reviewed changes

pkalever force-pushed the e2e-fixes branch from ac32650 to 7acca64 Compare October 12, 2020 17:24

nixpanic approved these changes Oct 13, 2020

View reviewed changes

Madhu-1 approved these changes Oct 13, 2020

View reviewed changes

ceph-csi-bot force-pushed the e2e-fixes branch from 7acca64 to bc5f8a8 Compare October 13, 2020 12:57

Prasanna Kumar Kalever added 3 commits October 14, 2020 08:24

e2e: update external-snapshotter version to v3.0.1

116652f

For external-snapshotter v3.0.1, Supported CSI Spec Versions: 1.0.0, 1.1.0, 1.2.0 Minimum Kubernetes version: 1.17 Recommended Kubernetes version: 1.17 Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

e2e: add SNAPSHOT_VERSION to build.env

3cb094d

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

ceph-csi-bot force-pushed the e2e-fixes branch from bc5f8a8 to 3cb094d Compare October 14, 2020 08:24

mergify bot merged commit 4b7c9ea into ceph:master Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: defend on k8s version for snap-controller install #1534

e2e: defend on k8s version for snap-controller install #1534

pkalever commented Sep 28, 2020 •

edited

pkalever commented Sep 28, 2020

pkalever commented Sep 28, 2020

pkalever commented Sep 28, 2020 •

edited

nixpanic left a comment

Madhu-1 commented Sep 29, 2020

Madhu-1 Sep 29, 2020

pkalever Sep 29, 2020

pkalever commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

pkalever commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

agarwal-mudit commented Sep 29, 2020

nixpanic left a comment

pkalever commented Oct 1, 2020

agarwal-mudit commented Oct 1, 2020

Madhu-1 commented Oct 1, 2020

pkalever commented Oct 5, 2020 •

edited

Madhu-1 commented Oct 7, 2020

pkalever commented Oct 12, 2020

Madhu-1 commented Oct 12, 2020

nixpanic Oct 12, 2020

pkalever Oct 12, 2020

nixpanic Oct 12, 2020

pkalever Oct 12, 2020

pkalever commented Oct 14, 2020

e2e: defend on k8s version for snap-controller install #1534

e2e: defend on k8s version for snap-controller install #1534

Conversation

pkalever commented Sep 28, 2020 • edited

Describe what this PR does

pkalever commented Sep 28, 2020

pkalever commented Sep 28, 2020

pkalever commented Sep 28, 2020 • edited

nixpanic left a comment

Choose a reason for hiding this comment

Madhu-1 commented Sep 29, 2020

Madhu-1 Sep 29, 2020

Choose a reason for hiding this comment

pkalever Sep 29, 2020

Choose a reason for hiding this comment

pkalever commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

pkalever commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

Madhu-1 commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

pkalever commented Sep 29, 2020

agarwal-mudit commented Sep 29, 2020

nixpanic left a comment

Choose a reason for hiding this comment

pkalever commented Oct 1, 2020

agarwal-mudit commented Oct 1, 2020

Madhu-1 commented Oct 1, 2020

pkalever commented Oct 5, 2020 • edited

Madhu-1 commented Oct 7, 2020

pkalever commented Oct 12, 2020

Madhu-1 commented Oct 12, 2020

nixpanic Oct 12, 2020

Choose a reason for hiding this comment

pkalever Oct 12, 2020

Choose a reason for hiding this comment

nixpanic Oct 12, 2020

Choose a reason for hiding this comment

pkalever Oct 12, 2020

Choose a reason for hiding this comment

pkalever commented Oct 14, 2020

pkalever commented Sep 28, 2020 •

edited

pkalever commented Sep 28, 2020 •

edited

pkalever commented Oct 5, 2020 •

edited