[release-2.1] Fix release-tools/prow.sh for K8s 1.20

[ialidzhikov]

/kind bug

This PR backports kubernetes-csi/csi-release-tools@1d60e779 into release-2.1 branch to fix the git clone issue in for pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20.

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @ialidzhikov. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[xing-yang]

/ok-to-test

[xing-yang]

/assign @mattcary
/assign @pohly

[pohly]

The problem in https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1358823154201399296 is a mismatch between the CRD that gets installed (https://github.com/kubernetes-csi/external-snapshotter/tree/release-2.1/config/crd = the one which supports only v1beta1 API) and the API that is expected by the csi-driver-host-path v1.5.0 deployment for 1.20 (https://github.com/kubernetes-csi/csi-driver-host-path/blob/v1.5.0/deploy/kubernetes-1.20/snapshotter/csi-hostpath-snapshotclass.yaml = v1).

Several options:

• keep testing release-2.1 with the older deployment files from csi-driver-host-path v1.4.0 (i.e. the ones for Kubernetes 1.18 = https://github.com/kubernetes-csi/csi-driver-host-path/tree/v1.4.0/deploy/kubernetes-1.18)
• update the CRDs in the release-2.1 branch

To do the former, we can either:

• remove the CSI_PROW_DRIVER_VERSION override from the Prow jobs
• override the override in .prow.sh of the release-2.1 branch

Conceptually, setting CSI_PROW_DRIVER_VERSION in the Prow job isn't necessary because prow.sh also has a default. I think @msau42 introduced it in the Prow config to avoid having to update individual repos.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ialidzhikov
To complete the pull request process, please assign lpabon after the PR has been reviewed.
You can assign the PR to them by writing /assign @lpabon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ialidzhikov]

@pohly , thanks for the help. It would be impossible to me to dig down for the failure reason and came with proposals how to proceed forward.
For now I would go for override the override in .prow.sh of the release-2.1 branch as it seems to be the simplest solution. remove the CSI_PROW_DRIVER_VERSION override from the Prow jobs sounds the much better option but I think that it would require setting the appropriate CSI_PROW_DRIVER_VERSION for each branch (I see that the current master defaults to CSI_PROW_DRIVER_VERSION=v1.3.0 if there is no override).
@pohly , in the latest force-push I added export CSI_PROW_DRIVER_VERSION=v1.4.0 to .prow.sh. I hope this will do the trick. 🙏

[k8s-ci-robot]

@ialidzhikov: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20	830300d	link	/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[mattcary]

/lgtm

This makes sense to me, it's essentially what we've done internally in GKE for testing deployments that are using the beta snapshot crds.

[mattcary]

Hmm, the presubmit is still using the 1.5 prow csi driver version. I'm not sure how to fix that.

I guess it's a little weird we're testing the 2.1 release branch on 1.20 at all. We should only be testing >= v4.0?

[ialidzhikov]

Hmm, the presubmit is still using the 1.5 prow csi driver version. I'm not sure how to fix that.

Hm, from the raw output of the job I see CSI driver version: CSI_PROW_DRIVER_VERSION=v1.4.0 which makes me think that the override thing works. But I am not sure what is presubmit and why it would still use v1.5.0.

[pohly]

remove the CSI_PROW_DRIVER_VERSION override from the Prow jobs sounds the much better option but I think that it would require setting the appropriate CSI_PROW_DRIVER_VERSION for each branch (I see that the current master defaults to CSI_PROW_DRIVER_VERSION=v1.3.0 if there is no override).

It's part of the TODO list in kubernetes-csi/csi-release-tools#120 - with low priority, because it wouldn't really be used in practice while the Prow job config overrides it.

It's now still failing in https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/471/pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20/1358873015818915840 because the E2E test in Kubernetes 1.20 is used and (I think) that one needs the v1 API.

I think the right fix is to skip testing of Kubernetes 1.20 in the .prow.sh script. Just return immediately. The old release branch was simply not meant to be tested on Kubernetes 1.20. Technically it works when using the v1beta1 API everywhere, but that's not what we recommend for Kubernetes 1.20.

I suggested today in the CSI standup meeting to add such a version check for "unknown" Kubernetes versions in advance. We agreed to not add it because we want to know when testing on a new Kubernetes version breaks. Also, in other repos it works.

I guess this repo is the exception where we have learned that testing on Kubernetes 1.20 doesn't work...

[mattcary]

I think the right fix is to skip testing of Kubernetes 1.20 in the .prow.sh script. Just return immediately. The old release branch was simply not meant to be tested on Kubernetes 1.20. Technically it works when using the v1beta1 API everywhere, but that's not what we recommend for Kubernetes 1.20.
...
I guess this repo is the exception where we have learned that testing on Kubernetes 1.20 doesn't work...

+1, I don't think testing in 1.20 is appropriate.

[ialidzhikov]

I think the right fix is to skip testing of Kubernetes 1.20 in the .prow.sh script. Just return immediately. The old release branch was simply not meant to be tested on Kubernetes 1.20. Technically it works when using the v1beta1 API everywhere, but that's not what we recommend for Kubernetes 1.20.
...
I guess this repo is the exception where we have learned that testing on Kubernetes 1.20 doesn't work...

+1, I don't think testing in 1.20 is appropriate.

Yep, I see that the e2e tests under k/k try to use the v1 api which won't work for release-2.1.
Then I would simply suggest adapting the skip_branches for the pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20 job - ref https://github.com/kubernetes/test-infra/blob/5e490580b8a60705d24c62079a1f65ec1531cf5b/config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml#L186.

[mattcary]

Would it be easier to to just skip in .prow.sh as @pohly suggeted (presumably using CSI_PROW_KUBERNETES_VERSION)? It seems easier to manage per-branch test configuration in the branch rather than having to also track it in the prow config.

[ialidzhikov]

Would it be easier to to just skip in .prow.sh as @pohly suggeted (presumably using CSI_PROW_KUBERNETES_VERSION)? It seems easier to manage per-branch test configuration in the branch rather than having to also track it in the prow config.

I believe with the k/test-infra approach the job won't be displayed at all on the PR (at least this is my assumption). With the artificial skip in each branch, I believe the job will be still visible in the PR and it will be wrongly reported as passed/green when there are no test executions in it. Feel free to correct me if I am wrong. Meanwhile I created kubernetes/test-infra#20792. I hope that it is enough and it will do the trick.

[mattcary]

I believe with the k/test-infra approach the job won't be displayed at all on the PR (at least this is my assumption). With the artificial skip in each branch, I believe the job will be still visible in the PR and it will be wrongly reported as passed/green when there are no test executions in it.

Ah, that's a good point.

Fingers crossed for your test-infra change :-)

[ialidzhikov]

Closing this PR in favour of kubernetes/test-infra#20792.

/close

[k8s-ci-robot]

@ialidzhikov: Closed this PR.

In response to this:

Closing this PR in favour of kubernetes/test-infra#20792.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.