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

Make deleteApp idempotent #4246

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
@@ -1,12 +1,7 @@
package org.broadinstitute.dsde.workbench.leonardo

import org.broadinstitute.dsde.workbench.azure.{
AKSClusterName,
ApplicationInsightsName,
BatchAccountName,
RelayNamespace
}

import org.broadinstitute.dsde.workbench.azure.{AKSClusterName, ApplicationInsightsName, BatchAccountName, RelayNamespace}
import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.{NamespaceName, ServiceAccountName}
import org.broadinstitute.dsde.workbench.google2.{NetworkName, SubnetworkName}

import java.util.UUID
Expand All @@ -31,9 +26,14 @@ final case class AKSCluster(name: String, tags: Map[String, Boolean]) {
def asClusterName: AKSClusterName = AKSClusterName(name)
}

final case class WsmManagedAzureIdentity(wsmResourceName: String, managedIdentityName: String)
final case class WsmManagedAzureIdentity(wsmResourceName: String, managedIdentityName: String, wsmResourceId: WsmControlledResourceId)

final case class WsmControlledKubernetesNamespaceResource(name: NamespaceName,
wsmResourceId: WsmControlledResourceId,
serviceAccountName: ServiceAccountName
)

final case class WsmControlledDatabaseResource(wsmDatabaseName: String, azureDatabaseName: String)
final case class WsmControlledDatabaseResource(wsmDatabaseName: String, azureDatabaseName: String, wsmResourceId: WsmControlledResourceId)

final case class LandingZoneResources(landingZoneId: UUID,
aksCluster: AKSCluster,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,12 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
.transaction
wsmDatabases <- wsmDatabases.traverse { wsmDatabase =>
F.blocking(wsmApi.getAzureDatabase(workspaceId.value, wsmDatabase.resourceId.value))
.map(db => WsmControlledDatabaseResource(db.getMetadata.getName, db.getAttributes.getDatabaseName))
.map(db =>
WsmControlledDatabaseResource(db.getMetadata.getName,
db.getAttributes.getDatabaseName,
WsmControlledResourceId(db.getMetadata.getResourceId)
)
)
}

// call WSM resource API to get list of ReferenceDatabases
Expand Down Expand Up @@ -502,35 +507,44 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
legacyWsmDao.getLandingZoneResources(billingProfileId, leoAuth)
}

wsmControlledResourceApi <- buildWsmControlledResourceApiClient
wsmResourceApi <- buildWsmResourceApiClient

// WSM deletion order matters here. Delete WSM database resources first.
wsmDatabases <- appControlledResourceQuery
wsmDatabaseRecords <- appControlledResourceQuery
.getAllForAppByType(app.id.id, WsmResourceType.AzureDatabase)
.transaction
wsmDatabases <- wsmDatabaseRecords.traverse { wsmDatabase =>
F.blocking(wsmControlledResourceApi.getAzureDatabase(workspaceId.value, wsmDatabase.resourceId.value))
.map(db =>
WsmControlledDatabaseResource(db.getMetadata.getName,
db.getAttributes.getDatabaseName,
WsmControlledResourceId(db.getMetadata.getResourceId)
)
)
}
_ <- childSpan("deleteWsmDatabases").use { implicit ev =>
wsmDatabases.traverse { database =>
deleteWsmResource(workspaceId, app, database)
deleteWsmResource(workspaceId, app, database.wsmResourceId, WsmResourceType.AzureDatabase)
}
}

// Then delete namespace resources
wsmNamespaces <- appControlledResourceQuery
.getAllForAppByType(app.id.id, WsmResourceType.AzureKubernetesNamespace)
.transaction
namespacePrefix = app.appResources.namespace.value
wsmNamespaces <- retrieveWsmNamespace(wsmResourceApi, namespacePrefix, workspaceId.value)
deletedNamespace <- childSpan("deleteWsmNamespace").use { implicit ev =>
wsmNamespaces
.traverse { namespace =>
deleteWsmNamespaceResource(workspaceId, app, namespace)
deleteWsmNamespaceResource(wsmControlledResourceApi, workspaceId, app, namespace.wsmResourceId)
}
.map(_.nonEmpty)
}

// Then delete identity resources
wsmIdentities <- appControlledResourceQuery
.getAllForAppByType(app.id.id, WsmResourceType.AzureManagedIdentity)
.transaction
wsmIdentities <- retrieveWsmManagedIdentity(wsmResourceApi, app.appType, workspaceId.value)
_ <- childSpan("deleteWsmIdentity").use { implicit ev =>
wsmIdentities.traverse { identity =>
deleteWsmResource(workspaceId, app, identity)
deleteWsmResource(workspaceId, app, identity.wsmResourceId, WsmResourceType.AzureManagedIdentity)
}
}

