You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "shrprasa (via GitHub)" <gi...@apache.org> on 2023/02/22 19:40:53 UTC
[GitHub] [spark] shrprasa opened a new pull request, #40128: [SPARK-42466][Core][K8S]: Cleanup k8s upload directory when job terminates
shrprasa opened a new pull request, #40128:
URL: https://github.com/apache/spark/pull/40128
### What changes were proposed in this pull request?
1. Instead of creating multiple sub directories in k8s upload directory (spark.kubernetes.file.upload.path) for each file to be uploaded, create a single subdirectory to be used for all file uploads of a specific application. This directory will be named using the spark application id.
2. Delete the sub directory and it's content when job terminates
### Why are the changes needed?
The change is required to cleanup the files and directories which are created under the k8s upload path to prevent space getting full. without this change, user needs to manually clean up these files.
### Does this PR introduce _any_ user-facing change?
Yes, Users submitting spark on k8s job, won't need to manually cleanup the files under upload directory.
### How was this patch tested?
Through git action and Manually by running Spark on K8s jobs.
Some logs for verification:
23/02/22 18:41:16 INFO SparkContext: Successfully stopped SparkContext
23/02/22 18:41:16 INFO ShutdownHookManager: Shutdown hook called
23/02/22 18:41:16 INFO ShutdownHookManager: Deleting directory /spark-local2/spark-f58ecb91-0bb6-4b10-a372-405f69780c15
23/02/22 18:41:16 INFO ShutdownHookManager: Deleting directory /tmp/spark-c10b27ca-fe68-4382-913f-4a535833f64e
23/02/22 18:41:16 INFO ShutdownHookManager: Deleting directory /spark-local1/spark-8c4ef9d8-7688-4f51-b587-16dee1d264c7
23/02/22 18:41:16 INFO UploadDirManager: Shutdown hook called
23/02/22 18:41:17 INFO UploadDirManager: Upload dir deleted successfully.: hdfs://****:8020/user/*****/k8s/spark-upload-b6609808729a49e9b6f53f20ed50d05b
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1579198508
Gentle Ping @dongjoon-hyun
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1124885459
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
This is a weird place to register a shutdown hook, why not just register earlier instead of inside all of these exception cases?
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
I wonder if we can share logic here with YARN? Since I imagine Spark on YARN also needs to cleanup these jars.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yb12138 commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "yb12138 (via GitHub)" <gi...@apache.org>.
yb12138 commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1144285572
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
Why uploadDirManager not placed in KubernetesClientApplication? When driver pod status is failed, you can delete k8s upload directory.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1463227858
Gentle ping @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1124899927
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
The staging directory provided by spark.kubernetes.file.upload.path is specific to K8s. Though Yarn has already got logic to cleanup it's staging directory. That is present in ApplicationMaster.scala.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
URL: https://github.com/apache/spark/pull/40128
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1144786744
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
The reason it's not put in k8s module is because cleanup can happen in two cases:
1. Spark app submission fails and clean up is required -> This is handled through KubernetesClientApplication.
2. Spark app submission succeeds and driver is launched in client mode. Clean up is required when job finishes -> This is handled in SparkSumbit, which is not in k8s module. This scenario can't be handled in KubernetesClientApplication as it's not called on the driver side.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1472394127
Gentle ping @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1453938288
Gentle ping @dongjoon-hyun @holdenk @srowen
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1486231326
Gentle Ping @dongjoon-hyun @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1144467202
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
It is being registered in KubernetesClientApplication also.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1133449379
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
ah interesting. What about if we only register in client mode?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1465517975
Gentle ping @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1457586866
gentle ping @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1480402673
@shrprasa .
1. It seems that you have an assumption that Shutdown hook is magically reliable. However, shutdown hook has a well-known limitation where JVM can be destroyed abruptly and K8s Pod can be deleted also without giving the inside processes enough time to handle business logic.
2. As I mentioned in the above, public cloud storage systems have a better and complete TTL-based solution for that issue. In that context, this PR is only trying to mitigate HDFS issue, https://issues.apache.org/jira/browse/HDFS-6382, partially.
> The change to clean up the upload directory is not specific to HDFS. The reason we should do cleanup is because if the spark job is creating new directories/files, it should clean them up too just like it's being done in YARN and also for other files like shuffle spill.
Also, can you please explain why the approach seems to be incomplete? How is unable to prevent leftover from upload directory?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1480495550
@dongjoon-hyun Thanks for the clarification. But the unreliability for shutdown hook is common for all other shutdown tasks also. This doesn't mean we haven't impletened them. So, why this should stop us from adding the clean up logic.
The TTL-based solution is specific to the filesystem. It's nice to have but I don't understand why we should rely on external solution to perform cleanup on behalf of spark job and why not the job itself does the cleanup? What's the downside of doing this?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1482363384
Gentle Ping @dongjoon-hyun @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1517271816
@dongjoon-hyun @holdenk Can we have some conclusion on this issue?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1128983339
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
@holdenk do you have any suggestion to handle this in a better way?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1487537833
So as I think about it, what about if we did a cleanup as a script for the driver pod on terminate to address some of @dongjoon-hyun's concern?
The TTL based approach feels fundamentally "better" though. Certainly it is filesystem specific, but I think (besides) HDFS all of the major filesystems we would use support TTL yes? Perhaps we should just document this?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yb12138 commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "yb12138 (via GitHub)" <gi...@apache.org>.
yb12138 commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1144505024
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
I mean, this cleanup is only for the kubernetes.It will be clearer to put all in the k8s module.You can tell by the status of driver pod if a cleanup is needed.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1476234703
Gentle ping @holdenk
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1133671423
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
Client mode will not help as in that case cleanup will be missed if job submission fails for cluster deploy mode.
We are doing some other resource cleanup activities in the catch blocks of this class. So, registering the upload path cleanup should also be ok.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1124903174
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
The reason we need to register only in exception cases is that the cleanup of files and directory under file upload path should happen on client side only if job submission fails. If we register early, and job is submitted with
spark.kubernetes.submission.waitAppCompletion conf value as false, shutdown hook will be called as soon as app submission completes and the files will be deleted. This will case the job to fail as driver will not be able to find these files.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1124903174
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
The reason we need to register only in exception cases is that the cleanup of files and directory under file upload path should happen on client side only if job submission fails. If we register early, and job is submitted with
spark.kubernetes.submission.waitAppCompletion conf value as false, shutdown hook will be called as soon as app submission completes and the files will be deleted. This will result into job failure as driver will not be able to find these files.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1124903174
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -143,6 +144,9 @@ private[spark] class Client(
logError("Please check \"kubectl auth can-i create [resource]\" first." +
" It should be yes. And please also check your feature step implementation.")
kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ // register shutdownhook for cleaning up the upload dir only
Review Comment:
The reason we need to register only in exception cases is that the cleanup of files and directory under file upload path should happen on client side only if job submission fails. If we register early, and job is submitted with
spark.kubernetes.submission.waitAppCompletion conf value as false, shutdown hook will be called as soon as app submission completes and the files will be deleted. This will job failure as driver will not be able to find these files.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][Core][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1444968969
@holdenk @dongjoon-hyun Can you please review this PR?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on a diff in pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on code in PR #40128:
URL: https://github.com/apache/spark/pull/40128#discussion_r1144786744
##########
core/src/main/scala/org/apache/spark/UploadDirManager.scala:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import org.apache.hadoop.fs.{FileSystem, Path}
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.Logging
+import org.apache.spark.util.{ShutdownHookManager, Utils}
+
+/**
+ * Kubernetes upload directory manager. It's responsible for cleanup of the upload directory
+ * when spark job finishes.
+ */
+private[spark] class UploadDirManager(conf: SparkConf, appId: String) extends Logging {
Review Comment:
The reason it's not put in k8s module is because cleanup can happen in two cases:
1. Spark app submission fails and clean up is required -> THis is handled through KubernetesClientApplication.
2. Spark app submission succeeds and driver is launched in client mode. Clean up is required when job finishes -> This is handled in SparkSumbit, which is not in k8s module. This scenario can't be handled in KubernetesClientApplication as it's not called on the driver side.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1480215990
Hi @dongjoon-hyun
The change to clean up the upload directory is not specific to HDFS. The reason we should do cleanup is because if the spark job is creating new directories/files, it should clean them up too just like it's being done in YARN and also for other files like shuffle spill.
Also, can you please explain why the approach seems to be incomplete? How is unable to prevent leftover from upload directory?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1720315092
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1579579865
Sorry but I'm still in the same position, @shrprasa .
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1449406133
@dongjoon-hyun @holdenk Can you please review this PR?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1492818493
@holdenk @dongjoon-hyun TTL based clearnce has limitations as pointed out in this comment
https://github.com/apache/spark/pull/40363#issuecomment-1467439787
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] holdenk commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1517285071
I'm a soft +1, I think it's a useful incremental feature (even if imperfect) but if @dongjoon-hyun would rather not then I defer to his judgement here (I mostly run with object storage where TTLs are fine).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shrprasa commented on pull request #40128: [SPARK-42466][K8S]: Cleanup k8s upload directory when job terminates
Posted by "shrprasa (via GitHub)" <gi...@apache.org>.
shrprasa commented on PR #40128:
URL: https://github.com/apache/spark/pull/40128#issuecomment-1532589815
@dongjoon-hyun can you please comment on whether we should be doing this change or not.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org