fix migration, tests for more repo types #4398
Conversation
frrist
commented
Sep 10, 2024
frrist
commented
- closes Repo migration is failing #4389
pkg/repo/migrations/v3_4.go
Outdated
| // we still need to write a node name to the system_metadata.yaml file. | ||
| nodeID, err := config.GetNodeID(context.Background(), types.Default.NameProvider) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := r.WriteNodeName(nodeID); err != nil { | ||
| return fmt.Errorf("migrating node name: %w", err) | ||
| } |
There was a problem hiding this comment.
In the event the repo being migrated doesn't have a config file - resulting from a command like bacalhau version, then we need to create a node name here while migrating here as there isn't one in the config file to read from.
| if _, err := os.Stat(oldExecutionStorePath); err == nil { | ||
| newExecutionStorePath := filepath.Join(oldComputeDir, "state_boltdb.db") | ||
| if err := os.Rename(oldExecutionStorePath, newExecutionStorePath); err != nil { | ||
| return err |
There was a problem hiding this comment.
ensures a store is present before attempting to migrate it.
| if _, err := os.Stat(oldJobStorePath); err == nil { | ||
| newJobStorePath := filepath.Join(oldOrchestratorDir, "state_boltdb.db") | ||
| if err := os.Rename(oldJobStorePath, newJobStorePath); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
ensures a store is present before attempting to migrate it.
| @@ -40,7 +40,8 @@ func TestV3MigrationsTestSuite(t *testing.T) { | |||
| suite.Run(t, new(V3MigrationsTestSuite)) | |||
| } | |||
|
|
|||
| func (suite *V3MigrationsTestSuite) TestV3MigrationWithDefaultRepo() { | |||
There was a problem hiding this comment.
I've increased test coverage for a variety of possible repos:
- repo generated from
bacalhau serve --node-type=requester,compute - repo generated from
bacalhau serve --node-type=requester - repo generated from
bacalhau serve --node-type=compute - repo generated from
bacalhau version
pkg/config/config.go
Outdated
| @@ -249,3 +252,25 @@ func KeyAsEnvVar(key string) string { | |||
| fmt.Sprintf("%s_%s", environmentVariablePrefix, environmentVariableReplace.Replace(key)), | |||
| ) | |||
| } | |||
|
|
|||
| func GetNodeID(ctx context.Context, nodeNameProviderType string) (string, error) { | |||
There was a problem hiding this comment.
nit: better to name it GenerateNodeID to avoid confusing it with getting the actual node name
pkg/repo/migrations/v3_4.go
Outdated
| // we still need to write a node name to the system_metadata.yaml file. | ||
| nodeID, err := config.GetNodeID(context.Background(), types.Default.NameProvider) |
There was a problem hiding this comment.
We shouldn't assign a new node name while migrating. If the repo doesn't have a node name, then we shouldn't assign one during migration. It should be treated the same as starting a node with empty or partial repo, right?
There was a problem hiding this comment.
I have moved this logic to the serve command and will create a node name if one isn't present when the node starts.
will create a bacalhau node name and persist it if one is not present
| // if it takes longer than 5 seconds to get the node name from a provider, fail | ||
| nameCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| nodeName, err := getNodeID(nameCtx, cfg.NameProvider) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
since a node name is only needed by the serve command I have removed this code, it's now only done in cmd/cli/serve.go
