-
Notifications
You must be signed in to change notification settings - Fork 39
Require database exists if migration is enabled #1074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
| } | ||
|
|
||
| if options.Replication.Enable || options.Sync.Enable || options.Indexer.Enable { | ||
| if options.Replication.Enable || options.Sync.Enable || options.Indexer.Enable || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means that the migrator needs to have the Signer.PrivateKey of the node. Otherwise we will compute a different namespace. Do we want to pass this info around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed, as the migrator has to sign envelopes PayerEnvelope and OriginatorEnvelope both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we expect to have a separate payer key for this? Or are we reusing the node key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate payer key, already set up in testnet. That way we can properly identify the migrator "payer" if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the migrator has to share the DB With the sync and API nodes, right?
if namespace == "" {
namespace = utils.BuildNamespace(
options.Signer.PrivateKey,
options.Contracts.SettlementChain.NodeRegistryAddress,
)
}
So, to correctly determine the DB to share, it must have the same options.Signer.PrivateKey as the other services.
I guess it's okay if we have to provide both the node key and the payer key to the migrator. It just needs to be configured in the infra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the signer.private key should be checked in validateMigrationOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's done. Maybe I explained myself wrong.
The migrator has two keys:
- Separate payer key, to properly identify the migrator as an unique payer. Signs PayerEnvelopes.
- Node private key, which currently is the same as node 100. It's the same key where the payer runs, and the infra reflects this. Signs OriginatorEnvelopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. Can you add these keys to validateMigrationOptions? Approving otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Require the database to exist when the migration server is enabled by triggering namespace derivation and namespaced DB initialization in
main.mainUpdate
main.mainto broaden the conditional that gates namespace derivation and database initialization to also includeoptions.MigrationServer.Enablealongside the existing Replication, Sync, and Indexer checks in main.go.📍Where to Start
Start at the conditional in
main.mainin main.go that gates namespace derivation and namespaced database initialization.Changes since #1074 opened
Macroscope summarized 2b93b22.