Expand Down Expand Up @@ -706,7 +720,8 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
createWsmIdentityResource(app, namespacePrefix, workspaceId)
.map(i =>
WsmManagedAzureIdentity(i.getAzureManagedIdentity.getMetadata.getName,
i.getAzureManagedIdentity.getAttributes.getManagedIdentityName
i.getAzureManagedIdentity.getAttributes.getManagedIdentityName,
WsmControlledResourceId(i.getResourceId)
)
)
.map(_.some)
Expand Down Expand Up @@ -804,7 +819,8 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
createWsmDatabaseResource(app, workspaceId, controlledDbForApp, namespacePrefix, owner, wsmApi).map {
db =>
WsmControlledDatabaseResource(db.getAzureDatabase.getMetadata.getName,
db.getAzureDatabase.getAttributes.getDatabaseName
db.getAzureDatabase.getAttributes.getDatabaseName,
WsmControlledResourceId(db.getResourceId)
)
}
}
Expand Down Expand Up @@ -923,10 +939,12 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
wsmManagedIdentities.map { identities =>
// there should be only 1 Azure managed identity per app
identities
.find(r => wsmResourceName == r.getMetadata().getName())
.map(r =>
WsmManagedAzureIdentity(r.getMetadata.getName,
r.getResourceAttributes.getAzureManagedIdentity.getManagedIdentityName
.find(identity => wsmResourceName == identity.getMetadata.getName)
.map(identity =>
WsmManagedAzureIdentity(
wsmResourceName,
identity.getResourceAttributes.getAzureManagedIdentity.getManagedIdentityName,
WsmControlledResourceId(identity.getMetadata.getResourceId)
)
)
}
Expand All @@ -948,7 +966,8 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
.filter(r => databaseNames.contains(r.getMetadata().getName()))
.map(r =>
WsmControlledDatabaseResource(r.getMetadata().getName(),
r.getResourceAttributes().getAzureDatabase().getDatabaseName()
r.getResourceAttributes().getAzureDatabase().getDatabaseName(),
WsmControlledResourceId(r.getMetadata.getResourceId)
)
)
}
Expand Down Expand Up @@ -1047,18 +1066,16 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
.transaction
} yield result

