kunwp1 commented on code in PR #6046:
URL: https://github.com/apache/texera/pull/6046#discussion_r3864137405
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/ComputingUnitManagingService.scala:
##########
@@ -32,9 +32,55 @@ import org.apache.texera.service.resource.{
ComputingUnitManagingResource,
HealthCheckResource
}
+import
org.apache.texera.service.resource.ComputingUnitManagingResource.TerminatedComputingUnitInfo
+import org.slf4j.LoggerFactory
import java.nio.file.Path
+import java.util.concurrent.TimeUnit
class ComputingUnitManagingService extends
Application[ComputingUnitManagingServiceConfiguration] {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingService])
+
+ private[service] def initSqlServer(
+ connect: (String, String, String) => Unit = SqlServer.initConnection
Review Comment:
Can you drop the `connect` parameter because it's never overridden?
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
Review Comment:
use `private[resource]`. Then you can remove duplicates in line 389.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
+
private def context: DSLContext =
SqlServer
.getInstance()
.createDSLContext()
+ private[resource] final class IdleComputingUnitCleanupConfig(
Review Comment:
Use `case class`. Then you can remove `copy()`.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/ComputingUnitManagingService.scala:
##########
@@ -32,9 +32,55 @@ import org.apache.texera.service.resource.{
ComputingUnitManagingResource,
HealthCheckResource
}
+import
org.apache.texera.service.resource.ComputingUnitManagingResource.TerminatedComputingUnitInfo
+import org.slf4j.LoggerFactory
import java.nio.file.Path
+import java.util.concurrent.TimeUnit
class ComputingUnitManagingService extends
Application[ComputingUnitManagingServiceConfiguration] {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingService])
+
+ private[service] def initSqlServer(
+ connect: (String, String, String) => Unit = SqlServer.initConnection
+ ): Unit =
+ connect(
+ StorageConfig.jdbcUrl,
+ StorageConfig.jdbcUsername,
+ StorageConfig.jdbcPassword
+ )
+
+ private[service] def registerIdleComputingUnitCleanup(
+ environment: Environment,
+ kubernetesComputingUnitEnabled: Boolean =
KubernetesConfig.kubernetesComputingUnitEnabled,
+ idleTimeoutMinutes: Long =
KubernetesConfig.computingUnitIdleTimeoutMinutes,
+ idleCheckIntervalMinutes: Long =
KubernetesConfig.computingUnitIdleCheckIntervalMinutes,
+ terminateIdleComputingUnits: () => List[TerminatedComputingUnitInfo] =
() =>
+ ComputingUnitManagingResource.terminateIdleKubernetesComputingUnits(),
+ logTerminatedUnits: String => Unit = message => logger.info(message),
+ logCleanupFailure: Throwable => Unit = throwable =>
+ logger.warn("Failed to terminate idle Kubernetes computing units",
throwable),
+ scheduleWithFixedDelay: Option[(Runnable, Long, Long, TimeUnit) => Unit]
= None
+ ): Unit =
+ if (kubernetesComputingUnitEnabled && idleTimeoutMinutes > 0) {
+ val scheduler = scheduleWithFixedDelay.getOrElse((command, initialDelay,
delay, unit) =>
Review Comment:
Can you add a guard to handle the case if `idleCheckIntervalMinutes` is 0 or
negative value because it will cause a service abort.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
+
private def context: DSLContext =
SqlServer
.getInstance()
.createDSLContext()
+ private[resource] final class IdleComputingUnitCleanupConfig(
+ val enabled: Boolean,
+ val idleTimeoutMinutes: Long
+ ) {
+ def copy(
+ enabled: Boolean = this.enabled,
+ idleTimeoutMinutes: Long = this.idleTimeoutMinutes
+ ): IdleComputingUnitCleanupConfig =
+ new IdleComputingUnitCleanupConfig(enabled, idleTimeoutMinutes)
+ }
+
+ private[resource] final class IdleComputingUnitCandidate(
+ val unit: WorkflowComputingUnit,
+ val username: Option[String]
+ )
+
+ private[resource] object WorkflowExecutionStatus extends Enumeration {
Review Comment:
Add a comment that this enum needs to be synced with amber's status code.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -621,8 +837,13 @@ class ComputingUnitManagingResource {
KubernetesClient.deletePod(cuid)
}
+ val terminationReason =
WorkflowComputingUnitTerminationReasonEnum.USER_REQUESTED
unit.setTerminateTime(new Timestamp(System.currentTimeMillis()))
+ unit.setTerminationReason(terminationReason)
cuDao.update(unit)
+ logger.info(
+ s"Terminated 1 Kubernetes computing unit(s): cuid=${unit.getCuid},
name=${unit.getName}, uid=${unit.getUid}, username=${user.getName},
reason=${terminationReason.getLiteral}"
Review Comment:
Rename it to `Terminated computing unit`. I think this path can be reachable
from local deployment.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -467,6 +677,12 @@ class ComputingUnitManagingResource {
@Path("")
def listComputingUnits(
@Auth user: SessionUser
+ ): List[DashboardWorkflowComputingUnit] =
+ listComputingUnits(user, DefaultKubernetesPodOperations)
+
+ private[resource] def listComputingUnits(
+ user: SessionUser,
+ podOperations: KubernetesPodOperations
Review Comment:
Is there a reason to separate these functions? Can you merge these two
functions?
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
+
private def context: DSLContext =
SqlServer
.getInstance()
.createDSLContext()
+ private[resource] final class IdleComputingUnitCleanupConfig(
+ val enabled: Boolean,
+ val idleTimeoutMinutes: Long
+ ) {
+ def copy(
+ enabled: Boolean = this.enabled,
+ idleTimeoutMinutes: Long = this.idleTimeoutMinutes
+ ): IdleComputingUnitCleanupConfig =
+ new IdleComputingUnitCleanupConfig(enabled, idleTimeoutMinutes)
+ }
+
+ private[resource] final class IdleComputingUnitCandidate(
+ val unit: WorkflowComputingUnit,
+ val username: Option[String]
+ )
+
+ private[resource] object WorkflowExecutionStatus extends Enumeration {
+ val UninitializedOrReady: Value = Value(0)
+ val Running: Value = Value(1)
+ val Paused: Value = Value(2)
+
+ def toDbStatus(status: Value): java.lang.Short =
Short.box(status.id.toShort)
+ }
+
+ private[resource] trait KubernetesPodOperations {
+ val podExists: Int => Boolean
+ val deletePod: Int => Unit
+ }
+
+ private[resource] object DefaultKubernetesPodOperations extends
KubernetesPodOperations {
+ private[resource] var podExistsDelegate: Int => Boolean =
KubernetesClient.podExists
Review Comment:
I don't see the need of making these define as two variables. Can you
collapse it into `podExists` and `deletePod` to simplify the code?
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -131,6 +332,14 @@ object ComputingUnitManagingResource {
.get
)
+ final class TerminatedComputingUnitInfo(
Review Comment:
Use `case class`
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
+
private def context: DSLContext =
SqlServer
.getInstance()
.createDSLContext()
+ private[resource] final class IdleComputingUnitCleanupConfig(
+ val enabled: Boolean,
+ val idleTimeoutMinutes: Long
+ ) {
+ def copy(
+ enabled: Boolean = this.enabled,
+ idleTimeoutMinutes: Long = this.idleTimeoutMinutes
+ ): IdleComputingUnitCleanupConfig =
+ new IdleComputingUnitCleanupConfig(enabled, idleTimeoutMinutes)
+ }
+
+ private[resource] final class IdleComputingUnitCandidate(
+ val unit: WorkflowComputingUnit,
+ val username: Option[String]
+ )
+
+ private[resource] object WorkflowExecutionStatus extends Enumeration {
+ val UninitializedOrReady: Value = Value(0)
+ val Running: Value = Value(1)
+ val Paused: Value = Value(2)
+
+ def toDbStatus(status: Value): java.lang.Short =
Short.box(status.id.toShort)
+ }
+
+ private[resource] trait KubernetesPodOperations {
+ val podExists: Int => Boolean
+ val deletePod: Int => Unit
+ }
+
+ private[resource] object DefaultKubernetesPodOperations extends
KubernetesPodOperations {
+ private[resource] var podExistsDelegate: Int => Boolean =
KubernetesClient.podExists
+ private[resource] var deletePodDelegate: Int => Unit =
KubernetesClient.deletePod
+
+ override val podExists: Int => Boolean = cuid => podExistsDelegate(cuid)
+ override val deletePod: Int => Unit = cuid => deletePodDelegate(cuid)
+ }
+
+ private[resource] def lastComputingUnitActivityTime(
+ unit: WorkflowComputingUnit,
+ latestUpdateTime: Option[Timestamp],
+ latestStartTime: Option[Timestamp]
+ ): Timestamp =
+ Seq(
+ latestUpdateTime,
+ latestStartTime,
+ Option(unit.getCreationTime)
+ ).flatten.maxBy(_.getTime)
+
+ private[resource] def shouldTerminateIdleComputingUnit(
+ hasActiveExecution: Boolean,
+ lastExecutionTime: Timestamp,
+ cutoff: Timestamp
+ ): Boolean =
+ !hasActiveExecution && lastExecutionTime.before(cutoff)
+
+ def terminateIdleKubernetesComputingUnits():
List[TerminatedComputingUnitInfo] =
+ runIdleKubernetesComputingUnitCleanup(
+ new IdleComputingUnitCleanupConfig(
+ KubernetesConfig.kubernetesComputingUnitEnabled,
+ KubernetesConfig.computingUnitIdleTimeoutMinutes
+ ),
+ () => new Timestamp(System.currentTimeMillis()),
+ DefaultKubernetesPodOperations
+ )
+
+ private[resource] def runIdleKubernetesComputingUnitCleanup(
+ cleanupConfig: IdleComputingUnitCleanupConfig,
+ currentTime: () => Timestamp,
+ podOperations: KubernetesPodOperations
+ ): List[TerminatedComputingUnitInfo] = {
+ if (!cleanupConfig.enabled || cleanupConfig.idleTimeoutMinutes <= 0) {
+ return List.empty
+ }
+
+ val now = currentTime()
+ val cutoff = new Timestamp(now.getTime - cleanupConfig.idleTimeoutMinutes
* 60 * 1000)
+
+ idleKubernetesComputingUnitCandidates(cutoff).flatMap(candidate =>
+ terminateIdleKubernetesComputingUnitCandidate(candidate, now,
podOperations)
+ )
+ }
+
+ private[resource] def idleKubernetesComputingUnitCandidates(
+ cutoff: Timestamp
+ ): List[IdleComputingUnitCandidate] = {
+ val activeStatuses = Seq(
+ WorkflowExecutionStatus.UninitializedOrReady,
+ WorkflowExecutionStatus.Running,
+ WorkflowExecutionStatus.Paused
+ ).map(WorkflowExecutionStatus.toDbStatus)
+
+ // All three questions asked per computing unit -- is any execution still
active, when did an
+ // execution last report progress, when did one last start -- are
aggregates over the same rows
+ // grouped by the same key, so one grouped query answers them for every
unit at once. The left
+ // joins keep units that have no executions (both max() are NULL) and
units whose owner row is
+ // gone (name is NULL), matching what a per-unit scan would produce.
+ val latestUpdateTime = max(WORKFLOW_EXECUTIONS.LAST_UPDATE_TIME)
+ val latestStartTime = max(WORKFLOW_EXECUTIONS.STARTING_TIME)
+ val hasActiveExecution =
boolOr(WORKFLOW_EXECUTIONS.STATUS.in(activeStatuses: _*))
Review Comment:
I have one concern. There are cases where the workflow execution crashes for
some reason (the coordinator died, or execution has an OOM). In that case, the
status is still in `RUNNING` state. However, the PR doesn't delete the CU with
the crashed execution. I wonder whether you thought about this cause.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -61,18 +67,213 @@ import org.apache.texera.service.util.{
KubernetesClient
}
import org.jooq.{DSLContext, EnumType}
+import org.jooq.impl.DSL.{boolOr, max}
+import org.slf4j.LoggerFactory
import play.api.libs.json._
import java.sql.Timestamp
import scala.annotation.unused
import scala.jdk.CollectionConverters.CollectionHasAsScala
+import scala.util.control.NonFatal
object ComputingUnitManagingResource {
+ private val logger =
LoggerFactory.getLogger(classOf[ComputingUnitManagingResource])
+
private def context: DSLContext =
SqlServer
.getInstance()
.createDSLContext()
+ private[resource] final class IdleComputingUnitCleanupConfig(
+ val enabled: Boolean,
+ val idleTimeoutMinutes: Long
+ ) {
+ def copy(
+ enabled: Boolean = this.enabled,
+ idleTimeoutMinutes: Long = this.idleTimeoutMinutes
+ ): IdleComputingUnitCleanupConfig =
+ new IdleComputingUnitCleanupConfig(enabled, idleTimeoutMinutes)
+ }
+
+ private[resource] final class IdleComputingUnitCandidate(
Review Comment:
Better to use `case class` here too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
