Repository navigation
[Migration Engine Part 3] Implement the RavenDB to SQL migration machinery - #5911
warwickschroeder wants to merge 8 commits into
Conversation
Adds the RavenDB source and the EF Core target for the KnownEndpoints and EndpointSettings categories, the startup checks that refuse an unsupported or unready migration, and the stall watchdog that stops a copy committing nothing for 30 minutes. Documents the migration contracts.
Adds unit tests for the startup checks, the stall watchdog and the refusals, target and reader tests for both persisters, a SQL Server collation test for the endpoint settings key, and acceptance tests for a copy that is killed, restarted and opened on. Approves the two new migration settings.
…mpatibility across migration targets
…ings and behavior
3fc7d09 to
58d4114
Compare
| - **Processing attempt history collapses to the newest attempt.** The SQL model has no attempts table. This affects every failed message that failed more than once, whether it is unresolved, archived or resolved. A message that failed five times arrives showing one attempt, and the other four are gone. | ||
| - **Subscriptions that differ only in message-type version merge onto one row**, because the target key carries the type name without the version. | ||
| - **Endpoint settings for two endpoint names that differ only in case merge onto one row on SQL Server**, because SQL Server's default collation compares names without case, so one of the two settings is kept. PostgreSQL keeps both, and so does a SQL Server database created with a case-sensitive collation. The dry run counts this one too, by asking SQL Server how the name column compares, though for unusual characters its count can differ from what the copy does. | ||
| - **Endpoint settings for two endpoint names that differ only in case merge onto one row on SQL Server**, because the collation of the name column decides the comparison and the default collation compares names without case, so one of the two settings is kept. It is the column's own collation that decides, not the database default, so a case-sensitive database whose name column was given a case-insensitive collation still merges. PostgreSQL keeps both. The dry run counts this one too, by asking SQL Server how the name column compares, though for unusual characters its count can differ from what the copy does. |
There was a problem hiding this comment.
This is a functional change that could impact licensing, is it actually ok?
There was a problem hiding this comment.
Specifically on licensing - licensing never goes through the endpoint settings table. So merging of endpoint settings here doesnt affect that. Licensing is already case-blind on both stores. EF lowercases the licensing key (LicensingDataStore.cs:331, Normalize uses ToLowerInvariant), and RavenDB keys licensing documents on {Name}/{ThroughputSource} (EndpointExtensions.cs:9), and RavenDB ids ignore case. So the migration changes no licensing counts.
Known endpoints dont merge either. They are keyed on a GUID. The only thing that merges is the settings for the endpoints, which is only 1 TrackInstances flag.
SQL Server persistence already does this. EndpointSettingsStore.UpdateEndpointSettings upserts on that case-insensitive key, so a live SQL Server instance can't hold Sales and sales apart either. RavenDB and PostgreSQL keep both.
The question is, whats the impact of this. Its either fine, or its a product bug on SQL Server that the migration just inherited. @johnsimons - thoughts?
There was a problem hiding this comment.
This feels like a defect we need to resolve..
- The settings sync runs 20 seconds after start and then every 6 hours. It reads the stored names ({"Sales"}), compares them case-sensitively with the known endpoints ({"Sales", "sales"}), and decides sales has no setting.
- It then upserts sales with the default value. The key ignores case, so that upsert overwrites the shared Sales row.
- Result: both endpoints are reset to the instance-wide default at startup and every 6 hours after. Any choice an operator makes for either one in ServicePulse is lost by the next sync.
- Old-instance cleanup only ever matches the stored spelling, so dead instances of sales are never cleaned up when tracking is off.
| - **A small category is not protected by the floor.** A separate rule halts any category that lost more than half its rows, whatever the floor says, because a category smaller than the floor would otherwise never reach it however much of it was lost. | ||
| - **A category also halts if it reaches the end of the source short.** If copied, skipped and already-present rows together come to less than the source count taken at the start, the category halts even though no threshold was crossed. **Planned:** before halting, the copier counts the source again, and halts only if the missing rows still exist in RavenDB, because rows RavenDB expired during the copy are an absence rather than a loss. | ||
| - Rows left behind because SQL would remove them anyway are counted and reported, but never halt a category. Today this exemption covers exactly one reason, settings for an unknown endpoint, and it withdraws itself: if the known endpoints copy skipped anything, an unknown endpoint can be this migration's own doing, so those skips start counting toward a halt again. | ||
| - The percentage is measured against what the run has processed so far rather than against the category's total, so a run that starts badly looks worse than it is. The floor is what keeps that harmless in a large category, since fewer than 101 skipped rows never consults the percentage at all. More than that, bunched at the start, does halt a category whose overall rate would have been fine, and the cost is one restart: the skipped rows commit with the cursor, so the next run resumes past them with its counters back at zero. |
There was a problem hiding this comment.
Is this behaviour what we want?
If this is being run in a containerised environment they very often have auto-restart enabled.
There was a problem hiding this comment.
I don't think so.
We want users to be able to retry failed category migrations. A required category with anything but "successful" should stop ServiceControl from starting. The user can fix what needs to be fixed and then try again. If there are acceptable losses, the user can mark it as "abandoned". I've updated the overview doc with this design.
|
|
||
| foreach (var row in batch.Rows) | ||
| { | ||
| var endpoint = (KnownEndpoint)row.Document; |
There was a problem hiding this comment.
If these made use of a generic typed base class you could have a test that enforced the reader/writer pair are using the same payload types to make this cast safer.
|
|
||
| public Task<long> Count(MigrationCategory category, CancellationToken cancellationToken = default) => | ||
| throw new NotSupportedException($"The RavenDB migration source cannot count category {category.Id} yet"); | ||
| // Counted by streaming the same documents Read walks, not from RavenDB's collection statistics: a total that |
There was a problem hiding this comment.
Is this being overly pessimistic?
| $"The RavenDB database '{databaseName}' carries no ServiceControl data version stamp. Start this instance once on RavenDB with version {RavenDataVersion.Current} before setting {MigrationSettings.EnabledKey}, so the source is brought up to date and stamped."); | ||
| } | ||
|
|
||
| // RavenDB deserializes with Newtonsoft, which ignores the required modifier, so a document saved without |
There was a problem hiding this comment.
This might be a bit of a future footgun since newer Newtonsoft versions do honor the required attributes. Is there a test that provides for that regression?
| } | ||
|
|
||
| [Test] | ||
| public async Task A_category_smaller_than_the_floor_that_loses_every_row_halts_rather_than_completing() |
There was a problem hiding this comment.
Should the floor be a percentage of the total instead when the total is low? Maybe 10%?
| /// Refuses any target but SQL Server or PostgreSQL, since the source is always RavenDB. The seams underneath are | ||
| /// general enough to copy between any two persisters, and this is the check that says which pair is actually supported. | ||
| /// </summary> | ||
| class MigrationPairIsSupportedCheck(Settings settings) : IMigrationStartupCheck |
There was a problem hiding this comment.
Could this be driven of metadata in the manifests instead of being static to our current use case?
3 participants
What this adds
The machinery that copies an instance's data from RavenDB into SQL Server or PostgreSQL during startup, before the host opens. Only known endpoints and endpoint settings can be copied, so a real instance with
Migration/Enabledset still refuses to start; message bodies are not read, andMigration/AllowIncompleteExitdoes nothing yet.Tests
ServiceControl.UnitTests/Migration: the engine, each startup check, the stall watchdog and the refusal messages, against fakes and a fake clock.ServiceControl.Persistence.Tests/EFCore/Migration: the target and its two writers on SQL Server and PostgreSQL, and a check that fails when an entity has no migration decision.ServiceControl.Persistence.Tests.RavenDB/DataMigration: the readers, the source and its data version against an embedded server.ServiceControl.Persistence.Tests.SqlServer: endpoint settings keys that differ only in case, and settings wiring.ServiceControl.Migration.AcceptanceTests(new): a killed copy, a restart, each refusal and the host opening on the target, on both providers.ServiceControl.Migration.Tests: every category with a reader has a writer, and the reverse.