Goto command support to handle dirty Database state #36

venkat-iblox · 2024-09-23T15:13:57Z

This PR introduces improvements to the migration workflow by implementing robust handling for dirty states, effective failure management, and cleanup of newer migration files.

Goto command can handle the reversal of dirty DB state if the following args are set:

-force-dirty-handling: Lets the goto module know that the command has to handle the dirty state(required)
-cache-dir: The path where the current migrations(successfully executed) are stored. This is usually a mounted volume to the migration container and persists beyond the migration container's lifecycle.

Key Changes:

Handle Dirty State:

Implements HandleDirtyState to apply the last successful migration when a dirty state is detected.

Migration Failure Management:

Integrated HandleMigrationFailure to capture and store the last successful migration version in case of failure, ensuring available recovery options.

File Cleanup:

Adds CleanupFiles to remove migration files with versions greater than the current migration version after successful execution, maintaining a clean working directory.

Added UTs for the new functionality.

daniel-garcia · 2024-09-23T17:25:39Z

cmd/migrate/config.go

@@ -35,4 +35,8 @@ var (

 	flagConfigDirectory = pflag.String("config.source", defaultConfigDirectory, "directory of the configuration file")
 	flagConfigFile      = pflag.String("config.file", "", "configuration file name without extension")
+
+	// goto command flags
+	flagDirty   = pflag.Bool("dirty", false, "migration is dirty")


maybe this should be a verb instead of a noun. for example, 'handle-dirty', 'force-dirty-handling'. i'm open to the naming here but dirty by itself is not invocative about what this does.

force-dirty-handling seems clear and concise on what it is supposedly doing

That works for me

I would also suggest to update the description of the flag.

venkat-iblox · 2024-09-23T17:38:13Z

@daniel-garcia I am not able to add reviewers to this PR. Please review when you get a chance.
In the meantime, I will review the UT failure

akleinib · 2024-09-25T14:55:48Z

cmd/migrate/config.go

@@ -35,4 +35,8 @@ var (

 	flagConfigDirectory = pflag.String("config.source", defaultConfigDirectory, "directory of the configuration file")
 	flagConfigFile      = pflag.String("config.file", "", "configuration file name without extension")
+
+	// goto command flags
+	flagDirty   = pflag.Bool("dirty", false, "migration is dirty")


I would also suggest to update the description of the flag.

akleinib · 2024-09-25T14:58:11Z

cmd/migrate/config.go

+
+	// goto command flags
+	flagDirty   = pflag.Bool("dirty", false, "migration is dirty")
+	flagPVCPath = pflag.String("intermediate-path", "", "path to the mounted volume which is used to copy the migration files")


Cannot it be any directory and not a PVC if used natively? If so, I would suggest to remove the PVC term.

I still see the mounted volume term here.

internal/cli/main.go

migrate.go

migrate_test.go

venkat-iblox · 2024-10-02T23:44:22Z

@akleinib @daniel-garcia I have addressed comments except #36 (comment). To address this comment, we will need a broader work item to introduce new methods in the driver interface and its implementation across various supported drivers

akleinib · 2024-10-03T17:54:08Z

@akleinib @daniel-garcia I have addressed comments except #36 (comment). To address this comment, we will need a broader work item to introduce new methods in the driver interface and its implementation across various supported drivers

Maybe we can put a limitation that the new options only work if the file driver is used for source.

venkat-iblox · 2024-10-03T18:00:45Z

@akleinib @daniel-garcia I have addressed comments except #36 (comment). To address this comment, we will need a broader work item to introduce new methods in the driver interface and its implementation across various supported drivers

Maybe we can put a limitation that the new options only work if the file driver is used for source.

Sure

akleinib · 2024-10-03T17:26:00Z

migrate.go

+	ds *dirtyStateHandler
+}
+
+type dirtyStateHandler struct {


Shan't we rename the structure to use something like Config rather than Handler?

akleinib · 2024-10-03T17:46:37Z

migrate.go

@@ -744,6 +839,15 @@ func (m *Migrate) runMigrations(ret <-chan interface{}) error {
 			if migr.Body != nil {
 				m.logVerbosePrintf("Read and execute %v\n", migr.LogString())
 				if err := m.databaseDrv.Run(migr.BufferedBody); err != nil {
+					if m.dirtyStateConf != nil && m.dirtyStateConf.enable {


Cannot we use IsDirtyHandlingEnabled() here?

akleinib · 2024-10-03T17:47:29Z

migrate.go

+func (m *Migrate) handleDirtyState() error {
+	// Perform the following actions when the database state is dirty
+	/*
+	   1. Update the source driver to read the migrations from the mounted volume


As before regarding mounted volume term used multiple times in this comment.

use cache-dir

akleinib · 2024-10-03T17:56:08Z

migrate_test.go

@@ -1414,3 +1417,282 @@ func equalDbSeq(t *testing.T, i int, expected migrationSequence, got *dStub.Stub
 		t.Fatalf("\nexpected sequence %v,\ngot               %v, in %v", bs, got.MigrationSequence, i)
 	}
 }
+
+// Setting up temp directory to be used as the volume mount
+func setupTempDir(t *testing.T) (string, func()) {


Did you try https://pkg.go.dev/testing#T.TempDir?

akleinib · 2024-10-03T18:05:01Z

migrate_test.go

+			}
+
+			for _, file := range test.copiedFiles {
+				if _, err := os.Stat(filepath.Join(destDir, file)); os.IsNotExist(err) {


Ideally you want to check that "current.sql" hasn't been copied.

I don't know if using https://pkg.go.dev/os#ReadDir you can write a helper function returning all the files in a directory you can compare with an expected list of files. It would avoid in the various test-cases the two loops (one checking for existence, one checking for absence).

I don't think it would matter. current.sql is a project specific concept, migrate file driver doesn't know anything about things that don't match its pattern.

daniel-garcia

LGTM. The public interface (cli switches and behaviors) is in decent shape. We can address other concerns in a followup PR

…own feature (#37) * address comments * address comments + new UTs * address comments * remove build files

venkat-iblox added 3 commits August 28, 2024 18:15

First pass at downmigrate

ec51861

First pass at downmigrate

98be678

downmigrate changes + UTs + cleanups

8ab5a72

daniel-garcia requested changes Sep 23, 2024

View reviewed changes

daniel-garcia assigned kd7lxl and drewwells Sep 24, 2024

akleinib reviewed Sep 25, 2024

View reviewed changes

address comments

6bebd33

akleinib reviewed Oct 3, 2024

View reviewed changes

daniel-garcia approved these changes Oct 3, 2024

View reviewed changes

venkat-iblox merged commit 13318b2 into infobloxopen:ib Oct 4, 2024
2 of 5 checks passed

venkat-iblox deleted the goto-command-changes branch October 4, 2024 00:21

venkat-iblox mentioned this pull request Oct 4, 2024

Minor code updates from missed open comments from #36 to the migratedown feature #37

Merged

venkat-iblox added a commit that referenced this pull request Oct 9, 2024

Minor code updates from missed open comments from #36 to the migrated…

5d974f1

…own feature (#37) * address comments * address comments + new UTs * address comments * remove build files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Goto command support to handle dirty Database state #36

Goto command support to handle dirty Database state #36

venkat-iblox commented Sep 23, 2024 •

edited

Loading

daniel-garcia Sep 23, 2024

venkat-iblox Sep 23, 2024

daniel-garcia Sep 24, 2024

akleinib Sep 25, 2024

venkat-iblox commented Sep 23, 2024 •

edited

Loading

akleinib Sep 25, 2024

akleinib Sep 25, 2024

venkat-iblox Sep 25, 2024

akleinib Oct 3, 2024

venkat-iblox commented Oct 2, 2024

akleinib commented Oct 3, 2024

venkat-iblox commented Oct 3, 2024

akleinib Oct 3, 2024

akleinib Oct 3, 2024

akleinib Oct 3, 2024

daniel-garcia Oct 3, 2024

akleinib Oct 3, 2024

akleinib Oct 3, 2024

daniel-garcia Oct 3, 2024

daniel-garcia left a comment

Goto command support to handle dirty Database state #36

Goto command support to handle dirty Database state #36

Conversation

venkat-iblox commented Sep 23, 2024 • edited Loading

Key Changes:

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

venkat-iblox commented Sep 23, 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

venkat-iblox commented Oct 2, 2024

akleinib commented Oct 3, 2024

venkat-iblox commented Oct 3, 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

Choose a reason for hiding this comment

daniel-garcia left a comment

Choose a reason for hiding this comment

venkat-iblox commented Sep 23, 2024 •

edited

Loading

venkat-iblox commented Sep 23, 2024 •

edited

Loading