-
Notifications
You must be signed in to change notification settings - Fork 16
REP-5328 Compare shard keys as part of metadata verification. #63
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
Changes from all commits
7c547be
82effb4
cc5495c
2219b6f
5460ff0
1272f19
afa6f47
43db3b4
129cd21
700b138
1166dd2
1784516
22cf1e9
47e2ced
b4f45b3
be6555b
9c164b8
1b699f3
10837f4
4a29f5a
dd5b4ad
7dba357
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package util | ||
|
|
||
| import ( | ||
| "context" | ||
| "slices" | ||
|
|
||
| "github.com/10gen/migration-verifier/mslices" | ||
| "github.com/10gen/migration-verifier/option" | ||
| "github.com/pkg/errors" | ||
| "go.mongodb.org/mongo-driver/bson" | ||
| "go.mongodb.org/mongo-driver/mongo" | ||
| ) | ||
|
|
||
| // ServerThinksTheseMatch runs an aggregation on the server that determines | ||
| // whether the server thinks a & b are equal. This allows you, e.g., to | ||
| // ignore BSON type differences for equivalent numbers. | ||
| // | ||
| // tinker is an optional pipeline that operates on the documents in the | ||
| // pipeline (`a` and `b`, respectively) before they're compared. | ||
| // | ||
| // In migration-verifier the client is generally expected to be for | ||
| // the metadata cluster. | ||
| func ServerThinksTheseMatch( | ||
| ctx context.Context, | ||
| client *mongo.Client, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we want to restrict this function to only run on the meta cluster. Can we make it a method of the verifier and make sure that the query only runs on the meta cluster?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think it’s better as a static function so that there’s better separation of concerns. The more things we “hang” on Verifier, the less modular it all is.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a comment that to the function that explains it expects the client to only be the meta client for most of the cases?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| a, b any, | ||
| tinker option.Option[mongo.Pipeline], | ||
| ) (bool, error) { | ||
| pipeline := mongo.Pipeline{ | ||
| {{"$documents", []bson.D{ | ||
| { | ||
| {"a", bson.D{{"$literal", a}}}, | ||
| {"b", bson.D{{"$literal", b}}}, | ||
| }, | ||
| }}}, | ||
|
|
||
| // Now check to be sure that those specs match. | ||
| {{"$match", bson.D{ | ||
| {"$expr", bson.D{ | ||
| {"$eq", mslices.Of("$a", "$b")}, | ||
| }}, | ||
| }}}, | ||
| } | ||
|
|
||
| if extra, hasExtra := tinker.Get(); hasExtra { | ||
| pipeline = slices.Insert( | ||
| pipeline, | ||
| 1, | ||
| extra..., | ||
| ) | ||
| } | ||
|
|
||
| cursor, err := client.Database("admin").Aggregate(ctx, pipeline) | ||
|
|
||
| if err == nil { | ||
| defer cursor.Close(ctx) | ||
|
|
||
| if cursor.Next(ctx) { | ||
| return true, nil | ||
| } | ||
|
|
||
| err = cursor.Err() | ||
| } | ||
|
|
||
| return false, errors.Wrapf(err, "failed to ask server if a (%v) matches b (%v)", a, b) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package util | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/10gen/migration-verifier/option" | ||
| "github.com/pkg/errors" | ||
| "go.mongodb.org/mongo-driver/bson" | ||
| "go.mongodb.org/mongo-driver/mongo" | ||
| ) | ||
|
|
||
| const ( | ||
| configDBName = "config" | ||
| collsCollName = "collections" | ||
| ) | ||
|
|
||
| // GetShardKey returns the collection's shard key, or an empty option | ||
| // if the collection is unsharded. | ||
| func GetShardKey( | ||
| ctx context.Context, | ||
| coll *mongo.Collection, | ||
| ) (option.Option[bson.Raw], error) { | ||
| namespace := coll.Database().Name() + "." + coll.Name() | ||
|
|
||
| configCollectionsColl := coll.Database().Client(). | ||
| Database(configDBName). | ||
| Collection(collsCollName) | ||
|
|
||
| decoded := struct { | ||
| Key option.Option[bson.Raw] | ||
| }{} | ||
|
|
||
| err := configCollectionsColl. | ||
| FindOne(ctx, bson.D{{"_id", namespace}}). | ||
| Decode(&decoded) | ||
|
|
||
| if errors.Is(err, mongo.ErrNoDocuments) { | ||
| return option.None[bson.Raw](), nil | ||
| } else if err != nil { | ||
| return option.None[bson.Raw](), errors.Wrapf( | ||
| err, | ||
| "failed to find sharding info for %#q", | ||
| namespace, | ||
| ) | ||
| } | ||
|
|
||
| key, hasKey := decoded.Key.Get() | ||
|
|
||
| if !hasKey { | ||
| return option.None[bson.Raw](), nil | ||
| } | ||
|
|
||
| return option.Some(key), nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| package verifier | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/10gen/migration-verifier/internal/util" | ||
| "github.com/10gen/migration-verifier/option" | ||
| "github.com/pkg/errors" | ||
| "github.com/samber/lo" | ||
| "go.mongodb.org/mongo-driver/mongo" | ||
| ) | ||
|
|
||
| // This is the Field for a VerificationResult for shard key mismatches. | ||
| const ShardKeyField = "Shard Key" | ||
|
|
||
| func (verifier *Verifier) verifyShardingIfNeeded( | ||
| ctx context.Context, | ||
| srcColl, dstColl *mongo.Collection, | ||
| ) ([]VerificationResult, error) { | ||
|
|
||
| // We only need to compare if both clusters are sharded | ||
| srcSharded := verifier.srcClusterInfo.Topology == util.TopologySharded | ||
| dstSharded := verifier.dstClusterInfo.Topology == util.TopologySharded | ||
|
|
||
| if !srcSharded || !dstSharded { | ||
| return nil, nil | ||
| } | ||
|
|
||
| srcShardOpt, err := util.GetShardKey(ctx, srcColl) | ||
| if err != nil { | ||
| return nil, errors.Wrapf( | ||
| err, | ||
| "failed to fetch %#q's shard key on source", | ||
| FullName(srcColl), | ||
| ) | ||
| } | ||
|
|
||
| dstShardOpt, err := util.GetShardKey(ctx, dstColl) | ||
| if err != nil { | ||
| return nil, errors.Wrapf( | ||
| err, | ||
| "failed to fetch %#q's shard key on destination", | ||
| FullName(dstColl), | ||
| ) | ||
| } | ||
|
|
||
| srcKey, srcIsSharded := srcShardOpt.Get() | ||
| dstKey, dstIsSharded := dstShardOpt.Get() | ||
|
|
||
| if !srcIsSharded && !dstIsSharded { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if srcIsSharded != dstIsSharded { | ||
| return []VerificationResult{{ | ||
| Field: ShardKeyField, | ||
| Cluster: lo.Ternary(srcIsSharded, ClusterTarget, ClusterSource), | ||
| Details: Missing, | ||
| NameSpace: FullName(srcColl), | ||
| }}, nil | ||
| } | ||
|
|
||
| if bytes.Equal(srcKey, dstKey) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| areEqual, err := util.ServerThinksTheseMatch( | ||
| ctx, | ||
| verifier.metaClient, | ||
| srcKey, dstKey, | ||
| option.None[mongo.Pipeline](), | ||
| ) | ||
| if err != nil { | ||
| return nil, errors.Wrapf( | ||
| err, | ||
| "failed to ask server if shard keys (src %v; dst: %v) match", | ||
| srcKey, | ||
| dstKey, | ||
| ) | ||
| } | ||
|
|
||
| if !areEqual { | ||
| return []VerificationResult{{ | ||
| Field: ShardKeyField, | ||
| Details: fmt.Sprintf("%s: src=%v; dst=%v", Mismatch, srcKey, dstKey), | ||
| NameSpace: FullName(srcColl), | ||
| }}, nil | ||
| } | ||
|
|
||
| return nil, nil | ||
| } |
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 comment is not very accurate. It has a shortcut that compares the values in memory first.
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.
That’s an implementation, though. It seems trivially true that two binary-equivalent BSON values are ones that the server would also consider equivalent.