[Test] delete kind cluster after tests run

[mucahitkurt]

Delete kind cluster after tests run. Inside a real Prow job it is better to clean up at runtime instead of leaving that to the Prow job cleanup code because the later sometimes times out.

Related issue is kubernetes-csi/csi-release-tools#24

[k8s-ci-robot]

@mucahitkurt: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mucahitkurt
To complete the pull request process, please assign saad-ali
You can assign the PR to them by writing /assign @saad-ali 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:

• release-tools/OWNERS

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

[pohly]

Can you explain why you want this changed?

The prow.sh script intentionally doesn't clean up after itself. When it's used inside a Prow job, deleting that job should clean up. When it's used on a developer's machine, then keeping everything running simplifies debugging in case of a failure. Running the script again will then destroy the cluster and start anew, so it's not leaving an unbounded number of clusters behind.

I had once contemplated to write a wrapper script around prow.sh which takes care of setting up a temporary GOPATH (which does get modified by the script!) and properly cleans up afterwards. That makes sense to me, but not trying to clean up inside the script itself (because it isn't always needed or desired).

[mucahitkurt]

Can you explain why you want this changed?

The prow.sh script intentionally doesn't clean up after itself. When it's used inside a Prow job, deleting that job should clean up. When it's used on a developer's machine, then keeping everything running simplifies debugging in case of a failure. Running the script again will then destroy the cluster and start anew, so it's not leaving an unbounded number of clusters behind.

I had once contemplated to write a wrapper script around prow.sh which takes care of setting up a temporary GOPATH (which does get modified by the script!) and properly cleans up afterwards. That makes sense to me, but not trying to clean up inside the script itself (because it isn't always needed or desired).

@pohly I opened the PR because of the issue kubernetes-csi/csi-release-tools#24. I got the issue as not cleaning cluster after test run can cause timeout for the test jobs.

Btw this is the test PR, actual PR is kubernetes-csi/csi-release-tools#46.

[pohly]

kubernetes-csi/csi-release-tools#24 (comment) explains why this change is needed. This is information that better should better be copied into the commit message and the PR description because then it is available when reviewers like me need it. I wasn't aware of that issue.

For csi-release-tools changes it is better to first discuss and finalize the review in one of the components, then create the PR in csi-release-tools. That way there's no confusion about where to discuss the change and if changes are needed, you don't need to update the csi-release-tools PR potentially multiple times.

[pohly]

Can we make this conditional and only do it in the CI? At least I sometimes run this script manually and prefer to keep the cluster around for debugging.

We could turn this into:

# Inside a real Prow job it is better to clean up at runtime
# instead of leaving that to the Prow job cleanup code
# because the later sometimes times out (https://github.com/kubernetes-csi/csi-release-tools/issues/24#issuecomment-554765872).
if [ "$JOB_NAME" ]; then
    delete_cluster
fi

[mucahitkurt]

Fixed! I assume you don't want the same thing for tests_need_alpha_cluster?

[pohly]

Sorry, I do. I just didn't point it out explicitly.

[mucahitkurt]

I moved the control to delete_cluster method and renamed it to delete_cluster_inside_prow_job.

[mucahitkurt]

kubernetes-csi/csi-release-tools#24 (comment) explains why this change is needed. This is information that better should better be copied into the commit message and the PR description because then it is available when reviewers like me need it. I wasn't aware of that issue.

For csi-release-tools changes it is better to first discuss and finalize the review in one of the components, then create the PR in csi-release-tools. That way there's no confusion about where to discuss the change and if changes are needed, you don't need to update the csi-release-tools PR potentially multiple times.

Ok. Thanks for the heads up!

[k8s-ci-robot]

@mucahitkurt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-csi-driver-host-path-unit	7fd2c42	link	/test pull-kubernetes-csi-csi-driver-host-path-unit

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.

[pohly]

Looks good to me, please update the csi-release-tools PR and we can merge it there.

I can then kick of a round of repo updates to merge that into the different sidecars.

[mucahitkurt]

Looks good to me, please update the csi-release-tools PR and we can merge it there.

I can then kick of a round of repo updates to merge that into the different sidecars.

@pohly I updated the csi-release-tools PR, thank you!