Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WOR-1823 part 3: workspace deletion refactoring #3049

Merged
merged 38 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
a85850a
break out shared processing of workspace response
blakery Sep 18, 2024
5c4d9a2
refactor getWorkspaceDetails to use for comprehension
blakery Sep 19, 2024
7a44f60
remove pointless transaction when getting bucket options
blakery Sep 19, 2024
860cc9a
remove inspection supression, fix resulting issues
blakery Sep 19, 2024
ec3c984
clean up getBucketUsage and add unit tests
blakery Sep 20, 2024
fff57c0
clean up context parameters to reflect actual usage
blakery Sep 20, 2024
bc2f459
move getBucketOptions next to getBucketUsage and use for comprehensio…
blakery Sep 20, 2024
7b62fdb
move raw datasource calls to repository
blakery Sep 20, 2024
4a64f2f
eliminate some raw usages of datasource
blakery Sep 20, 2024
8ee5087
moved admin methods to separate service
blakery Sep 20, 2024
7956f1e
only retrieve transfers from queried workspace
blakery Sep 20, 2024
09cff13
move database operations for lock/unlock into repository
blakery Sep 20, 2024
f6ce425
fixed unit tests for getWorkspaceById
blakery Sep 23, 2024
b280c87
moved only method usage of tryIsCurator inline
blakery Sep 24, 2024
fb66ae0
move only usage of withLibraryAttributeNamespaceCheck to calling service
blakery Sep 24, 2024
c6e10d1
simplified canShare resolution
blakery Sep 25, 2024
4a9082c
add method to list submissions, change single query to return an option
blakery Sep 25, 2024
9bac0c0
refactor list workspaces
blakery Sep 25, 2024
21899ff
add unit tests for listWorkspaces
blakery Sep 26, 2024
2be1d09
scalafmt
blakery Sep 26, 2024
b727928
move sync validation method inline, and remove unnecessary future app…
blakery Sep 26, 2024
be34c72
add simple trace method for sync operations using RawlsRequestContext
blakery Sep 27, 2024
696ddd3
add an apply method for RawlsExceptionWithErrorReport
blakery Sep 27, 2024
e4cdad4
convert unnecessary usage of a Future Applicative to a sync operation
blakery Sep 27, 2024
5b944c5
rename, reformat, and clean up for more concise tests
blakery Sep 27, 2024
8f932ec
refactor outer deleteWorkspace method and add tests
blakery Sep 27, 2024
f644662
refactor deleteWorkspacesInternal and add tests
blakery Sep 27, 2024
8190331
remove unused function
blakery Sep 27, 2024
8a4d4d5
delete function moved to submissions repo
blakery Sep 27, 2024
905e3f1
fix disabled annotations and remove unused imports
blakery Sep 27, 2024
0a691a4
remove unnecessary cats effect
blakery Sep 27, 2024
6b3e91b
make service consistent about not handling MC workspace deletion
blakery Sep 27, 2024
daf7335
move deleteWorkspaceInternal inline
blakery Sep 27, 2024
d5e3507
move attribute exceptions to AttributeSupport
blakery Sep 27, 2024
52b1b18
Squashed commit of the following:
blakery Sep 27, 2024
aa4f1dc
add back method that got overwritten when rebasing
blakery Sep 30, 2024
5c5067a
added back attempt to delete workspace in WSM
blakery Sep 30, 2024
74fe8a1
run scalafmt
blakery Sep 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,15 @@ import org.broadinstitute.dsde.rawls.entities.exceptions.{
import org.broadinstitute.dsde.rawls.expressions.ExpressionEvaluator
import org.broadinstitute.dsde.rawls.metrics.RawlsInstrumented
import org.broadinstitute.dsde.rawls.model.AttributeUpdateOperations.{AttributeUpdateOperation, EntityUpdateDefinition}
import org.broadinstitute.dsde.rawls.model.{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are just from moving an exception definition to the place where it's thrown.

AttributeEntityReference,
Entity,
EntityCopyDefinition,
EntityQuery,
ErrorReport,
SamResourceTypeNames,
SamWorkspaceActions,
WorkspaceName,
_
import org.broadinstitute.dsde.rawls.model._
import org.broadinstitute.dsde.rawls.util.{
AttributeSupport,
AttributeUpdateOperationException,
EntitySupport,
JsonFilterUtils,
WorkspaceSupport
}
import org.broadinstitute.dsde.rawls.util.{AttributeSupport, EntitySupport, JsonFilterUtils, WorkspaceSupport}
import org.broadinstitute.dsde.rawls.workspace.{AttributeUpdateOperationException, WorkspaceRepository}
import org.broadinstitute.dsde.rawls.workspace.WorkspaceRepository
import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport}
import slick.jdbc.{ResultSetConcurrency, ResultSetType, TransactionIsolation}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,12 @@ object MetricsHelper {
def incrementFastPassRevokedCounter(memberType: IamMemberType): IO[Unit] =
incrementFastPassUpdatedCounter(memberType, "revoke")

def incrementCounter(name: String,
count: Int = 1,
labels: Map[String, String] = Map.empty,
description: Option[String] = None
): IO[Unit] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneccesary IO wrapping - only called in one place anyway.

def incrementCounter(
name: String,
count: Int = 1,
labels: Map[String, String] = Map.empty,
description: Option[String] = None
): Unit = {
val metrics = meter
.counterBuilder(s"$PREFIX/$name")
.setDescription(description.getOrElse("none"))
Expand All @@ -53,7 +54,7 @@ object MetricsHelper {
labels.foreach { case (k, v) =>
labelBuilder.put(k, v)
}
IO(metrics.add(count, labelBuilder.build()))
metrics.add(count, labelBuilder.build())
}

private def incrementFastPassUpdatedCounter(memberType: IamMemberType, action: String) =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package org.broadinstitute.dsde.rawls.submissions

import org.broadinstitute.dsde.rawls.dataaccess.SlickDataSource
import org.broadinstitute.dsde.rawls.dataaccess.slick.WorkflowRecord
import org.broadinstitute.dsde.rawls.metrics.RawlsInstrumented
import org.broadinstitute.dsde.rawls.model.{WorkflowStatuses, Workspace, WorkspaceName}

import scala.concurrent.{ExecutionContext, Future}

/**
* Data access for workflow submissions
*
* The intention of this class is to hide direct dependencies on Slick behind a relatively clean interface
* to ease testability of higher level business logic.
*/
class SubmissionsRepository(
dataSource: SlickDataSource,
trackDetailedSubmissionMetrics: Boolean = true,
override val workbenchMetricBaseName: String
) extends RawlsInstrumented {

import dataSource.dataAccess.driver.api._

def getActiveWorkflowsAndSetStatusToAborted(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved in wholesale from WorkspaceService.

workspace: Workspace
)(implicit ex: ExecutionContext): Future[Seq[WorkflowRecord]] =
dataSource
.inTransaction { dataAccess =>
for {
// Gather any active workflows with external ids
workflowsToAbort <- dataAccess.workflowQuery.findActiveWorkflowsWithExternalIds(workspace)

// If a workflow is not done, automatically change its status to Aborted
_ <- dataAccess.workflowQuery.findWorkflowsByWorkspace(workspace).result.map { workflowRecords =>
workflowRecords
.filter(workflowRecord => !WorkflowStatuses.withName(workflowRecord.status).isDone)
.foreach { workflowRecord =>
dataAccess.workflowQuery.updateStatus(workflowRecord, WorkflowStatuses.Aborted) { status =>
if (trackDetailedSubmissionMetrics)
Option(
workflowStatusCounter(
workspaceSubmissionMetricBuilder(workspace.toWorkspaceName, workflowRecord.submissionId)
)(
status
)
)
else None
}
}
}
} yield workflowsToAbort
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.broadinstitute.dsde.rawls.util

import akka.http.scaladsl.model.StatusCodes
import org.broadinstitute.dsde.rawls.RawlsExceptionWithErrorReport
import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport}
import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap
import org.broadinstitute.dsde.rawls.model.AttributeUpdateOperations.{
AddListMember,
Expand All @@ -26,7 +26,6 @@ import org.broadinstitute.dsde.rawls.model.{
ErrorReport,
MethodConfiguration
}
import org.broadinstitute.dsde.rawls.workspace.{AttributeNotFoundException, AttributeUpdateOperationException}

import scala.concurrent.Future

Expand Down Expand Up @@ -172,10 +171,13 @@ trait AttributeSupport {
*
* @param entity to update
* @param operations sequence of operations
* @throws org.broadinstitute.dsde.rawls.workspace.AttributeNotFoundException when removing from a list attribute that does not exist
* @throws AttributeNotFoundException when removing from a list attribute that does not exist
* @throws AttributeUpdateOperationException when adding or removing from an attribute that is not a list
* @return the updated entity
*/
def applyOperationsToEntity(entity: Entity, operations: Seq[AttributeUpdateOperation]): Entity =
entity.copy(attributes = applyAttributeUpdateOperations(entity, operations))
}

class AttributeUpdateOperationException(message: String) extends RawlsException(message)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place these are thrown, so it makes sense for them to be defined here.

class AttributeNotFoundException(message: String) extends AttributeUpdateOperationException(message)
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,33 @@ object TracingUtils {
def traceFuture[T](name: String)(f: RawlsTracingContext => Future[T])(implicit ec: ExecutionContext): Future[T] =
traceFutureWithParent(name, RawlsTracingContext(otelContext = Option(Context.root())))(f)

/**
* Trace a sync operation with the RawlsRequestContext
*
* @param name The name that will be used in the trace
* @param parentContext the RawlsRequestContext to use for tracing, if `parentContext.otelContext` is defined
* @param f the operation to execute within the trace
* @return the result of the operation `f`
*/
def trace[T](name: String, parentContext: RawlsRequestContext)(f: RawlsRequestContext => T): T =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having this was pushing a lot of different places to use a Future where it wasn't really necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be equivalent to traceNakedWithParent below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's using RawlsTracingContext not RawlsRequestContext.

It's arguable that the better approach would be an overload or something, but we have similar divergence between others, such as between traceFutureWithParent and traceFuture.
Also, this is the most common/basic/unembellished instance of this, and therefore worth giving the most basic name. But I didn't feel like renaming usages of traceNakedWithParent in what is already a substantial PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Reading is hard. I prefer this to overloading 👍

parentContext.otelContext match {
case Some(otelContext) if instrumenter.shouldStart(otelContext, name) =>
val childContext = instrumenter.start(otelContext, name)
val result = Try(f(parentContext.copy(otelContext = Option(childContext))))
instrumenter.end(childContext, name, name, result.failed.toOption.orNull)
result.get
case _ =>
f(parentContext)
}

/**
* Trace a sync operation with the RawlsTracingContext
*
* @param name The name that will be used in the trace
* @param parentContext the RawlsTracingContext to use for tracing, if `parentContext.otelContext` is defined
* @param f the operation to execute within the trace
* @return the result of the operation `f`
*/
def traceNakedWithParent[T](name: String, parentContext: RawlsTracingContext)(
f: RawlsTracingContext => T
): T =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.broadinstitute.dsde.rawls.workspace
import akka.http.scaladsl.model.StatusCodes
import org.broadinstitute.dsde.rawls.RawlsExceptionWithErrorReport
import org.broadinstitute.dsde.rawls.dataaccess.SlickDataSource
import org.broadinstitute.dsde.rawls.dataaccess.slick.PendingBucketDeletionRecord
import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap
import org.broadinstitute.dsde.rawls.model.{
ErrorReport,
Expand All @@ -18,7 +19,6 @@ import org.broadinstitute.dsde.rawls.model.{
import org.broadinstitute.dsde.rawls.model.WorkspaceState.WorkspaceState
import org.broadinstitute.dsde.rawls.util.TracingUtils.traceDBIOWithParent
import org.joda.time.DateTime
import slick.dbio.DBIO

import java.util.UUID
import scala.concurrent.{ExecutionContext, Future}
Expand Down Expand Up @@ -51,7 +51,9 @@ class WorkspaceRepository(dataSource: SlickDataSource) {
_.workspaceQuery.getV2WorkspaceId(workspaceName)
}

def listWorkspacesByIds(workspaceIds: Seq[UUID], attributeSpecs: Option[WorkspaceAttributeSpecs] = None): Future[Seq[Workspace]] = dataSource.inTransaction {
def listWorkspacesByIds(workspaceIds: Seq[UUID],
attributeSpecs: Option[WorkspaceAttributeSpecs] = None
): Future[Seq[Workspace]] = dataSource.inTransaction {
_.workspaceQuery.listV2WorkspacesByIds(workspaceIds, attributeSpecs)
}

Expand All @@ -77,6 +79,22 @@ class WorkspaceRepository(dataSource: SlickDataSource) {
access.workspaceQuery.delete(workspaceName)
}

def deleteRawlsWorkspace(workspace: Workspace)(implicit ex: ExecutionContext): Future[Unit] =
dataSource.inTransaction { dataAccess =>
for {
// Delete components of the workspace
_ <- dataAccess.submissionQuery.deleteFromDb(workspace.workspaceIdAsUUID)
_ <- dataAccess.methodConfigurationQuery.deleteFromDb(workspace.workspaceIdAsUUID)
_ <- dataAccess.entityQuery.deleteFromDb(workspace)

// Schedule bucket for deletion
_ <- dataAccess.pendingBucketDeletionQuery.save(PendingBucketDeletionRecord(workspace.bucketName))

// Delete the workspace
_ <- dataAccess.workspaceQuery.delete(workspace.toWorkspaceName)
} yield ()
}

def createMCWorkspace(workspaceId: UUID,
workspaceName: WorkspaceName,
attributes: AttributeMap,
Expand Down Expand Up @@ -153,13 +171,13 @@ class WorkspaceRepository(dataSource: SlickDataSource) {
def savePendingCloneWorkspaceFileTransfer(destWorkspace: UUID, sourceWorkspace: UUID, prefix: String): Future[Int] =
dataSource.inTransaction(_.cloneWorkspaceFileTransferQuery.save(destWorkspace, sourceWorkspace, prefix))

def getSubmissionSummaryStats(workspaceId: UUID)(implicit ex: ExecutionContext): Future[Option[WorkspaceSubmissionStats]] =
def getSubmissionSummaryStats(workspaceId: UUID)(implicit
ex: ExecutionContext
): Future[Option[WorkspaceSubmissionStats]] =
dataSource.inTransaction(_.workspaceQuery.listSubmissionSummaryStats(Seq(workspaceId))).map(_.values.headOption)

def listSubmissionSummaryStats(workspaceIds: Seq[UUID]): Future[Map[UUID, WorkspaceSubmissionStats]] =
dataSource.inTransaction(_.workspaceQuery.listSubmissionSummaryStats(workspaceIds))


dataSource.inTransaction(_.workspaceQuery.listSubmissionSummaryStats(workspaceIds))

def getTags(workspaceIds: Seq[UUID], query: Option[String], limit: Option[Int] = None): Future[Seq[WorkspaceTag]] =
dataSource.inTransaction(_.workspaceQuery.getTags(query, limit, Some(workspaceIds)))
Expand Down
Loading
Loading