parallel migration #8857

gites · 2020-04-27T13:12:20Z

This PR speed up keys migration using the vault operator migrate command.
Each key is migrated in a goroutine (the actual part that moves the data) and thus the blocking operation of copying data is handled separately from the part that iterates over objects in Vault. In addition, the number of parallel goroutines migrating the data can be controlled by setting the max_parallel option in the migration config.

Thanks to this change I manage to cut down Vault storage migration time between Consul and GCS from ~3h 30m to ~5m for around 90k of entries and max_parallel=100.

If that looks promising I will update the docs too.

hoerup · 2022-08-31T19:02:18Z

@hsimon-hashicorp what is the prospect of getting this one merged ?

heatherezell · 2022-08-31T20:21:47Z

@hsimon-hashicorp what is the prospect of getting this one merged ?

I'll see what I can do! I've asked for a review from our engineer on rotation - please feel free to ping me again if need be, as well. :)

HridoyRoy

Hi, thanks for submitting this PR!

I've added some review comments. I think a one abstract/general question I have is around testing. There's a duplicate PR here #8077 for this feature, and it would be great if we could port some of the testing logic to the feature branch, or alternatively add some custom testing that would verify the parallelism itself is working as intended.
Thanks again!

HridoyRoy · 2022-09-02T18:31:33Z

command/operator_migrate.go

