You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/03/18 07:33:44 UTC
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 opened a new pull request
#84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
TaoYang526 opened a new pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84
Please refer to issue: https://issues.apache.org/jira/browse/YUNIKORN-36
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-615478461
BTW, this approach is following the doc: https://kubernetes.io/docs/tasks/administer-cluster/access-cluster-api/#without-using-a-proxy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-613904701
Thanks @yangwwei for the feedback about the URL of APIServer.
I discovered that there's something wrong in the former image which may cause DNS can't resolve the service address correctly in search domains defined in /etc/resolv.conf, that's why it had to use the full service name with suffix '.cluster.local' to connect with API server.
Latest commit replaces the incorrect curl image with official curl image, and update the URL of APIServer with non-suffix service name: 'kubernetes.default.svc' just as described in the kubernetes official doc: https://kubernetes.io/docs/tasks/administer-cluster/access-cluster-api/#accessing-the-kubernetes-api.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm
chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#discussion_r409174553
##########
File path: helm-charts/yunikorn/templates/cleanup.yaml
##########
@@ -0,0 +1,38 @@
+apiVersion: batch/v1
+kind: Job
+metadata:
+ name: yunikorn-cleanup
+ namespace: {{ .Release.Namespace }}
+ annotations:
+ "helm.sh/hook": post-delete
+ "helm.sh/hook-weight": "3"
+ "helm.sh/hook-delete-policy": hook-succeeded
+ labels:
+ app: yunikorn
+ release: {{ .Release.Name }}
+spec:
+ template:
+ metadata:
+ name: yunikorn-cleanup
+ labels:
+ app: yunikorn
+ release: {{ .Release.Name }}
+ spec:
+ serviceAccountName: {{ .Values.serviceAccount }}
+ containers:
+ - name: curl
+ image: "{{ .Values.curl_image.repository }}:{{ .Values.curl_image.tag }}"
+ imagePullPolicy: {{ .Values.curl_image.pullPolicy }}
+ command:
+ - /bin/sh
+ - -c
+ - >
+ APISERVER=https://kubernetes.default.svc;
+ SERVICEACCOUNT=/var/run/secrets/kubernetes.io/serviceaccount;
+ NAMESPACE=$(cat ${SERVICEACCOUNT}/namespace);
+ TOKEN=$(cat ${SERVICEACCOUNT}/token);
+ CACERT=${SERVICEACCOUNT}/ca.crt;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/api/v1/namespaces/${NAMESPACE}/configmaps/yunikorn-configs;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/api/v1/namespaces/${NAMESPACE}/serviceaccounts/yunikorn-admin;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/apis/rbac.authorization.k8s.io/v1beta1/clusterrolebindings/yunikorn-rbac;
+ restartPolicy: OnFailure
Review comment:
I feel we don't need to restart this on failure, if the execution failed, probably the second attempt will fail too.
Can we change this to Never?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-614437204
Thanks @yangwwei for the feedback.
> When I delete the helm-chart second time, the job fails because the job (with that name) already exists.
Do you mean that "helm uninstall" command was executed again before the uninstall process was done? I think it's ok since the first execution couldn't be affected by the second one, right?
> when the job finished, the job is not deleted and the cleanup pod is left-over.
I haven't seen this happen, it should be cleaned up after the job is done since we have configured `"helm.sh/hook-delete-policy": hook-succeeded` in cleanup.yaml. Could you please elaborate how this may happen?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the
helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#discussion_r409229403
##########
File path: helm-charts/yunikorn/templates/cleanup.yaml
##########
@@ -0,0 +1,38 @@
+apiVersion: batch/v1
+kind: Job
+metadata:
+ name: yunikorn-cleanup
+ namespace: {{ .Release.Namespace }}
+ annotations:
+ "helm.sh/hook": post-delete
+ "helm.sh/hook-weight": "3"
+ "helm.sh/hook-delete-policy": hook-succeeded
+ labels:
+ app: yunikorn
+ release: {{ .Release.Name }}
+spec:
+ template:
+ metadata:
+ name: yunikorn-cleanup
+ labels:
+ app: yunikorn
+ release: {{ .Release.Name }}
+ spec:
+ serviceAccountName: {{ .Values.serviceAccount }}
+ containers:
+ - name: curl
+ image: "{{ .Values.curl_image.repository }}:{{ .Values.curl_image.tag }}"
+ imagePullPolicy: {{ .Values.curl_image.pullPolicy }}
+ command:
+ - /bin/sh
+ - -c
+ - >
+ APISERVER=https://kubernetes.default.svc;
+ SERVICEACCOUNT=/var/run/secrets/kubernetes.io/serviceaccount;
+ NAMESPACE=$(cat ${SERVICEACCOUNT}/namespace);
+ TOKEN=$(cat ${SERVICEACCOUNT}/token);
+ CACERT=${SERVICEACCOUNT}/ca.crt;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/api/v1/namespaces/${NAMESPACE}/configmaps/yunikorn-configs;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/api/v1/namespaces/${NAMESPACE}/serviceaccounts/yunikorn-admin;
+ curl --cacert ${CACERT} --header "Authorization: Bearer ${TOKEN}" -X DELETE ${APISERVER}/apis/rbac.authorization.k8s.io/v1beta1/clusterrolebindings/yunikorn-rbac;
+ restartPolicy: OnFailure
Review comment:
Make sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-615477996
I agree this sounds good to me.
We can suggest people use helm3, for which I think most people are already doing that. Let's track the doc changes in another ticket. This one looks good to go. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-614423687
I did some tests, install/delete helm chart again and again.
I found an issue we need to fix. When I delete the helm-chart second time, the job fails because the job (with that name) already exists. I think we can fix this by
```
- name: yunikorn-cleanup
+ generateName: yunikorn-cleanup
```
so every time the job will have a different name.
Another thing is when the job finished, the job is not deleted and the cleanup pod is left-over. Can we take a look at if https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/#ttl-mechanism-for-finished-jobs can help the cleanup?
Other than these issues, it looks good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-603709071
Thanks @wilfred-s , @yangwwei for the review.
> Can we leverage the yunikorn scheduler docker image which already has the script installed, we can do `admission_util.sh delete ` to clean up everything
I think we can't do that since the command will be called in PreStop hook which is called immediately before a container is terminated due to an API request or management event such as liveness probe failure, preemption, resource contention and others, not just before the final termination. (Please refer to https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/#container-hooks)
I haven't find a proper image to replace deprecated hyperkube image after a quick search, I'll find another image or another way to solve this problem. Please share your thoughts if you have any idea about this. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-611931411
Hi, @yangwwei @wilfred-s
This PR has been updated to clean up things via curling to APIServer instead of relying on hyperkube which is removed from k8s base images, please help to review it again. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull
request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm
chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#discussion_r409175682
##########
File path: helm-charts/yunikorn/templates/cleanup.yaml
##########
@@ -0,0 +1,38 @@
+apiVersion: batch/v1
+kind: Job
+metadata:
+ name: yunikorn-cleanup
+ namespace: {{ .Release.Namespace }}
+ annotations:
+ "helm.sh/hook": post-delete
+ "helm.sh/hook-weight": "3"
Review comment:
My understanding the weight is giving us a determinable order when installing resources.
But why we have the weight=3 for the cleanup?
This is a post-delete hook, which only triggers when we delete the helm chart, correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on a change in
pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the
helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#discussion_r409229296
##########
File path: helm-charts/yunikorn/templates/cleanup.yaml
##########
@@ -0,0 +1,38 @@
+apiVersion: batch/v1
+kind: Job
+metadata:
+ name: yunikorn-cleanup
+ namespace: {{ .Release.Namespace }}
+ annotations:
+ "helm.sh/hook": post-delete
+ "helm.sh/hook-weight": "3"
Review comment:
Yes, absolutely right. I will remove this annotation. Thanks for this reminder.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 edited a comment on issue
#84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 edited a comment on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-615073065
After discussed with @yangwwei offline, it seems that `"helm.sh/hook-delete-policy": hook-succeeded` config may not works well in helm v2 (at least for several specific versions: https://github.com/helm/helm/issues/1769), we can assume that it can work well in helm v3 and I have already tested it for many times in my local environment.
Also, I have tested adding generateName for cleanup job but failed to uninstall since it's not available in helm(https://github.com/helm/helm/issues/3348), moreover TTL may not work well unless that both kube-apiserver and kube-controller-manager enable feature gate TTLAfterFinished according to https://kubernetes.io/docs/concepts/workloads/controllers/ttlafterfinished/.
Now helm v3 is the stable and default version according to helm official website(https://helm.sh/docs/), and there are lots of changes between v2 and v3, we decided to do nothing more for this PR and update helm commands based on helm v3 in the User Guide document via another PR in core project: https://github.com/apache/incubator-yunikorn-core/pull/129
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-603028229
I don't think that relying on hyperkube is the right thing as kubernetes has just merged the changes to stop releasing the hyperkube container image [kubernetes/kubernetes#88676](https://github.com/kubernetes/kubernetes/pull/88676)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-603562176
Hi @TaoYang526
Can we leverage the yunikorn scheduler docker image which already has the script installed, we can do
```
admission_util.sh delete
```
to clean up everything
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-615073065
After discussed with @yangwwei offline, it seems that `"helm.sh/hook-delete-policy": hook-succeeded` config may not works well in helm v2 (at least for several specific versions: https://github.com/helm/helm/issues/1769), we can assume that it can work well in helm v3 and I have already tested it for many times in my local environment.
Also, I have tested to add generateName but fails to uninstall since it's not available in helm(https://github.com/helm/helm/issues/3348), and TTL may not work well unless that both kube-apiserver and kube-controller-manager enable feature gate TTLAfterFinished according to https://kubernetes.io/docs/concepts/workloads/controllers/ttlafterfinished/.
Now helm v3 is the stable and default version according to helm official website(https://helm.sh/docs/), and there are lots of changes between v2 and v3, we decided to do nothing more for this PR and update helm commands based on helm v3 in the User Guide document via another PR in core project: https://github.com/apache/incubator-yunikorn-core/pull/129
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-yunikorn-k8shim] TaoYang526 commented on issue #84:
[YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on issue #84: [YUNIKORN-36] Cleanup leaked resources when deleting the helm chart
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/84#issuecomment-614370277
Thanks @yangwwei for the review.
> With this patch, does it mean we can delete all yunikorn created resources by a simple "helm delete" command? do we still need the pre-stop hook?
We can clean up all resources that yunikorn created by "helm uninstall" command. However, I think the pre-stop hook is more suitable to do that for admission-controller, since the resources created for admission-controller should better be managed by admission-controller itself, otherwise if these resources changed we may need to update cleanup.yaml for helm-charts as well as admission_util.sh script for admission-controller. Thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services