[Backend] Improve lifecycle management of the artifact storage migration #1146

kenxu95 · 2023-03-28T22:16:24Z

Lead reviewers: @saurav-c and @likawind.

Describe your changes and why you are making these changes

This is a WIP process PR, but the critical backend parts have been changed and tested. I would like some preliminary feedback on 1) the data model and 2) the updated logic in connect_integration.go to make sure there are no surprises when the rest of the work is fleshed out.

Testing:

Tested that I'm able to migration to an S3 bucket, and from that S3 bucket to another S3 bucket, and that the storage migration table is in the appropriate state afterwards.

I also updated configure_storage.go later following the same pattern. Once the latter is updated we'll be able to migrate from S3 back to local.

There will be a follow-up for the UI changes, as well as any additional backend routes we need to support those UI changes.

Known issues that I will address (or make tasks for):

We should perform the deletion of contents in the old storage layer AFTER updating the global storage config. Otherwise, this can lead to an inconsistent and broken state. It's not a huge deal if we accidentally leave some files behind.
There's currently no way of testing s3 -> local filesystem migration through the UI or SDK? (Task created)
We should probably lock the storage_migration table so that only one migration can be ongoing at a time. (Task created)
There should be an enforced timeout for the migration process. (Task created)
Disallow deleting an integration that is also being used as the current metadata store.
[Nice-to-have] Migrating to the same S3 bucket, or migrating local->local should error. This currently works for some reason, but we can probably disallow it earlier.

Related issue number (if any)

ENG-2604

Loom demo (if any)

https://www.loom.com/share/e04f228644f24c2eb0b0b583503c8abd

Checklist before requesting a review

I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
I have performed a self-review of my code.
I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
If this is a new feature, I have added unit tests and integration tests.
I have run the integration tests locally and they are passing.
I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
All features on the UI continue to work correctly.
Added one of the following CI labels:
- run_integration_test: Runs integration tests
- skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

likawind

Great work! Left a few comments

src/golang/cmd/server/handler/v2/list_storage_migrations.go

likawind · 2023-03-30T18:49:16Z

src/golang/cmd/server/routes/headers.go

