[TOAZ-283] [TOAZ-282] Update Cromwell helm params and move relay listener to terra-app-setup-chart #3036

rtitle · 2022-11-22T15:51:26Z

https://broadworkbench.atlassian.net/browse/TOAZ-282
https://broadworkbench.atlassian.net/browse/TOAZ-283

Depends on:

WX-833 Turn on DRS in Cromwell on Azure, add Martha URL to config broadinstitute/cromwhelm#127
TOAZ-283: Remove relaylistener from cromwhelm/coa-helm broadinstitute/cromwhelm#128
WX-697 Support for getting blob SAS token from WSM in Azure broadinstitute/cromwhelm#129
[TOAZ-283] Add Azure relay listener deployment to terra-app-setup-chart terra-app#26

A few changes:

Moves relaylistener deployment out of cromwell to the terra-app-setup-chart
Passes a few new cromwell helm parameters (WSM URL, workspace storage container id, etc)
Reads recent information from the LZ (postgres server, log analytics workspace) for use in the future

Unit tests pass, planning to test this on a BEE as well.

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

I've documented my API changes in Swagger

In all cases:

Get a thumbsworth of review and PO signoff if necessary
Verify all tests go green (you can re-run automation tests with a comment saying jenkins retest
Run the automation tests multiple times in parallel to weed out instability if applicable via a comment saying multi-test
Squash and merge; Delete your branch after this
Test this change deployed correctly and works on dev environment after deployment

rtitle · 2022-11-22T15:59:35Z

...rc/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/AppServiceInterp.scala

@@ -699,54 +698,77 @@ final class LeoAppServiceInterp[F[_]: Parallel](config: AppServiceConfig,

      // Step 2: call LZ for LZ resources
      lzResourcesByPurpose <- wsmDao.listLandingZoneResourcesByType(landingZoneId, userToken)
+      groupedLzResources = lzResourcesByPurpose.foldMap(a =>


It was bugging me that we were iterating through the full list of LZ resources each time we looked up a resource, so I grouped it to a Map[(purpose, type), resource] first.

rtitle · 2022-11-22T16:02:16Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreter.scala

-        .run(authContext)
+      _ <- assignVmScaleSet(params.landingZoneResources.clusterName, params.cloudContext, petMi)
+
+      // Deploy app chart


Looking forward to when we have more app types on Azure (e.g. after we split out WDS).

I'm treating the storage container as optional, and only required for Cromwell app at the moment.

rtitle · 2022-11-22T16:57:52Z

fiab-start failed, jenkins retest

codecov · 2022-11-22T17:50:54Z

Codecov Report

Merging #3036 (af4a09f) into develop (7d4ecba) will decrease coverage by 0.07%.
The diff coverage is 74.11%.

@@             Coverage Diff             @@
##           develop    #3036      +/-   ##
===========================================
- Coverage    72.95%   72.88%   -0.08%     
===========================================
  Files          125      125              
  Lines        11507    11580      +73     
  Branches       736      716      -20     
===========================================
+ Hits          8395     8440      +45     
- Misses        3112     3140      +28

Impacted Files	Coverage Δ
...nstitute/dsde/workbench/leonardo/azureModels.scala	`0.00% <ø> (ø)`
...dinstitute/dsde/workbench/leonardo/http/Boot.scala	`0.00% <ø> (ø)`
.../leonardo/monitor/LeoPubsubMessageSubscriber.scala	`66.75% <0.00%> (-2.45%)`	⬇️
...e/workbench/leonardo/util/AzurePubsubHandler.scala	`83.33% <66.66%> (ø)`
...dinstitute/dsde/workbench/leonardo/JsonCodec.scala	`76.07% <80.00%> (-0.04%)`	⬇️
.../dsde/workbench/leonardo/util/AKSInterpreter.scala	`83.56% <92.85%> (+1.33%)`	⬆️
...bench/leonardo/http/service/AppServiceInterp.scala	`85.26% <100.00%> (+0.50%)`	⬆️
...rdo/monitor/leoPubsubMessageSubscriberModels.scala	`81.27% <100.00%> (+0.77%)`	⬆️
...orkbench/leonardo/http/service/StatusService.scala	`86.95% <0.00%> (-8.70%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d4ecba...af4a09f. Read the comment docs.

…move relay listener to setup chart

rtitle · 2022-11-30T03:04:30Z

Tested in a BEE, this seems to work: broadinstitute/cromwhelm#128 (comment)

rtitle · 2022-11-30T13:50:44Z

jenkins retest

rtitle · 2022-11-30T14:41:35Z

core/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/azureModels.scala

@@ -18,11 +18,18 @@ final case class WsmJobId(value: String) extends AnyVal
 final case class ManagedIdentityName(value: String) extends AnyVal
 final case class BatchAccountName(value: String) extends AnyVal

+final case class PostgresName(value: String) extends AnyVal
+final case class LogAnalyticsWorkspaceName(value: String) extends AnyVal
+
 final case class LandingZoneResources(clusterName: AKSClusterName,


Adding a few more things to LandingZoneResources since I think we'll need them soon/later.

rtitle · 2022-11-30T14:42:07Z

http/src/main/resources/reference.conf

 }

 app-service {
  # Defaults to true, but disabled for fiab runs
  enable-custom-app-check = true
 }
+
+drs {


The DRS url is now used by Cromwell and Galaxy, so I made it a standalone thing.

this will require firecloud-develop change? also cc @andy7i since he'll likely be updating this in firecloud-develop

Oh yeah -- I can update fc-develop with the new config structure but keep the values the same.

https://github.com/broadinstitute/firecloud-develop/pull/3171 @Qi77Qi

rtitle · 2022-11-30T14:43:12Z

...in/scala/org/broadinstitute/dsde/workbench/leonardo/monitor/LeoPubsubMessageSubscriber.scala

-            Task(ctx.traceId, task, Some(handleKubernetesError), ctx.now, "createAppV2")
-          )
+          for {
+            landingZoneResources <- F.fromOption(


landingZoneResources should be non-optional for Azure apps.

rtitle · 2022-11-30T14:43:44Z

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/util/AKSInterpreter.scala

+
+    } yield ()
+
+  override def deleteApp(params: DeleteAKSAppParams)(implicit ev: Ask[F, AppContext]): F[Unit] = {


Simply moved this method to be next to createApp

Qi77Qi · 2022-11-30T14:49:33Z

http/src/main/resources/reference.conf

 }

 app-service {
  # Defaults to true, but disabled for fiab runs
  enable-custom-app-check = true
 }
+
+drs {


this will require firecloud-develop change? also cc @andy7i since he'll likely be updating this in firecloud-develop

rtitle · 2022-11-30T22:10:56Z

Tests passed! I'm going to merge this to get it in

rtitle commented Nov 22, 2022

View reviewed changes

rtitle force-pushed the rt-toaz-283 branch from 78a9e79 to 6b63d25 Compare November 22, 2022 16:13

rtitle force-pushed the rt-toaz-283 branch 2 times, most recently from 49917a4 to 241255e Compare November 29, 2022 13:22

rtitle added 7 commits November 29, 2022 21:28

Update cromwell helm parameters, read a few more things from the LZ, …

09f7ecd

…move relay listener to setup chart

Unit tests compile

b84c20f

Fix unit tests

3628651

Wire up DRS url to cromwell

4c0ceea

Fix config specs

2002b20

Fix a couple bugs

8f53b95

Post rebase fixes

942f815

rtitle force-pushed the rt-toaz-283 branch from 241255e to 942f815 Compare November 30, 2022 02:43

Fix stupid ConfigReaderSpec

88e6050

Fix deletion flow now that setup chart has pods

81b2a17

rtitle commented Nov 30, 2022

View reviewed changes

rtitle requested review from Qi77Qi, jordansafer and Ghost-in-a-Jar November 30, 2022 14:44

Qi77Qi approved these changes Nov 30, 2022

View reviewed changes

rtitle mentioned this pull request Nov 30, 2022

TOAZ-283: Update Cromwell version to 0.2.172 #3050

Closed

Bump cromwell chart version

7680ede

rtitle mentioned this pull request Nov 30, 2022

WX-833: Update Cromwell version to 0.2.171 #3040

Closed

Fix other spec, why is the chart version in so many places

af4a09f

rtitle merged commit abb0c7e into develop Nov 30, 2022

rtitle deleted the rt-toaz-283 branch November 30, 2022 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOAZ-283] [TOAZ-282] Update Cromwell helm params and move relay listener to terra-app-setup-chart #3036

[TOAZ-283] [TOAZ-282] Update Cromwell helm params and move relay listener to terra-app-setup-chart #3036

rtitle commented Nov 22, 2022 •

edited

Loading

rtitle Nov 22, 2022 •

edited

Loading

rtitle Nov 22, 2022

rtitle commented Nov 22, 2022

codecov bot commented Nov 22, 2022 •

edited

Loading

rtitle commented Nov 30, 2022

rtitle commented Nov 30, 2022

rtitle Nov 30, 2022

rtitle Nov 30, 2022

Qi77Qi Nov 30, 2022

rtitle Nov 30, 2022

rtitle Nov 30, 2022

rtitle Nov 30, 2022

rtitle Nov 30, 2022

Qi77Qi Nov 30, 2022

rtitle commented Nov 30, 2022


		} yield ()

		override def deleteApp(params: DeleteAKSAppParams)(implicit ev: Ask[F, AppContext]): F[Unit] = {

[TOAZ-283] [TOAZ-282] Update Cromwell helm params and move relay listener to terra-app-setup-chart #3036

[TOAZ-283] [TOAZ-282] Update Cromwell helm params and move relay listener to terra-app-setup-chart #3036

Conversation

rtitle commented Nov 22, 2022 • edited Loading

rtitle Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Nov 22, 2022

codecov bot commented Nov 22, 2022 • edited Loading

Codecov Report

rtitle commented Nov 30, 2022

rtitle commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle commented Nov 30, 2022

rtitle commented Nov 22, 2022 •

edited

Loading

rtitle Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading