Add support for UC volumes in DABs #1762

shreyas-goenka · 2024-09-09T11:11:00Z

Changes

This PR adds the UC volumes resource to DABs. This PR also adds a warning if a user tries to use a volume in the artifact_path that is managed by DABs and has not been deployed yet. Users would need to do two incremental deploys in this case because we always upload artifacts before creating all the resources.

A proper fix where we can allow using the volume within the same deploy would require us to head in a direction of removing our dependency on terraform to manage the CRUD lifecycle of DABs resources.

This PR also has custom behaviour for UC volumes, where presents.name_prefix or the default prefix is only applied on a volume when the catalog or schema is not already scoped to the user.

Tests

Unit, integration test and manually.

Manual outputs:

UC volume does not exist:

➜  bundle-playground git:(master) ✗ cli bundle deploy
Error: failed to fetch metadata for the UC volume /Volumes/main/caps/my_volume that is configured in the artifact_path: Not Found

UC Volume does not exist, but is defined in DAB

➜  bundle-playground git:(master) ✗ cli bundle deploy
Error: failed to fetch metadata for the UC volume /Volumes/main/caps/managed_by_dab that is configured in the artifact_path: Not Found

Warning: You might be using a UC volume in your artifact_path that is managed by this bundle but which has not been deployed yet. Please deploy the UC volume in a separate bundle deploy before using it in the artifact_path.
  at resources.volumes.bar
  in databricks.yml:24:7

shreyas-goenka · 2024-09-16T01:13:27Z

bundle/config/resources/volume.go

+
+	// TODO: Are there fields in the edit API or terraform that are not in this struct?
+	// If so call it out in the PR.
+	*catalog.CreateVolumeRequestContent


Notably OWNER is missing from the schema here. Terraform allows managing the owner, but it's not available in the create API.

We don't do owner assignment elsewhere, can you clarify the significance?

No real significance for DABs. I just wanted to call this out since it's a divergence from Terraform. In fact, this is nice because it clearly guarantees that the owner of the UC volume is the deploying user. We do not allow modifying the owner of any of our UC resources.

shreyas-goenka · 2024-09-16T01:16:03Z

bundle/phases/deploy.go


 	// We don't need to display any prompts in this case.
-	if len(dltActions) == 0 && len(schemaActions) == 0 {
+	if len(schemaActions) == 0 && len(dltActions) == 0 && len(volumeActions) == 0 {


This PR introduces yet again interactive flow to DABs. At this point, I think it's worth it to invest some time in adding proper end to end tests for this. I'd like to pick it up in the next ~month.

Let's pick this next quarter as in scope for terminal UX improvements.

shreyas-goenka · 2024-09-16T02:12:16Z

Nightlies passed once. Triggered another round after some minor updates.

shreyas-goenka · 2024-10-15T16:12:28Z

bundle/config/mutator/apply_presets_test.go

+	assert.False(t, ok,
+		`Since you are adding support for UC catalogs to DABs please ensure that
+you add logic to skip applying presets.name_prefix for UC schemas, UC volumes and
+any other resources that fall under a catalog in the UC hierarchy (like registered models).


Do we want to retroactively change the prefix behaviour for registered models as well to keep it consistent? No one has complained yet so it's probably fine to keep as is.

shreyas-goenka · 2024-10-16T11:43:14Z

bundle/phases/deploy.go


 	// We don't need to display any prompts in this case.
-	if len(dltActions) == 0 && len(schemaActions) == 0 {
+	if len(schemaActions) == 0 && len(dltActions) == 0 && len(volumeActions) == 0 {


Let's pick this next quarter as in scope for terminal UX improvements.

shreyas-goenka · 2024-10-16T11:56:39Z

bundle/config/mutator/apply_presets.go

@@ -193,6 +195,30 @@ func (m *applyPresets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos
 		// the Databricks UI and via the SQL API.
 	}

+	// Apply the prefix to volumes
+	for _, v := range r.Volumes {


Manually verified that this indeed works end to end.

pietern · 2024-10-31T10:30:17Z

bundle/config/resources/volume.go

+
+	// TODO: Are there fields in the edit API or terraform that are not in this struct?
+	// If so call it out in the PR.
+	*catalog.CreateVolumeRequestContent


We don't do owner assignment elsewhere, can you clarify the significance?

bundle/libraries/filer.go

bundle/phases/deploy.go

pietern · 2024-10-31T10:44:11Z

internal/bundle/artifacts_test.go

+			},
+		}
+
+		diags := bundle.Apply(ctx, b, bundle.Seq(libraries.ExpandGlobReferences(), libraries.Upload()))


Which mutator triggers the error?

internal/bundle/artifacts_test.go

internal/bundle/bundles/uc_volume/databricks_template_schema.json

bundle/config/mutator/apply_presets.go

pietern · 2024-10-31T11:00:55Z

bundle/config/mutator/apply_presets.go

+			log.Debugf(ctx, "Skipping prefix for volume %s because schema %s is defined in the bundle and the schema name will be interpolated at runtime", v.Name, v.SchemaName)
+			continue
+
+		}


This feels magical and has the risk of the same issue we have with schemas today.

To avoid we could make prefixing opt-in through a preset, and recommend setting it in development mode unless there is signal that prefixing is already done. Then it will be a conscious decision at all times. We can discuss more but this should be in line with what we do for schemas (and later also the registered models).

github-actions · 2024-10-31T17:01:57Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

PR number: 1762
Commit SHA: 1218178e64844a091fb06293b6fc14552273942b

Checks will be approved automatically on success.

eng-dev-ecosystem-bot · 2024-10-31T17:02:35Z

Test Details: go/deco-tests/11616451005

shreyas-goenka added 25 commits September 9, 2024 13:10

first comment

f772ce4

Merge remote-tracking branch 'origin' into feature/uc-volumes

8f4f3ae

fix convertor and add unit test

ce5792c

run as support

7c7abef

-

d04b6b0

add apply target mode prefix functionality

9b66cd5

add conversion tests

4b22e2d

add inteprolation for volumes fields

88d0402

add prompt and crud test for volumes

6f9817e

Merge remote-tracking branch 'origin' into feature/uc-volumes

d47b0d6

add filer

d180bab

Merge remote-tracking branch 'origin' into feature/uc-volumes

fa54577

fix test and improve error

73826ac

-

de7eb94

-

f10038a

add integration tests for artifacts portion:

df3bbad

unit test for comparision of locatoin

aeab4ef

cleanup and add test

aa2e16d

fix unit tests

a90eb57

fix test on windows

39cb5e8

cleanup todos

13748f1

-

bdecd08

iter

274fd63

-

d3d5d4c

fixes

227dfe9

shreyas-goenka marked this pull request as ready for review September 16, 2024 02:10

shreyas-goenka commented Sep 16, 2024

View reviewed changes

shreyas-goenka requested review from pietern and andrewnester September 16, 2024 02:11

shreyas-goenka added 4 commits October 15, 2024 15:27

-

eb94cd6

fix test

3e3ddfd

add integration test for grant on volume

d241c2b

add custom prefixing behaviour for volumes

6192835

shreyas-goenka commented Oct 15, 2024

View reviewed changes

shreyas-goenka added 3 commits October 15, 2024 22:03

fmt

701b178

-

810da66

-

1a961eb

shreyas-goenka commented Oct 16, 2024

View reviewed changes

shreyas-goenka requested a review from pietern October 16, 2024 11:56

merge

8a2fe49

shreyas-goenka temporarily deployed to test-trigger-is October 29, 2024 10:38 — with GitHub Actions Inactive

pietern reviewed Oct 31, 2024

View reviewed changes

shreyas-goenka added 2 commits October 31, 2024 15:10

Merge remote-tracking branch 'origin' into feature/uc-volumes

49b2cf2

use IsVolumesPath

e32ebd0

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 14:25 — with GitHub Actions Inactive

better message

6b12234

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 14:57 — with GitHub Actions Inactive

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 14:58 — with GitHub Actions Inactive

address comments

f9287e0

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 16:53 — with GitHub Actions Inactive

rename to volume

250d426

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 17:00 — with GitHub Actions Inactive

-

1218178

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 17:01 — with GitHub Actions Inactive

shreyas-goenka temporarily deployed to test-trigger-is October 31, 2024 17:02 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for UC volumes in DABs #1762

Add support for UC volumes in DABs #1762

shreyas-goenka commented Sep 9, 2024 •

edited

Loading

shreyas-goenka Sep 16, 2024

pietern Oct 31, 2024

shreyas-goenka Oct 31, 2024

shreyas-goenka Sep 16, 2024

shreyas-goenka Oct 16, 2024

shreyas-goenka commented Sep 16, 2024

shreyas-goenka Oct 15, 2024

shreyas-goenka Oct 16, 2024

shreyas-goenka Oct 16, 2024

pietern Oct 31, 2024

pietern Oct 31, 2024

pietern Oct 31, 2024

github-actions bot commented Oct 31, 2024

eng-dev-ecosystem-bot commented Oct 31, 2024

Add support for UC volumes in DABs #1762

Are you sure you want to change the base?

Add support for UC volumes in DABs #1762

Conversation

shreyas-goenka commented Sep 9, 2024 • edited Loading

Changes

Tests

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

shreyas-goenka commented Sep 16, 2024

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

github-actions bot commented Oct 31, 2024

eng-dev-ecosystem-bot commented Oct 31, 2024

shreyas-goenka commented Sep 9, 2024 •

edited

Loading