@@ -12,6 +12,11 @@ const (
 	IntegrationConfigHeader  = "integration-config"
 	IntegrationIDsHeader     = "integration-ids"

+	// Storage Migration Headers


based on some other APIs like https://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/get_dags , I'd suggest using request body to send / capture a json-serialized query parameters

I don't think any of these are json'd though?

actually you are right, but it looks like they are query parameters (/storage-migrations?limit=5) tough?

cc @jpurusho65 as you may have better experience on API pattern

likawind · 2023-03-30T18:50:03Z

src/golang/lib/repos/tests/storage_migration.go

@@ -0,0 +1,3 @@
+package tests
+
+// TODO


Just confirming this is an immediate TODO in follow-up PR?

I will do this as part of this PR.

likawind · 2023-03-30T18:55:48Z

src/golang/lib/storage_migration/list.go

+
+	// Apply the filters in the following order: status, completedSince, then limit.
+	if status != nil {
+		migrations = filterStorageMigrationsByStatus(migrations, *status)


how hard is it to push these filtering / limit to query layer? I don't have particular concerns on this use case, but if it's something easily built, we can avoid potential pitfalls in future

It's not too hard but it is a little tricky and I felt like there probably aren't enough storage migrations for listing them all to matter.

src/golang/cmd/server/handler/connect_integration.go

likawind · 2023-03-30T19:11:43Z

src/golang/cmd/server/handler/connect_integration.go

+	}
+
+	// Only need to do this if we're committing a transaction.
+	if txn, ok := db.(database.Transaction); ok {


What does this block do? It feels like an antipattern to me - all transaction owner (whoever calls txn := db.BeginTx()) should be responsible for calling Commit() and TxnRollBack(). I'm confused why we need to access some transaction this helper didn't create itself.

It's a little confusing, I agree. Refactored it so that the transaction definition happens at the top of the function.

saurav-c

Left initial comments, still working through the rest of the PR.

src/golang/cmd/server/handler/connect_integration.go

kenxu95 · 2023-03-31T18:39:55Z

@saurav-c @likawind ok this PR should have all the components ready now. The UI part is here: #1155

src/golang/lib/models/shared/execution_state.go

likawind

Thanks for addressing comments, nothing major from my end!

src/golang/cmd/server/handler/connect_integration.go

saurav-c

Left a couple minor comments, otherwise LGTM

src/golang/cmd/server/handler/get_config.go

saurav-c · 2023-03-31T20:59:42Z

src/golang/cmd/server/handler/v2/list_storage_migrations.go

+	*aq_context.AqContext
+
+	// See the route description above for what each of these filters mean.
+	status         *string


Why does this need to be a pointer, same with completedSince?

Those are optional fields that will be left as nil if the request doesn't want to apply those filters. I'll document it here better.

kenxu95 · 2023-04-01T18:46:20Z

Pre-merge tests already ran, so skipping them when merging in UI changes.

…1155)

…igration (#1146)

kenxu95 requested review from cw75, likawind and saurav-c March 28, 2023 22:16

kenxu95 force-pushed the eng-2604-s3-migration-error-failing-should-not branch from f4de02c to ef87ff7 Compare March 29, 2023 23:18

kenxu95 added 5 commits March 29, 2023 17:25

put the skeleton of the db calls together

be4745c

finished making changes to connect_integration. Ready to test.

a040ece

tweaked connect_integration a tad bit more

1434238

context swtich

e80a4c1

ready for preliminary review

770ed7d

kenxu95 force-pushed the eng-2604-s3-migration-error-failing-should-not branch from ef87ff7 to 770ed7d Compare March 30, 2023 00:25

kenxu95 added 2 commits March 30, 2023 11:14

pull in all the backen dchanges

1f1e16a

deleted vault/storage keys best-effort afterwards

adb2aae

likawind reviewed Mar 30, 2023

View reviewed changes

saurav-c reviewed Mar 30, 2023

View reviewed changes

src/golang/cmd/server/handler/connect_integration.go Outdated Show resolved Hide resolved

src/golang/cmd/server/handler/connect_integration.go Outdated Show resolved Hide resolved

kenxu95 added 5 commits March 30, 2023 13:27

seems to work after big refactor

b711f0d

ready to merge in configure_storage.go

3682538

merged in backend for configure_store.py

2a9d1e2

added backtick

b1da05d

intiial draft

b5931ba

kenxu95 changed the title ~~[Feedback Requested][Backend] Improve lifecycle management of the S3 migration~~ [Backend] Improve lifecycle management of the S3 migration Mar 31, 2023

kenxu95 added 3 commits March 30, 2023 17:47

context switch

24d5399

pulled in changes

357cbf3

ready for review

4db640e

kenxu95 changed the title ~~[Backend] Improve lifecycle management of the S3 migration~~ [Backend] Improve lifecycle management of the artifact storage migration Mar 31, 2023

kenxu95 mentioned this pull request Mar 31, 2023

[UI] Improve lifecycle management of the artifact storage migration #1155

Merged

8 tasks

kenxu95 added 2 commits March 31, 2023 10:24

fixed golang tests

2bac529

fixed up tests. Now ready for review again

4bde825

kenxu95 requested review from likawind and saurav-c March 31, 2023 18:39

likawind reviewed Mar 31, 2023

View reviewed changes

src/golang/lib/models/shared/execution_state.go Show resolved Hide resolved

likawind approved these changes Mar 31, 2023

View reviewed changes

src/golang/cmd/server/handler/connect_integration.go Outdated Show resolved Hide resolved

saurav-c approved these changes Mar 31, 2023

View reviewed changes

last cr

c108359

kenxu95 added run_integration_test Triggers integration tests skip_integration_test Skips required integration test (only documentation/UI changes) and removed run_integration_test Triggers integration tests labels Apr 1, 2023

[UI] Improve lifecycle management of the artifact storage migration (#…

9945822

…1155)

kenxu95 merged commit a32206c into main Apr 1, 2023

kenxu95 deleted the eng-2604-s3-migration-error-failing-should-not branch April 1, 2023 18:54

jpurusho65 pushed a commit that referenced this pull request Apr 3, 2023

[Backend] [UI] Improve lifecycle management of the artifact storage m…

55db0aa

…igration (#1146)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backend] Improve lifecycle management of the artifact storage migration #1146

[Backend] Improve lifecycle management of the artifact storage migration #1146

kenxu95 commented Mar 28, 2023 •

edited

Loading

likawind left a comment

likawind Mar 30, 2023

kenxu95 Mar 31, 2023

likawind Mar 31, 2023

likawind Mar 30, 2023

kenxu95 Mar 31, 2023

likawind Mar 30, 2023

kenxu95 Mar 31, 2023

likawind Mar 30, 2023

kenxu95 Mar 31, 2023

saurav-c left a comment

kenxu95 commented Mar 31, 2023

likawind left a comment

saurav-c left a comment

saurav-c Mar 31, 2023

kenxu95 Mar 31, 2023 •

edited

Loading

kenxu95 commented Apr 1, 2023

[Backend] Improve lifecycle management of the artifact storage migration #1146

[Backend] Improve lifecycle management of the artifact storage migration #1146

Conversation

kenxu95 commented Mar 28, 2023 • edited Loading

Describe your changes and why you are making these changes

Related issue number (if any)

Loom demo (if any)

Checklist before requesting a review

likawind 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

saurav-c left a comment

Choose a reason for hiding this comment

kenxu95 commented Mar 31, 2023

likawind left a comment

Choose a reason for hiding this comment

saurav-c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

kenxu95 commented Apr 1, 2023

kenxu95 commented Mar 28, 2023 •

edited

Loading

kenxu95 Mar 31, 2023 •

edited

Loading