You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/04 15:59:22 UTC
[GitHub] [spark] Yikun opened a new pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Yikun opened a new pull request #35733:
URL: https://github.com/apache/spark/pull/35733
### What changes were proposed in this pull request?
Cleanup pod resources after queue test.
### Why are the changes needed?
Test pods with custom group label are not be cleanup after each test completed. Especially, when user specified
### Does this PR introduce _any_ user-facing change?
NO, K8S IT only.
### How was this patch tested?
```
$ build/sbt -Pvolcano -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r -Dtest.include.tags=volcano -Dspark.kubernetes.test.namespace=default "kubernetes-integration-tests/test"
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (13 seconds, 172 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enable) (17 seconds, 409 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enable) (27 seconds, 448 milliseconds)
$ k get pod
No resources found in default namespace.
```
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think I need to split pod deleted and yaml resource cleanup deleted into two PRs for easy review?
The pod deleted might more urgent, and yaml resource cleanup only address the case when test case failed we could address later.
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think we need to split pod deleted and yaml resource cleanup deleted into two PRs for easy review?
The pod deleted might more urgent, and yaml resource cleanup only address the case when test case failed we could address later.
--
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 closed pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35733:
URL: https://github.com/apache/spark/pull/35733
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059715420
@martin-g Thanks, will address this tonight.
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think I need to split **pod cleanup** and **yaml resource cleanup** into two PRs for easy review?
The pod deleted might more urgent, and yaml resource cleanup only address the case when test case failed we could address later.
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun If you think I need to split **pod cleanup** and **yaml resource cleanup** into two PRs for easy review, feel free to ping me. Otherwise, it would be good if you could take a look when you get some time, Thanks!
Note that: if we don't specified `-Dspark.kubernetes.test.namespace=` these resources can be cleanup because we delete the namespace, all resources in random namespace can be cleanup. (That's also why I didn't found this before)
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059622648
@dongjoon-hyun Thanks for suggestion!
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think I need to split **pod cleanup** and **yaml resource cleanup** into two PRs for easy review?
The pod deleted might more urgent, and yaml resource cleanup refactor only address the case when test case failed we could address later.
--
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] Yikun commented on a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820300284
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
Review comment:
Actually, I make testGroup clear back in the beforeEach, I guess it's already clear for each test?
But I think it's fine for me to move set clear operation into every cleanup api call completed.
--
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] Yikun commented on a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820300284
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
Review comment:
Actually, I make testGroup clear back in the beforeEach, I guess it's already clear for each test? or did I missed something?
But I think it's fine for me to move set clear operation into every cleanup api call completed, it makes every deleted more separately.
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think I need to split **pod cleanup** and **yaml resource cleanup** into two PRs for easy review?
Note that: if we don't specified `-Dspark.kubernetes.test.namespace=` these resources can be cleanup because we delete the namespace, all resources in random namespace can be cleanup. (That's also why I didn't found this before)
--
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 a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820272123
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
Review comment:
This function should make `testGroups` empty back because this is called at `afterEach`. Otherwise, the previous test case's leftover group name will remain and we are going to try to delete it again and again in the subsequent test cases.
--
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] Yikun commented on a change in pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820160101
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ testGroups = mutable.Set.empty
+ testYAMLPaths = mutable.Set.empty
+ super.beforeEach()
Review comment:
done. : )
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
--
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 a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820303391
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ super.beforeEach()
+ testGroups = mutable.Set.empty
Review comment:
Yes, we need to have these inside `afterEach`.
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ super.beforeEach()
+ testGroups = mutable.Set.empty
Review comment:
Yes, we need to move these inside `afterEach`.
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ super.beforeEach()
+ testGroups = mutable.Set.empty
Review comment:
Yes, we need to move these into `afterEach`.
--
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] martin-g commented on a change in pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820060556
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -173,19 +205,20 @@ private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
}
test("SPARK-38188: Run SparkPi jobs with 2 queues (all enable)", k8sTestTag, volcanoTag) {
Review comment:
```suggestion
test("SPARK-38188: Run SparkPi jobs with 2 queues (all enabled)", k8sTestTag, volcanoTag) {
```
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,52 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ protected var testGroups: mutable.Set[String] = _
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.map{ g =>
+ kubernetesTestComponents.kubernetesClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val size = kubernetesTestComponents.kubernetesClient
+ .pods()
+ .withLabel("spark-app-locator", g)
+ .list().getItems.size()
+ assert(size === 0)
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
Review comment:
it is better to call `super.beforeEach()` here even if it does nothing
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,52 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ protected var testGroups: mutable.Set[String] = _
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.map{ g =>
+ kubernetesTestComponents.kubernetesClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val size = kubernetesTestComponents.kubernetesClient
+ .pods()
+ .withLabel("spark-app-locator", g)
+ .list().getItems.size()
+ assert(size === 0)
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ testGroups = mutable.Set.empty
+ }
+
+ override protected def afterEach(): Unit = {
+ super.afterEach()
+ deletePodInTestGroup()
Review comment:
usually it is better to cleanup this instance's resources and then ask the parent to cleanup its resources. The reverse of what `beforeEach()` does
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059744723
```
$ build/sbt -Pvolcano -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r -Dtest.include.tags=volcano -Dspark.kubernetes.test.namespace=default "kubernetes-integration-tests/testOnly"
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (12 seconds, 202 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enabled) (17 seconds, 398 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enabled) (26 seconds, 452 milliseconds)
[info] Run completed in 59 seconds, 484 milliseconds.
[info] Total number of tests run: 3
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 3, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
$ k get pod
No resources found in default namespace.
```
--
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 #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1060016411
I deleted the following because it's a little misleading. I believe you want to say that 'those are going to be deleted even after test case fails via `afterEach`, but those are not going to be deleted if the test suite itself fail somehow`.
> - Yaml resources are not be deleted if test suite failed
--
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] Yikun removed a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
Yikun removed a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059744723
```
$ build/sbt -Pvolcano -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r -Dtest.include.tags=volcano -Dspark.kubernetes.test.namespace=default "kubernetes-integration-tests/testOnly"
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (12 seconds, 202 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enabled) (17 seconds, 398 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enabled) (26 seconds, 452 milliseconds)
[info] Run completed in 59 seconds, 484 milliseconds.
[info] Total number of tests run: 3
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 3, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
$ k get pod
No resources found in default namespace.
```
--
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 a change in pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820123141
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ testGroups = mutable.Set.empty
+ testYAMLPaths = mutable.Set.empty
+ super.beforeEach()
Review comment:
Could you invoke `super.beforeEach()` before setting `testGroups` and `testYAMLPaths`?
--
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 a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820272182
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
Review comment:
Ditto. testYAMLPaths should be clear in this method.
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059766041
```
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (12 seconds, 208 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enabled) (17 seconds, 297 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enabled) (27 seconds, 296 milliseconds)
[info] Run completed in 1 minute, 1 second.
[info] Total number of tests run: 3
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 3, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 101 s (01:41), completed Mar 5, 2022 9:33:56 PM
$ k get pod
No resources found in default namespace.
$ k get queue
NAME AGE
default 26h
```
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup pod resource after queue test
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059622986
```
$ build/sbt -Pvolcano -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r -Dtest.include.tags=volcano -Dspark.kubernetes.test.namespace=default "kubernetes-integration-tests/testOnly"
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (12 seconds, 200 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enable) (18 seconds, 345 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enable) (28 seconds, 436 milliseconds)
$ k get pod
No resources found in default namespace.
```
--
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] Yikun commented on pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1060116319
```
[info] KubernetesSuite:
[info] VolcanoSuite:
[info] - Run SparkPi with volcano scheduler (12 seconds, 215 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (only 1 enabled) (18 seconds, 361 milliseconds)
[info] - SPARK-38188: Run SparkPi jobs with 2 queues (all enabled) (28 seconds, 378 milliseconds)
[info] Run completed in 1 minute, 2 seconds.
[info] Total number of tests run: 3
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 3, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 248 s (04:08), completed Mar 7, 2022 9:52:03 AM
$ k get pod
No resources found in default namespace.
$ k get queue
NAME AGE
default 2d14h
```
--
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] Yikun edited a comment on pull request #35733: [SPARK-38188][K8S][TEST][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun edited a comment on pull request #35733:
URL: https://github.com/apache/spark/pull/35733#issuecomment-1059905012
cc @dongjoon-hyun Do you think I need to split *pod cleanup* and *yaml resource cleanup* into two PRs for easy review?
The pod deleted might more urgent, and yaml resource cleanup only address the case when test case failed we could address later.
--
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] Yikun commented on a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820301195
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
+ }
+
+ private def deleteYamlResources(): Unit = {
+ testYAMLPaths.foreach { yaml =>
+ deleteYAMLResource(yaml)
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ val resources = k8sClient.load(new FileInputStream(yaml)).fromServer.get.asScala
+ // Make sure all elements are null (no specific resources in cluster)
+ resources.foreach { r => assert(r === null) }
+ }
+ }
+ }
+
+ override protected def beforeEach(): Unit = {
+ super.beforeEach()
+ testGroups = mutable.Set.empty
Review comment:
Looks like it can be deleted if we decided to move the clear back operation into deleted call completed?
cc @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] Yikun commented on a change in pull request #35733: [SPARK-38188][K8S][TESTS][FOLLOWUP] Cleanup resources in `afterEach`
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35733:
URL: https://github.com/apache/spark/pull/35733#discussion_r820300284
##########
File path: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/VolcanoTestsSuite.scala
##########
@@ -29,21 +29,63 @@ import scala.concurrent.Future
import io.fabric8.kubernetes.api.model.Pod
import io.fabric8.kubernetes.client.NamespacedKubernetesClient
import io.fabric8.volcano.client.VolcanoClient
+import org.scalatest.BeforeAndAfterEach
import org.scalatest.concurrent.Eventually
import org.apache.spark.SparkFunSuite
import org.apache.spark.deploy.k8s.Config._
import org.apache.spark.deploy.k8s.features.VolcanoFeatureStep
import org.apache.spark.internal.config.NETWORK_AUTH_ENABLED
-private[spark] trait VolcanoTestsSuite { k8sSuite: KubernetesSuite =>
+private[spark] trait VolcanoTestsSuite extends BeforeAndAfterEach { k8sSuite: KubernetesSuite =>
import VolcanoTestsSuite._
import org.apache.spark.deploy.k8s.integrationtest.VolcanoSuite.volcanoTag
import org.apache.spark.deploy.k8s.integrationtest.KubernetesSuite.{k8sTestTag, INTERVAL, TIMEOUT}
lazy val volcanoClient: VolcanoClient
= kubernetesTestComponents.kubernetesClient.adapt(classOf[VolcanoClient])
lazy val k8sClient: NamespacedKubernetesClient = kubernetesTestComponents.kubernetesClient
+ private var testGroups: mutable.Set[String] = mutable.Set.empty
+ private var testYAMLPaths: mutable.Set[String] = mutable.Set.empty
+
+ private def deletePodInTestGroup(): Unit = {
+ testGroups.foreach { g =>
+ k8sClient.pods().withLabel("spark-group-locator", g).delete()
+ Eventually.eventually(TIMEOUT, INTERVAL) {
+ assert(k8sClient.pods().withLabel("spark-group-locator", g).list().getItems.isEmpty)
+ }
+ }
Review comment:
Actually, I make testGroup clear back in the beforeEach, I guess it's already clear for each test?
But I think it's fine for me to move set clear operation into every cleanup api call.
--
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