[WOR-1781][WOR-1782] APIs to manage workspace settings #2983

marctalbott · 2024-08-08T21:29:09Z

Add workspace settings APIs with support for setting lifecycle rules on GCP workspace buckets

PR checklist

Include the JIRA issue number in the PR description and title
Make sure Swagger is updated if API changes
- ...and Orchestration's Swagger too!
If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
Get two thumbsworth of PR review
Verify all tests go green, including CI tests
Squash commits and merge to develop (branches are automatically deleted after merging)
Inform other teams of any substantial changes via Slack and/or email

aednichols

Nice, this looks awesome.

I'm currently working on making Rawls instruct Cromwell to segregate workflow files into submissions/intermediates and submissions/final-outputs.

Do you think it makes sense to enable that behavior with a setting instead of forcing it on everyone?

{
    "type": "SegregateFinalOutputs",
    "config": {
        "enable_segregation": true
    }
}

marctalbott · 2024-08-09T13:03:36Z

@aednichols You certainly could do that with settings! You would know better than me whether it should be forced on everyone, but if you expect that users may prefer different segregation behavior then adding a setting type may be the best way to go about that.

aherbst-broad

Looking good! Can you describe your thinking around handling concurrent requests? I know we talked a bit about a constraint etc. but I don't see evidence of that here -- it'd be good to go in knowing how this will behave in the face of simultaneous requests to mutate settings.

aherbst-broad · 2024-08-09T12:55:52Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceRepository.scala

@@ -99,4 +102,55 @@ class WorkspaceRepository(dataSource: SlickDataSource) {
      } yield newWorkspace
    }

+  def getWorkspaceSettings(workspaceId: UUID): Future[List[WorkspaceSetting]] =


All of these methods could use comments, esp. to explain the semantics around only returning Applied, the expected calling pattern etc.

aherbst-broad · 2024-08-09T13:01:07Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

@@ -2001,6 +2005,115 @@ class WorkspaceService(
      }
    } yield {}

+  def getWorkspaceSettings(workspaceName: WorkspaceName): Future[List[WorkspaceSetting]] =


What do you think about placing these methods in a more focused WorkspaceSettingsService class or some such thing -- this service is already very large.

I considered it, but I ultimately didn't as I felt that managing settings belonged in the WorkspaceService as much as anything else. Still, I'm not opposed to it and it would certainly make changing it easier given how much Intellij struggles with the WorkspaceService

