fix: implement NodeStore migration #4029
Conversation
|
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
First, awesome work in implementing both options to unblock the release as soon as possible. Great bias for action ;)
Overall, I am more inclined with this approach than migrating the whole repo or deleting the full node store:
- First, it aligns more with Rethinking Configurations where the repo just a simple path and is agnostic of the content and files that different internal components are creating inside it. It is not responsible for creating those files if they don't exist, and not responsible for migrating them
- The repo is limited to what it can do to migrate. An example is with this migration where the repo is only able to delete the whole store, while the individual component (kvstore) is able to do entry level migration
- There will be more migrations in the future and you just laid the groundwork to simplify those future migrations. I understand with this migration the option to delete the whole kvstore is acceptable as we didn't expose the option to manually approve nodes yet, but that won't be an option in the future and we will have to do entry level migrations
I have left minor comments and you merge after addressing them
pkg/routing/kvstore/kvstore.go
Outdated
| toKV, err := js.CreateKeyValue(ctx, jetstream.KeyValueConfig{ | ||
| Bucket: to, | ||
| }) |
There was a problem hiding this comment.
do we need to create the bucket first, or will this create it for us? What if the if the bucket already exist?
There was a problem hiding this comment.
// CreateKeyValue will create a KeyValue store with the given
// configuration.
//
// If a KeyValue store with the same name already exists and the
// configuration is different, ErrBucketExists will be returned.
My interpretation of the comment is:
- Create a store if it DNE
- Open a store if it exists with the same config
- Fail if the store exists, but with a different config.
And, fwiw, this is the same method we use in NewNodeStore to open/create the bucket. I think we're good here.
- reduce memory footrpint of migration. - buckets are named based on version. - extract core migration logic to method with generalize migrator function.
I am far from happy with this change (its a bit gross) as I had hoped to keep the number of migrations needed to a minimum. Further I had hoped to keep all migration related code tied to the repo version of the node. However, it appears we need one, and since the migration logic requires a jetstream client, which itself requires a NatsConfig it seemed simplest to renames the NodeState bucket, migrate the old one to it, and delete it.
bacalhau node listreturns errorfailed request: invalid node type: nodeTypeUndefined#4024