@@ -44,6 +45,7 @@ type migratorConfig struct {
 	StorageSource      *server.Storage `hcl:"-"`
 	StorageDestination *server.Storage `hcl:"-"`
 	ClusterAddr        string          `hcl:"cluster_addr"`
+	MaxParallel        string          `hcl:"max_parallel"`


max_parallel might cause a bug with the start flag. Could we please make max_parallel a flag instead of a config option and error if both these flags are specified?

Removed in favor of a command flag.

HridoyRoy · 2022-09-02T18:32:43Z

command/operator_migrate_test.go

@@ -55,11 +56,11 @@ func TestMigration(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-
+		maxParallel := "10"


I'm not sure I follow how this tests the parallelism feature. Could you please elaborate on this?

Hard to say after 2,5 years ;). Don't look like this makes any sens here. I set the parallel value to 10 only for DFS test and rest is set to 1.

HridoyRoy · 2022-09-02T18:36:56Z

command/operator_migrate.go

+				defer permitPool.Release()
+				err := cb(ctx, key)
+				if err != nil {
+					return err


I think this error doesn't make it to the outside dfsScan function. Something should be done with the error.

What would you suggest? Sending via channel?

That could work, or using an ErrorGroup should work I think. I can't think of any particular standardization at the moment that I'm particularly partial toward, just that there should be some error handling here. I can circle back with some other folks to see if there's a particular preference or standardization we have as well.

hoerup · 2022-09-13T16:36:55Z

@gites are you still active, and would you be willing on answering the questions?

vercel · 2022-09-13T19:31:29Z

@gites is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

gites · 2022-09-13T19:49:59Z

@HridoyRoy @hoerup I updated the pr and answered some questions.

peteski22 · 2022-09-13T20:50:02Z

command/operator_migrate.go

@@ -135,6 +144,11 @@ func (c *OperatorMigrateCommand) Run(args []string) int {
 	}
 	c.logger = logging.NewVaultLogger(log.LevelFromString(c.flagLogLevel))

+	if (c.flagStart != "") && c.flagParallel != 1 {


Please could we add some sanity check on the c.flagParallel value before this to ensure it's >= 1? You could modify this if statement to check c.flagParallel > 1 after that.

Added flagParallel sanity check for positive int32. Running a migration with flagParallel = 1 is not really a parallel migration so I changed the if here to check for c.flagParallel >= 2

mpalmi · 2022-09-21T18:12:38Z

command/operator_migrate.go

@@ -144,8 +144,13 @@ func (c *OperatorMigrateCommand) Run(args []string) int {
 	}
 	c.logger = logging.NewVaultLogger(log.LevelFromString(c.flagLogLevel))

-	if (c.flagStart != "") && c.flagParallel != 1 {
-		c.UI.Error("Error: flag -start and -parallel can't be used together")
+	if c.flagParallel < 1 || c.flagParallel > 2147483647 {


Can we use math.MaxInt32 instead of a magic number (here and below)?

As a matter of fact, we'll likely not gain much from running math.maxInt32 goroutines in parallel and it may not even be possible. How do you feel about limiting flagParallel to some lower number. Say, 100?

This should actually be fine as just math.maxInt32. We can leave it up to the user to properly tune, but maybe we should specifically call that out in the docs.

mpalmi · 2022-09-23T13:15:06Z

command/operator_migrate.go

+		Name:    "parallel",
+		Default: 1,
+		Target:  &c.flagParallel,
+		Usage: "Use go rutines when migrating. Can speed up the migration " +


Suggested change

Usage: "Use go rutines when migrating. Can speed up the migration " +

Usage: "Use go routines when migrating. Can speed up the migration " +

msheldyakov · 2022-11-14T09:50:17Z

Hello! I would like to finish this PR(it looks like almost everything is already here, there are little things left). @gites what do you think? and thanks for all this work!

gites · 2022-11-14T15:15:00Z

@msheldyakov go ahead and finish it! Good luck!

hghaf099 · 2022-11-22T16:08:50Z

@gites and @msheldyakov There are some comments and questions that we would like to sort out before merging the PR. Would you be interested to respond/address those comments?

peteski22 · 2022-12-05T21:18:01Z

@gites I tried to help you out with some things if you've been too busy to get back to this, appreciate it's been a long lived affair.

gites#1

heatherezell · 2023-01-24T00:39:36Z

Hi folks, are we still working on this? Otherwise, our engineers may be able to fork it and make some progress and get it merged. Thanks in advance!

peteski22 · 2023-01-24T09:24:07Z

Hi folks, are we still working on this? Otherwise, our engineers may be able to fork it and make some progress and get it merged. Thanks in advance!

Hey @hsimon-hashicorp, it may well be that the original contributor just hasn't the time now to continue this and it would be a shame to see their efforts wasted. I'm happy to push my fork of @gites' work into a HashiCorp branch so we can try and wrap it up.

I'll leave this comment up for a few days in case @gites and other co-authors want to finish it themselves, and failing a response, or a response to suggest we 'go ahead' I think we should do just that. :)

peteski22 · 2023-01-24T13:28:21Z

Hi folks, are we still working on this? Otherwise, our engineers may be able to fork it and make some progress and get it merged. Thanks in advance!

Hey @hsimon-hashicorp, it may well be that the original contributor just hasn't the time now to continue this and it would be a shame to see their efforts wasted. I'm happy to push my fork of @gites' work into a HashiCorp branch so we can try and wrap it up.

I'll leave this comment up for a few days in case @gites and other co-authors want to finish it themselves, and failing a response, or a response to suggest we 'go ahead' I think we should do just that. :)

Oh I see that someone else already asked and was granted permission to finish this off (in the comments above) #8857 (comment). Since nothing is happening on the PR I will start trying to move into 'into' HashiCorp.

gites · 2023-01-24T16:51:40Z

@hsimon-hashicorp @peteski22 Yes, go ahead

gites force-pushed the parallel-migration branch 2 times, most recently from f47c0b3 to 6af20d8 Compare April 27, 2020 15:54

gites mentioned this pull request Apr 29, 2020

Support for parallel storage migration #8883

Closed

heatherezell added core/migration enhancement labels Aug 8, 2022

HridoyRoy suggested changes Sep 2, 2022

View reviewed changes

HridoyRoy requested a review from peteski22 September 6, 2022 16:18

gites force-pushed the parallel-migration branch from 286ead5 to 78452f6 Compare September 13, 2022 19:05

gites requested a review from a team September 13, 2022 19:05

gites requested review from a team and austingebauer as code owners September 13, 2022 19:05

gites requested review from a team, mdeggies and jeanneryan and removed request for a team September 13, 2022 19:05

bump

35ca46b

gites force-pushed the parallel-migration branch from 78452f6 to 35ca46b Compare September 13, 2022 19:31

gites added 2 commits September 13, 2022 21:34

Merge branch 'main' into parallel-migration

2cdc047

bump2

840ceb9

peteski22 reviewed Sep 13, 2022

View reviewed changes

flagParallel sanity check

6da0c50

mpalmi reviewed Sep 21, 2022

View reviewed changes

mpalmi reviewed Sep 23, 2022

View reviewed changes

peteski22 added the waiting-for-response label Nov 22, 2022

maxcoulombe removed the request for review from a team January 19, 2023 16:00

peteski22 mentioned this pull request Jan 24, 2023

Parallel migration #18815

Merged

peteski22 mentioned this pull request Jan 24, 2023

Parallel migration (#18815) #18817

Merged

peteski22 closed this Jan 25, 2023

github-actions bot removed the waiting-for-response label Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parallel migration #8857

parallel migration #8857

gites commented Apr 27, 2020

hoerup commented Aug 31, 2022

heatherezell commented Aug 31, 2022

HridoyRoy left a comment

HridoyRoy Sep 2, 2022

gites Sep 13, 2022

HridoyRoy Sep 2, 2022

gites Sep 13, 2022

HridoyRoy Sep 2, 2022

gites Sep 13, 2022

HridoyRoy Sep 13, 2022

hoerup commented Sep 13, 2022

vercel bot commented Sep 13, 2022

gites commented Sep 13, 2022

peteski22 Sep 13, 2022

gites Sep 14, 2022

mpalmi Sep 21, 2022

mpalmi Sep 21, 2022

mpalmi Sep 23, 2022

mpalmi Sep 23, 2022

msheldyakov commented Nov 14, 2022

gites commented Nov 14, 2022

hghaf099 commented Nov 22, 2022

peteski22 commented Dec 5, 2022

heatherezell commented Jan 24, 2023

peteski22 commented Jan 24, 2023

peteski22 commented Jan 24, 2023

gites commented Jan 24, 2023

	Usage: "Use go rutines when migrating. Can speed up the migration " +
	Usage: "Use go routines when migrating. Can speed up the migration " +

parallel migration #8857

parallel migration #8857

Conversation

gites commented Apr 27, 2020

hoerup commented Aug 31, 2022

heatherezell commented Aug 31, 2022

HridoyRoy 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

hoerup commented Sep 13, 2022

vercel bot commented Sep 13, 2022

gites commented Sep 13, 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

msheldyakov commented Nov 14, 2022

gites commented Nov 14, 2022

hghaf099 commented Nov 22, 2022

peteski22 commented Dec 5, 2022

heatherezell commented Jan 24, 2023

peteski22 commented Jan 24, 2023

peteski22 commented Jan 24, 2023

gites commented Jan 24, 2023