Use a separate class to track components that need to be cleaned up #122

mccheah · 2017-02-16T01:36:20Z

Instead of trying to handle try-catch blocks and keep in mind when objects are edited - instead store the things we want to clean up in a separate place, update the things to clean up whenever we edit them, and remove them if we deem that the component should be persisted beyond the lifetime of the spark-submit process.

Part of #109

mccheah · 2017-02-16T01:36:43Z

This refactor is not strictly dependent on #116 but I based this branch off of that for ease of avoiding merge conflicts later.

ash211

Like the approach -- only minor comments on logging and method naming

ash211 · 2017-02-16T14:01:49Z

...etes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesComponentCleaner.scala

+    registeredIngresses.remove(ingress.getMetadata.getName)
+  }
+
+  def purgeAllRegisteredComponentsFromKubernetes(): Unit = LOCK.synchronized {


purge -> delete (use k8s terminology)

ash211 · 2017-02-16T14:03:33Z

...etes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesComponentCleaner.scala

+    registeredSecrets.remove(secret.getMetadata.getName)
+  }
+
+  def registerOrUpdateIngress(ingress: Ingress): Unit = LOCK.synchronized {


do we have to have this separate LOCK object? We could have a more granular locking and lock on each map when updating that map, rather than the instance-wide LOCK

ash211 · 2017-02-16T14:08:18Z

...etes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesComponentCleaner.scala

+    registeredIngresses.remove(ingress.getMetadata.getName)
+  }
+
+  def purgeAllRegisteredComponentsFromKubernetes(): Unit = LOCK.synchronized {


put an info log line here that we're deleting registered kubernetes components plus the counts of each resource type

mccheah · 2017-02-16T23:39:48Z

This also should make #119 a little easier. We could probably cover it with a unit test, actually.

mccheah · 2017-02-18T01:50:54Z

@aash - I believe this would cover #94 as well.

Create a KubernetesComponentsCleaner which can register arbitrary pods, services, secrets, and ingresses. When an exception is thrown or the JVM shuts down, the cleaner automatically purges any of its registered components from Kubernetes. The components can be unregistered when the driver successfully begins running, so that the application persists beyond the lifetime of the spark-submit process.

…te-incremental' into refactor-submit-cleanup-rebased-2

mccheah · 2017-02-22T23:19:42Z

I changed the base branch fo k8s-support-alternate-incremental since the Ingress support PR won't be merged immediately. I will also target the other refactor PRs to merge into the base.

ash211 · 2017-02-22T23:39:17Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/Client.scala

@@ -107,99 +107,80 @@ private[spark] class Client(

    val k8ClientConfig = k8ConfBuilder.build
    Utils.tryWithResource(new DefaultKubernetesClient(k8ClientConfig)) { kubernetesClient =>
+      val kubernetesComponentCleaner = new KubernetesComponentCleaner(kubernetesClient)


there's a lot of passing around of this local variable. I wonder if it makes more sense to make this instance local, add to it throughout the run and other methods and then at the end of the run method (probably in a finally block) clean everything up. That approach reminds me a lot of a Haskell Monad accumulating state for later processing

ash211 · 2017-02-22T23:41:02Z

Totally agree with separating this from the Ingress PR -- let's get the refactoring PRs in separately since they're much less controversial

ash211 · 2017-02-22T23:55:12Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/Client.scala

-        (Array[EnvVar], Array[Volume], Array[VolumeMount], Array[Secret]) = {
+  private def configureSsl(
+      kubernetesClient: KubernetesClient,
+      kubernetesComponentCleaner: KubernetesComponentCleaner,


do we still need this as a parameter vs using the instance variable?

ash211

I'm comfortable merging when the build is green

mccheah · 2017-02-23T18:42:09Z

@erikerlandson @foxish @iyanuobidele - anyone have any more comments about this?

ash211

Still looks good after the last two changes

foxish · 2017-02-23T20:25:27Z

...etes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesComponentCleaner.scala

+    registeredPods.synchronized {
+      registeredPods.values.foreach { pod =>
+        Utils.tryLogNonFatalError {
+          kubernetesClient.pods().delete(pod)


I'm wondering if all of these can be accomplished using the kubernetesClient.resource(obj).delete(...) call?

It would simplify the logic here to instead just track each of them as resources, rather than pods/secrets/services.

mccheah · 2017-02-23T20:46:37Z

@foxish addressed your comments, thanks for the review. @ash211

ash211

That's a nice change to clean resources generically. I'm comfortable now -- will merge when the build is green

foxish · 2017-02-23T20:54:10Z

LGTM. Thanks!

…122) * Refactor the cleaning up of Kubernetes components. Create a KubernetesComponentsCleaner which can register arbitrary pods, services, secrets, and ingresses. When an exception is thrown or the JVM shuts down, the cleaner automatically purges any of its registered components from Kubernetes. The components can be unregistered when the driver successfully begins running, so that the application persists beyond the lifetime of the spark-submit process. * Fix spacing * Address comments * Fix compiler error * Pull KubernetesComponentCleaner into instance variable * Remove a parameter * Remove redundant registerOrUpdateSecret for SSL * Remove Ingresses from component cleaner * Clear resources generically as opposed to specifying each type * Remove incorrect test assertion * Rename variable

…pache-spark-on-k8s#122) * Refactor the cleaning up of Kubernetes components. Create a KubernetesComponentsCleaner which can register arbitrary pods, services, secrets, and ingresses. When an exception is thrown or the JVM shuts down, the cleaner automatically purges any of its registered components from Kubernetes. The components can be unregistered when the driver successfully begins running, so that the application persists beyond the lifetime of the spark-submit process. * Fix spacing * Address comments * Fix compiler error * Pull KubernetesComponentCleaner into instance variable * Remove a parameter * Remove redundant registerOrUpdateSecret for SSL * Remove Ingresses from component cleaner * Clear resources generically as opposed to specifying each type * Remove incorrect test assertion * Rename variable (cherry picked from commit 452f8f1)

…-palantir4-k8s-release Resync with k8s

…pache-spark-on-k8s#122) * Refactor the cleaning up of Kubernetes components. Create a KubernetesComponentsCleaner which can register arbitrary pods, services, secrets, and ingresses. When an exception is thrown or the JVM shuts down, the cleaner automatically purges any of its registered components from Kubernetes. The components can be unregistered when the driver successfully begins running, so that the application persists beyond the lifetime of the spark-submit process. * Fix spacing * Address comments * Fix compiler error * Pull KubernetesComponentCleaner into instance variable * Remove a parameter * Remove redundant registerOrUpdateSecret for SSL * Remove Ingresses from component cleaner * Clear resources generically as opposed to specifying each type * Remove incorrect test assertion * Rename variable

mccheah mentioned this pull request Feb 16, 2017

Extract SSL configuration handling to a separate class #123

Merged

mccheah changed the title ~~Refactor submit cleanup~~ Use a separate class to track components that need to be cleaned up Feb 16, 2017

ash211 reviewed Feb 16, 2017

View reviewed changes

mccheah force-pushed the refactor-submit-cleanup branch from dd599d4 to 8c69aa9 Compare February 16, 2017 22:46

mccheah force-pushed the refactor-submit-cleanup branch from 8c69aa9 to d1d76b0 Compare February 17, 2017 01:25

mccheah added 4 commits February 22, 2017 14:06

Fix spacing

c4f596e

Address comments

d40fcda

Fix compiler error

693d1ec

mccheah force-pushed the refactor-submit-cleanup branch from d1d76b0 to 693d1ec Compare February 22, 2017 23:15

Merge remote-tracking branch 'apache-spark-on-k8s/k8s-support-alterna…

9a77eb5

…te-incremental' into refactor-submit-cleanup-rebased-2

mccheah changed the base branch from ingress-support to k8s-support-alternate-incremental February 22, 2017 23:18

ash211 reviewed Feb 22, 2017

View reviewed changes

Pull KubernetesComponentCleaner into instance variable

e0b3b38

ash211 reviewed Feb 22, 2017

View reviewed changes

Remove a parameter

567a284

ash211 approved these changes Feb 23, 2017

View reviewed changes

mccheah added 2 commits February 23, 2017 10:43

Remove redundant registerOrUpdateSecret for SSL

50bce5d

Remove Ingresses from component cleaner

43b02e0

ash211 approved these changes Feb 23, 2017

View reviewed changes

foxish reviewed Feb 23, 2017

View reviewed changes

mccheah added 2 commits February 23, 2017 12:43

Clear resources generically as opposed to specifying each type

258d8f8

Remove incorrect test assertion

f1c978a

ash211 approved these changes Feb 23, 2017

View reviewed changes

Rename variable

8902f02

ash211 merged commit 452f8f1 into k8s-support-alternate-incremental Feb 23, 2017

mccheah deleted the refactor-submit-cleanup branch February 24, 2017 02:32

ash211 mentioned this pull request Mar 2, 2017

Ctrl+C on blocking spark-submit doesn't stop running job #94

Closed

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019

Merge pull request apache-spark-on-k8s#122 from palantir/branch-2.2.0…

5dff733

…-palantir4-k8s-release Resync with k8s

Use a separate class to track components that need to be cleaned up #122

Use a separate class to track components that need to be cleaned up #122

Uh oh!

Conversation

mccheah commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mccheah commented Feb 16, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

ash211 Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

mccheah commented Feb 16, 2017

Uh oh!

mccheah commented Feb 18, 2017

Uh oh!

mccheah commented Feb 22, 2017

Uh oh!

ash211 Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 commented Feb 22, 2017

Uh oh!

ash211 Feb 22, 2017

Choose a reason for hiding this comment

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

mccheah commented Feb 23, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

foxish Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

foxish Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

mccheah commented Feb 23, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

foxish commented Feb 23, 2017

Uh oh!

Uh oh!

mccheah commented Feb 16, 2017 •

edited

Loading