private[util] def deleteWsmNamespaceResource(workspaceId: WorkspaceId,
private[util] def deleteWsmNamespaceResource(wsmApi: ControlledAzureResourceApi,
workspaceId: WorkspaceId,
app: App,
wsmResource: AppControlledResourceRecord
wsmResourceId: WsmControlledResourceId
)(implicit
ev: Ask[F, AppContext]
): F[Unit] =
for {
ctx <- ev.ask

// Build WSM client
wsmApi <- buildWsmControlledResourceApiClient

// Build delete namespace request
jobId = UUID.randomUUID()
deleteNamespaceRequest = new bio.terra.workspace.model.DeleteControlledAzureResourceRequest().jobControl(
Expand All @@ -1069,15 +1086,15 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,

// Execute WSM call
result <- F.blocking(
wsmApi.deleteAzureKubernetesNamespace(deleteNamespaceRequest, workspaceId.value, wsmResource.resourceId.value)
wsmApi.deleteAzureKubernetesNamespace(deleteNamespaceRequest, workspaceId.value, wsmResourceId.value)
)

_ <- logger.info(ctx.loggingCtx)(s"WSM delete namespace response: ${result}")

// Update record in APP_CONTROLLED_RESOURCE table
_ <- appControlledResourceQuery
.updateStatus(
wsmResource.resourceId,
wsmResourceId,
AppControlledResourceStatus.Deleting
)
.transaction
Expand Down Expand Up @@ -1105,28 +1122,32 @@ class AKSInterpreter[F[_]](config: AKSInterpreterConfig,
} else F.unit

// Update record in APP_CONTROLLED_RESOURCE table
_ <- appControlledResourceQuery.delete(wsmResource.resourceId).transaction
_ <- appControlledResourceQuery.delete(wsmResourceId).transaction
} yield ()

private[util] def deleteWsmResource(workspaceId: WorkspaceId, app: App, wsmResource: AppControlledResourceRecord)(
implicit ev: Ask[F, AppContext]
private[util] def deleteWsmResource(workspaceId: WorkspaceId,
app: App,
wsmResourceId: WsmControlledResourceId,
wsmResourceType: WsmResourceType
)(implicit
ev: Ask[F, AppContext]
): F[Unit] = {
val delete = for {
ctx <- ev.ask
wsmApi <- buildWsmControlledResourceApiClient
_ <- logger.info(ctx.loggingCtx)(
s"Deleting WSM resource ${wsmResource.resourceId.value} for app ${app.appName.value} in workspace ${workspaceId.value}"
s"Deleting WSM resource ${wsmResourceId.value} for app ${app.appName.value} in workspace ${workspaceId.value}"
)
_ <- wsmResource.resourceType match {
_ <- wsmResourceType match {
case WsmResourceType.AzureManagedIdentity =>
F.blocking(wsmApi.deleteAzureManagedIdentity(workspaceId.value, wsmResource.resourceId.value))
F.blocking(wsmApi.deleteAzureManagedIdentity(workspaceId.value, wsmResourceId.value))
case WsmResourceType.AzureDatabase =>
F.blocking(wsmApi.deleteAzureDatabase(workspaceId.value, wsmResource.resourceId.value))
F.blocking(wsmApi.deleteAzureDatabase(workspaceId.value, wsmResourceId.value))
case _ =>
F.raiseError(AppDeletionException(s"Unexpected WSM resource type ${wsmResource.resourceType}"))
F.raiseError(AppDeletionException(s"Unexpected WSM resource type $wsmResourceType"))
}
// Update record in APP_CONTROLLED_RESOURCE table
_ <- appControlledResourceQuery.delete(wsmResource.resourceId).transaction
_ <- appControlledResourceQuery.delete(wsmResourceId).transaction
} yield ()

delete.handleErrorWith {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package org.broadinstitute.dsde.workbench.leonardo.app

import cats.effect.IO
import org.broadinstitute.dsde.workbench.google2.KubernetesSerializableName.ServiceAccountName
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo}
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo, wsmResourceId}
import org.broadinstitute.dsde.workbench.leonardo.{ManagedIdentityName, PostgresServer, WsmControlledDatabaseResource}
import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext
import org.broadinstitute.dsde.workbench.leonardo.http.ConfigReader
Expand All @@ -25,9 +25,9 @@ class CromwellAppInstallSpec extends BaseAppInstallSpec {
val cbasAzureDbName = "cbas_edfgvb"
val tesAzureDbName = "tes_pasgjf"
val cromwellOnAzureDatabases: List[WsmControlledDatabaseResource] = List(
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName),
WsmControlledDatabaseResource("cbas", cbasAzureDbName),
WsmControlledDatabaseResource("tes", tesAzureDbName)
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("cbas", cbasAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("tes", tesAzureDbName, wsmResourceId)
)

it should "build coa override values" in {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.broadinstitute.dsde.workbench.leonardo.app

import cats.effect.IO
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo}
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo, wsmResourceId}
import org.broadinstitute.dsde.workbench.leonardo.http.ConfigReader
import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext
import org.broadinstitute.dsde.workbench.leonardo.WsmControlledDatabaseResource
Expand All @@ -22,9 +22,9 @@ class CromwellRunnerAppInstallSpec extends BaseAppInstallSpec {
val tesAzureDbName = "tes_oiwjnz"
val cromwellMetadataAzureDbName = "cromwellmetadata_tyuiwk"
val cromwellRunnerAzureDatabases: List[WsmControlledDatabaseResource] = List(
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName),
WsmControlledDatabaseResource("tes", tesAzureDbName),
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName)
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("tes", tesAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName, wsmResourceId)
)

it should "build cromwell-runner override values" in {
Expand Down Expand Up @@ -92,9 +92,9 @@ class CromwellRunnerAppInstallSpec extends BaseAppInstallSpec {
it should "find the first instance of each database type" in {
cromwellRunnerAppInstall.toCromwellRunnerAppDatabaseNames(
List(
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName),
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName),
WsmControlledDatabaseResource("tes", tesAzureDbName)
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("cromwell", cromwellAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("tes", tesAzureDbName, wsmResourceId)
) // put cromwellmetadata first to ensure it doesn't get confused with cromwell
) should be(Some(CromwellRunnerAppDatabaseNames(cromwellAzureDbName, tesAzureDbName, cromwellMetadataAzureDbName)))
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.broadinstitute.dsde.workbench.leonardo.app

import cats.effect.IO
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo}
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{azureRegion, landingZoneResources, petUserInfo, wsmResourceId}
import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext
import org.broadinstitute.dsde.workbench.leonardo.{WorkspaceId, WsmControlledDatabaseResource}
import org.broadinstitute.dsde.workbench.leonardo.dao.WdsDAO
Expand All @@ -25,7 +25,7 @@ class WdsAppInstallSpec extends BaseAppInstallSpec {

val wdsAzureDbName = "wds_rtyjga"
val wdsAzureDatabases: List[WsmControlledDatabaseResource] = List(
WsmControlledDatabaseResource("wds", wdsAzureDbName)
WsmControlledDatabaseResource("wds", wdsAzureDbName, wsmResourceId)
)

it should "build wds override values" in {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package org.broadinstitute.dsde.workbench.leonardo.app

import cats.effect.IO
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{landingZoneResources, petUserInfo}
import org.broadinstitute.dsde.workbench.leonardo.CommonTestData.{landingZoneResources, petUserInfo, wsmResourceId}
import org.broadinstitute.dsde.workbench.leonardo.TestUtils.appContext
import org.broadinstitute.dsde.workbench.leonardo.WsmControlledDatabaseResource
import org.broadinstitute.dsde.workbench.leonardo.http.ConfigReader
Expand All @@ -22,8 +22,8 @@ class WorkflowsAppInstallSpec extends BaseAppInstallSpec {
val cbasAzureDbName = "cbas_wgsdoi"
val cromwellMetadataAzureDbName = "cromwellmetadata_tyuiwk"
val workflowsAzureDatabases: List[WsmControlledDatabaseResource] = List(
WsmControlledDatabaseResource("cbas", cbasAzureDbName),
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName)
WsmControlledDatabaseResource("cbas", cbasAzureDbName, wsmResourceId),
WsmControlledDatabaseResource("cromwellmetadata", cromwellMetadataAzureDbName, wsmResourceId)
)

it should "build workflows app override values" in {
Expand Down
Loading