aherbst-broad · 2024-08-09T13:03:03Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+      } else {
+        val requestedSettingsMap = requestedSettings.map(s => s.`type` -> s.config).toMap
+        existingSettings.map { case WorkspaceSetting(settingType, _) =>
+          WorkspaceSetting(settingType, requestedSettingsMap.getOrElse(settingType, settingType.defaultConfig()))


Does this mean a previously configured setting will assume the default on any new settings request, regardless if it's included or not? I think it should retain the existing setting, if that's the case.

Yeah, it does. My thinking was that this would be a simple overwrite -- any settings provided become the new settings on the workspace and old settings are reverted to the default if not respecified. It's pretty heavy-handed and requires the caller to do more work, so I'm definitely not attached to it.

aherbst-broad · 2024-08-09T13:08:11Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+        case _ => throw new RawlsException("unsupported workspace setting")
+      }).recoverWith { case e =>
+        workspaceRepository
+          .removePendingSetting(workspace.workspaceIdAsUUID, setting.`type`)


Can e be logged somewhere? It looks like it's being swallowed which will make diagnosing issues here difficult. Agreed it should not be surfaced in the error report.

Absolutely, I'll log it

aherbst-broad · 2024-08-09T13:09:58Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+
+          for {
+            _ <- gcsDAO.setBucketLifecycle(workspace.bucketName, googleRules)
+            _ <- workspaceRepository.markWorkspaceSettingApplied(workspace.workspaceIdAsUUID, setting.`type`)


Making sure I understand: not only will this shift the newly inserted setting record from PENDING -> APPLIED, it will mark any previous settings of this type for the workspace from APPLIED -> DELETED. Is that right?

That's correct

aherbst-broad · 2024-08-09T13:18:08Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+        case WorkspaceSetting(WorkspaceSettingTypes.GcpBucketLifecycle, GcpBucketLifecycleConfig(rules)) =>
+          val googleRules = rules.map { rule =>
+            val conditionBuilder =
+              LifecycleCondition.newBuilder().setMatchesPrefix(rule.conditions.matchesPrefix.toList.asJava)


Is it possible rule, or rule.conditions or rule.conditions.matchesPrefix are null here?

No, we constructed setting here by parsing the json and none of those fields are optional. We'd throw a NoSuchElementException if any of them were null. I'll add some tests that assert that similar to this test that checks for the presence of config

aherbst-broad · 2024-08-09T13:19:07Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+              case actionType => Some(validationErrorReport(settingType, s"unsupported lifecycle action $actionType"))
+            }
+            val ageValidation = rule.conditions.age.collect {
+              case age if age < 0 =>


Should this be <= 0?

No, you can set age to 0 to delete immediately. We actually do it when deleting a bucket - deleteBucket

aherbst-broad · 2024-08-09T13:20:42Z

model/src/main/scala/org/broadinstitute/dsde/rawls/model/WorkspaceModel.scala

+
+  case class GcpBucketLifecycleRule(action: GcpBucketLifecycleAction, conditions: GcpBucketLifecycleCondition)
+  case class GcpBucketLifecycleAction(`type`: String)
+  case class GcpBucketLifecycleCondition(matchesPrefix: Set[String], age: Option[Int])


Can age be more strongly typed instead of an Int? I worry about mixing up seconds/hours/days etc.

I could add type Days = Int to the object so this would read as:

case class GcpBucketLifecycleCondition(matchesPrefix: Set[String], age: Option[Days])

Everything else would remain the same as it's functionally unchanged, but it would make it clearer to future devs

aherbst-broad · 2024-08-09T13:21:32Z

model/src/main/scala/org/broadinstitute/dsde/rawls/model/WorkspaceModel.scala

+  case class GcpBucketLifecycleConfig(rules: List[GcpBucketLifecycleRule]) extends WorkspaceSettingConfig
+
+  case class GcpBucketLifecycleRule(action: GcpBucketLifecycleAction, conditions: GcpBucketLifecycleCondition)
+  case class GcpBucketLifecycleAction(`type`: String)


Nit: I would prefer not to have to backtick every usage of this param, but I am lazy.

That's fair. I avoided it initially with WorkspaceSetting because I'm also not a fan, but then I wanted this to mimic the Google lifecycle rules format as closely as possible so I ended up caving and using backticks for both since they use type.

aherbst-broad · 2024-08-09T13:22:21Z

core/src/main/resources/swagger/api-docs.yaml

+          enum:
+            - Delete
+    GcpBucketLifecycleCondition:
+      type: object


Are any of these properties required?

Either can be omitted, but we do need at least one now that I think about it which isn't accounted for. I'll change that.

marctalbott · 2024-08-09T14:06:04Z

Looking good! Can you describe your thinking around handling concurrent requests? I know we talked a bit about a constraint etc. but I don't see evidence of that here -- it'd be good to go in knowing how this will behave in the face of simultaneous requests to mutate settings.

@aherbst-broad Certainly. WorkspaceRepository checks for existing pending settings before saving any new settings at the beginning of the request (source). If there are any pending settings, it will throw an exception. Your question did get me thinking that we could still be at risk of phantom reads and end up with multiple Pending settings, so perhaps we should change the isolation level to Serializable. I'm not super familiar with isolation levels, but does that sound reasonable to you?

… settings change, enforce at least one condition, make prefixes an Option

…ettings

blakery

Looks good with the repo split out

blakery · 2024-08-14T13:30:45Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

 import org.broadinstitute.dsde.rawls.billing.BillingRepository
+import org.broadinstitute.dsde.rawls.model.WorkspaceSettingConfig._


Are these imports actually needed?
I don't see any code changes, so I'm wondering of maybe they're left over from changes that were moved elsewhere?

blakery · 2024-08-14T19:34:01Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceRepository.scala

@@ -3,18 +3,23 @@ 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.WorkspaceSettingRecord


Nit: unused imports from when the settings were a part of this class

marctalbott added 13 commits August 7, 2024 15:26

liquibase changeset to create workspace settings table

097daee

wip, compiles but untested

1c2287a

swagger

a4536df

only return applied settings

01e0d27

rename WorkspaceSetting, handle empty settings, simplify rollback

e541f61

return errors alongside successes, soft delete settings

3dc24ec

bug fixes, require config, add validation

bbde9b5

start unit tests

03d7e31

fix tests

6259e9a

more unit tests, rework some db things

75301c5

repository tests

460df06

parsing tests and rename lifecycle case classes

39342b5

require config and update lastUpdated when updating setting record

029d41d

marctalbott requested review from a team, blakery and aherbst-broad and removed request for a team August 8, 2024 21:30

aednichols reviewed Aug 8, 2024

View reviewed changes

marctalbott added 2 commits August 9, 2024 08:54

cleanup, move lifecycle action verification earlier

73a61e3

scalafmt

5178b1e

aherbst-broad reviewed Aug 9, 2024

View reviewed changes

marctalbott marked this pull request as ready for review August 9, 2024 15:34

marctalbott added 6 commits August 9, 2024 14:52

change transaction isolation level, add comments, track user who made…

2c30dba

… settings change, enforce at least one condition, make prefixes an Option

no backticks

5dde8bb

don't fully overwrite, update; correct field name; handle identical s…

b6e416a

…ettings

separate settings service

7b538be

separate validation test cases

7686821

swagger and fix provider routes

13c934d

marctalbott added 3 commits August 13, 2024 15:38

fix unit tests

c5232c2

scalafmt

118f779

break out WorkspaceSettingRepository

45ab826

blakery approved these changes Aug 14, 2024

View reviewed changes

remove leftover imports

90b7f6b

aherbst-broad approved these changes Aug 16, 2024

View reviewed changes

marctalbott merged commit bd2e5eb into develop Aug 16, 2024
30 checks passed

marctalbott deleted the mtalbott-workspace-settings branch August 16, 2024 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WOR-1781][WOR-1782] APIs to manage workspace settings #2983

[WOR-1781][WOR-1782] APIs to manage workspace settings #2983

marctalbott commented Aug 8, 2024 •

edited by jira bot

Loading

aednichols left a comment

marctalbott commented Aug 9, 2024

aherbst-broad left a comment

aherbst-broad Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024 •

edited

Loading

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

aherbst-broad Aug 9, 2024

marctalbott Aug 9, 2024

marctalbott commented Aug 9, 2024

blakery left a comment

blakery Aug 14, 2024

blakery Aug 14, 2024

		import org.broadinstitute.dsde.rawls.billing.BillingRepository
		import org.broadinstitute.dsde.rawls.model.WorkspaceSettingConfig._

[WOR-1781][WOR-1782] APIs to manage workspace settings #2983

[WOR-1781][WOR-1782] APIs to manage workspace settings #2983

Conversation

marctalbott commented Aug 8, 2024 • edited by jira bot Loading

aednichols left a comment

Choose a reason for hiding this comment

marctalbott commented Aug 9, 2024

aherbst-broad left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aherbst-broad Aug 9, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctalbott commented Aug 9, 2024

blakery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctalbott commented Aug 8, 2024 •

edited by jira bot

Loading

aherbst-broad Aug 9, 2024 •

edited

Loading