added a destroy-script for the resources installed #216

aayushrangwala · 2020-11-09T20:01:09Z

added labels to the resources

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Need a cleanup script to delete all the installed resources

Which issue(s) this PR fixes:
Fixes #212

Does this PR introduce a user-facing change?:
NONE

All the csi resources from k8s 1.17 will be having a common label `partof: csi-hostpath-driver` which will be used to collectively identify all the resources under csi installation and also can be used to delete them in bulk

k8s-ci-robot · 2020-11-09T20:01:17Z

Hi @aayushrangwala. 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.

aayushrangwala · 2020-11-09T20:49:22Z

/cc @pohly

pohly · 2020-11-10T10:04:45Z

deploy/util/destroy-hostpath.sh

+
+BASE_DIR=$(dirname "$0")
+
+LABELS=(csi-snapshotter csi-hostpathplugin csi-attacher csi-resizer csi-hostpath-driver csi-provisioner csi-hostpath-socat)


The labels have to be specific to the hostpath deployment. We cannot risk deleting something that may be part of some other CSI driver deployment.

The setup script has to add those labels instead of assuming that the sidecar RBAC objects already have certain labels. When the script was originally written, kustomize was still new. It might be possible to rely on the kustomize in kubectl >= 1.17 to add labels. Even the image selection might now be done that way.

Right. Makes more sense. I believe using kustomize across the project is in the scope of another ticket. But I will note your point and change the labels and maybe create a new pr for the kustomize

Resolved them. Please re-review

pohly · 2020-11-16T10:18:16Z

.gitignore

+# Output of the go coverage tool, specifically when used with LiteIDE
+*.out
+
+# Dependency directories (remove the comment below to include it)


In your next push, please also clean up your commit:

don't change .gitignore

use a commit message that has a subject line and a body (separated by blank line) which explains what this is about and why the change is relevant

@aayushrangwala do you want to finish this PR?

@pohly Yeah, I will wrap this up. Got stuck in something completely unavoidable

cleaned commit

pohly · 2021-04-26T16:07:09Z

deploy/util/deploy-hostpath.sh

+
+    if [[ "${current}" =~ ^http:// ]] || [[ "${current}" =~ ^https:// ]]; then
+      curl "${current}" --output rbac.yaml --silent --location
+    fi


... and this rbac.yaml gets used where?

Please add a comment about why downloading that file with curl is necessary.

My bad. Skipped one commit to push. Will add the comment as well and push again

pohly

Can you also update deploy/kubernetes-distributed?

The deploy.sh script there is a fork, but your changes apply with some offsets:

$ git diff origin/master..HEAD deploy/util/deploy-hostpath.sh | patch deploy/kubernetes-distributed/deploy.sh
patching file deploy/kubernetes-distributed/deploy.sh
Hunk #1 succeeded at 13 with fuzz 2 (offset -14 lines).
Hunk #2 succeeded at 137 with fuzz 1 (offset -31 lines).

pohly · 2021-04-27T08:38:34Z

deploy/kubernetes-1.20/hostpath/csi-hostpath-plugin.yaml

+    app.kubernetes.io/instance: hostpath.csi.k8s.io
+    app.kubernetes.io/part-of: csi-driver-host-path
+    app.kubernetes.io/name: csi-hostpathplugin
+    app.kubernetes.io/component: plugin


Those labels also should be set inside the PodSpec, otherwise kubectl get all -l app.kubernetes.io/instance=hostpath.csi.k8s.io returns an incomplete result.

I think it is okay to use the same key/value pairs as for the StatefulSet itself.

The same change is needed also for the other StatefulSets and DaemonSets.

Yeah makes sense it will make the child pods for the app object distinguishable

pohly · 2021-04-27T08:40:34Z

deploy/util/deploy-hostpath.sh

@@ -164,7 +168,30 @@ for component in CSI_PROVISIONER CSI_ATTACHER CSI_SNAPSHOTTER CSI_RESIZER CSI_EX
        echo "Using non-default RBAC rules for $component. Changes from $original to $current are:"
        diff -c <(wget --quiet -O - "$original") <(if [[ "$current" =~ ^http ]]; then wget --quiet -O - "$current"; else cat "$current"; fi) || true
    fi
-    run kubectl apply -f "${current}"
+
+    echo "adding labels to rbac"


I ran this script and did not fine this line useful. Can you remove it?

pohly · 2021-04-27T08:42:14Z

deploy/util/deploy-hostpath.sh

+    # since we are deploying rbas directly with the url, the kustomize plugin only works with the local files
+    # we need to add the files locally in temp folder and using kustomize adding labels it will be applied
+    if [[ "${current}" =~ ^http:// ]] || [[ "${current}" =~ ^https:// ]]; then
+      curl "${current}" --output "${TEMP_DIR}"/rbac.yaml --silent --location


More important is to wrap this with run, because then it will be obvious what failed if curl prints error messages.

aayushrangwala · 2021-05-01T17:38:26Z

/retest

aayushrangwala · 2021-05-02T08:37:50Z

/retest

pohly

One last fix and then it should be ready.

Please also squash all commits.

pohly · 2021-05-03T08:53:44Z

deploy/kubernetes-distributed/destroy.sh

+# kubectl maintains a few standard resources under "all" category which can be deleted by using just "kubectl delete all"
+# and other resources such as roles, clusterrole, serivceaccount etc needs to be deleted explicitly
+kubectl delete all --all-namespaces -l app.kubernetes.io/instance=hostpath.csi.k8s.io,app.kubernetes.io/part-of=csi-driver-host-path --wait=true
+kubectl delete role,clusterrole,rolebinding,clusterrolebinding,serviceaccount,storageclass --all-namespaces -l app.kubernetes.io/instance=hostpath.csi.k8s.io,app.kubernetes.io/part-of=csi-driver-host-path --wait=true


csidriver must be added here.

…Driver - added a version specific destroy script parallel to deploy-script - added labels to all the resources of csi deployment along with the podSpec template label-selector - added labels to rbac files on the fly locally in /tmp/* folder to be able to track using kustomize - cleanup gitignore - Useful to cleanup the resources which can interfere with the redeployments or on some automations based on the Driver deployment

pohly

/lgtm
/approve

Thanks for the contribution.

k8s-ci-robot · 2021-05-03T10:21:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aayushrangwala, pohly

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:

~~OWNERS~~ [pohly]

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

6613c39 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae99 Update k8s image repo url 77e47cc Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b Fix dep version mismatch 8f83905 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94d Update go version to 1.20 to match k/k v1.27 e322ce5 Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a512 test: fix golint error aa61bfd Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d19 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171be Merge pull request kubernetes-csi#216 from msau42/process cb98782 Merge pull request kubernetes-csi#217 from msau42/owners a11216e add new reviewers and remove inactive reviewers dd98675 Add step for checking builds git-subtree-dir: release-tools git-subtree-split: 6613c39

Add step for checking builds

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 9, 2020

k8s-ci-robot requested review from jsafrane and saad-ali November 9, 2020 20:01

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 9, 2020

aayushrangwala force-pushed the destroy-script branch 2 times, most recently from 24e6f50 to 2aa6e5d Compare November 9, 2020 20:46

aayushrangwala changed the title ~~- added a destroy-script for the resources installed~~ added a destroy-script for the resources installed Nov 9, 2020

k8s-ci-robot requested a review from pohly November 9, 2020 20:49

pohly requested changes Nov 10, 2020

View reviewed changes

aayushrangwala mentioned this pull request Nov 10, 2020

added labels to the resources to be used for collective operation kubernetes-csi/external-health-monitor#42

Closed

aayushrangwala force-pushed the destroy-script branch from 2aa6e5d to b96a027 Compare November 10, 2020 10:34

pohly reviewed Nov 16, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021

aayushrangwala force-pushed the destroy-script branch from b96a027 to 8ed7de5 Compare April 15, 2021 07:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2021

aayushrangwala force-pushed the destroy-script branch from 8ed7de5 to e3cd8e2 Compare April 15, 2021 07:53

aayushrangwala requested a review from pohly April 15, 2021 07:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 15, 2021

aayushrangwala force-pushed the destroy-script branch 2 times, most recently from 0ab8133 to c063ce5 Compare April 15, 2021 07:58

pohly requested changes Apr 26, 2021

View reviewed changes

aayushrangwala requested a review from pohly April 26, 2021 16:25

pohly requested changes Apr 27, 2021

View reviewed changes

aayushrangwala force-pushed the destroy-script branch from c8ecbb5 to 1656581 Compare May 1, 2021 16:58

aayushrangwala requested a review from pohly May 1, 2021 16:58

aayushrangwala force-pushed the destroy-script branch from 1656581 to cabad3b Compare May 1, 2021 17:20

aayushrangwala force-pushed the destroy-script branch 3 times, most recently from a7b54f3 to 68ac347 Compare May 1, 2021 19:33

aayushrangwala force-pushed the destroy-script branch 5 times, most recently from 80a5cc0 to b0e2723 Compare May 2, 2021 10:27

pohly requested changes May 3, 2021

View reviewed changes

aayushrangwala force-pushed the destroy-script branch from b0e2723 to 03204df Compare May 3, 2021 09:14

aayushrangwala requested a review from pohly May 3, 2021 09:15

pohly approved these changes May 3, 2021

View reviewed changes

k8s-ci-robot assigned pohly May 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2021

k8s-ci-robot merged commit 14ad6d8 into kubernetes-csi:master May 3, 2021

TerryHowe pushed a commit to TerryHowe/csi-driver-host-path that referenced this pull request Oct 17, 2024

Merge pull request kubernetes-csi#216 from msau42/process

a2171be

Add step for checking builds

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a destroy-script for the resources installed #216

added a destroy-script for the resources installed #216

aayushrangwala commented Nov 9, 2020

k8s-ci-robot commented Nov 9, 2020

aayushrangwala commented Nov 9, 2020

pohly Nov 10, 2020

aayushrangwala Nov 10, 2020

aayushrangwala Apr 25, 2021

pohly Nov 16, 2020

pohly Jan 26, 2021

aayushrangwala Apr 15, 2021 •

edited

Loading

aayushrangwala Apr 15, 2021

pohly Apr 26, 2021

pohly Apr 26, 2021

aayushrangwala Apr 26, 2021

pohly left a comment

pohly Apr 27, 2021

aayushrangwala May 1, 2021

pohly Apr 27, 2021

aayushrangwala May 1, 2021

pohly Apr 27, 2021

aayushrangwala May 1, 2021

aayushrangwala commented May 1, 2021

aayushrangwala commented May 2, 2021

pohly left a comment

pohly May 3, 2021

aayushrangwala May 3, 2021

pohly left a comment

k8s-ci-robot commented May 3, 2021


		BASE_DIR=$(dirname "$0")

		LABELS=(csi-snapshotter csi-hostpathplugin csi-attacher csi-resizer csi-hostpath-driver csi-provisioner csi-hostpath-socat)

added a destroy-script for the resources installed #216

added a destroy-script for the resources installed #216

Conversation

aayushrangwala commented Nov 9, 2020

k8s-ci-robot commented Nov 9, 2020

aayushrangwala commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayushrangwala Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayushrangwala commented May 1, 2021

aayushrangwala commented May 2, 2021

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 3, 2021

aayushrangwala Apr 15, 2021 •

edited

Loading