Enable replicaSet support for MongoDb#5712
Enable replicaSet support for MongoDb#5712twsouthwick wants to merge 7 commits intomicrosoft:mainfrom
Conversation
c5a1982 to
ff89d50
Compare
ff89d50 to
350ce9f
Compare
| { | ||
| if (builder.Resource.TryGetLastAnnotation<MongoDbReplicaSetAnnotation>(out _)) | ||
| { | ||
| throw new InvalidOperationException("A replica set has already been added to the MongoDB server resource."); |
There was a problem hiding this comment.
Why throw instead of noop? Because of the name?
There was a problem hiding this comment.
yeah. I can check if there's already one with the same name and return in those cases if you want
| var dir = Path.Combine(Path.GetTempPath(), "aspire.mongo", Path.GetRandomFileName()); | ||
| Directory.CreateDirectory(dir); | ||
|
|
||
| var rsInitContents = $$"""rs.initiate({ _id:'{{replicaSet}}', members:[{_id:0,host:'localhost:{{port}}'}]})"""; |
There was a problem hiding this comment.
@davidfowl I'd prefer to be able to just docker exec into the container after it started, but couldn't figure out how to do that so I'm running a container and configuring things to ensure it completes before anything else needs to use the db. If there is a better way to do this with aspire things, let me know
There was a problem hiding this comment.
We're adding support for this in Aspire 9 very soon. Might be better to wait until that support is available in the app model.
There was a problem hiding this comment.
cool - is there a tracking issue for that?
There was a problem hiding this comment.
@davidfowl any progress on being able to exec into the container?
| // See the conversation about setting up replica sets in Docker here: https://github.com/docker-library/mongo/issues/246 | ||
| static string GetReplicaSetInitDockerfileDir(string replicaSet, string host, int port) | ||
| { | ||
| var dir = Directory.CreateTempSubdirectory("aspire.mongo").FullName; |
There was a problem hiding this comment.
A couple notes here:
- Who cleans up this directory?
- On Unix, this is going to create a directory that only the current user can access. If the container is running on non-root, the container won't be able to access this folder.
There was a problem hiding this comment.
is there a pattern that already exists to handle creating/cleaning up directories? Maybe another reason as @davidfowl mentioned above to maybe wait to exec into a container
There was a problem hiding this comment.
So after some thought, I think a pattern we can use here is:
- Put the docker file in the nuget package
- Mark it as content so it gets copied to the output (we'lll want to name it so it doesn't conflict)
- Use build arguments to parameterize the docker file and pass that state in
There was a problem hiding this comment.
With #7136, we have now introduced a new "IAspireStore" API:
This file could be written there and my above concerns would be resolved.
|
/azp run |
|
Commenter does not have sufficient privileges for PR 5712 in repo dotnet/aspire |
| .WithHttpEndpoint(targetPort: 8081, name: "http") | ||
| .ExcludeFromManifest(); | ||
| .ExcludeFromManifest() | ||
| .WaitFor(builder); |
There was a problem hiding this comment.
I'm not sure about using WaitFor invisibly like this. I can see the benefit but if one of the mongo db instances files to start then you won't be able to inspect any of the other databases.
There was a problem hiding this comment.
I added that because once you have a replica set, the existing connection string may cause express to fail if it tries to connect before the replica set is initialized (a race condition) and it doesn't recover from it.
|
+1 for this. Missing replica sets doesn't just block transactions, it also blocks change streams. These are two very commonly utilised features. This looks close to done, but is not active... is this likely to get merged @twsouthwick? Thanks in advance. |
|
@creyke I'd like to get this in, but in order to set up the replica set, we must run some commands in the container. sounds like docker exec support will be available soon, so this is waiting on that. Once that is available, I'll update it so this can be merged. |
Still no luck? Thats highly blocker for integration testing /( |
|
@twsouthwick was there anything left to do on this? Currently using the workaround on this issue |
| private static void ConfigureMongoExpressContainer(EnvironmentCallbackContext context, MongoDBServerResource resource) | ||
| { | ||
| // Mongo Exporess assumes Mongo is being accessed over a default Aspire container network and hardcodes the resource address | ||
| var sb = new StringBuilder($"mongodb://{resource.Name}:{resource.PrimaryEndpoint.TargetPort}/?directConnection=true"); |
There was a problem hiding this comment.
ReferenceExpressionBuilder
|
@twsouthwick is this still in progress? |
|
@twsouthwick @eerhardt and I have an offline thread about this one. |
|
Just commenting again as not to close this issue @davidfowl / @eerhardt was there any update on this at all? |
| /// .WaitFor(messaging); | ||
| /// </code> | ||
| /// </example> | ||
| public static IResourceBuilder<T> WaitFor<T>(this IResourceBuilder<T> builder, IResourceBuilder<IResource> dependency, bool includeHealthChecks) where T : IResource |
There was a problem hiding this comment.
I wonder if bool includeHealthChecks should instead be a WaitType. We currently have WaitUntilHealthy and WaitForCompletion. But could add WaitUntilStarted.
|
I took a stab at updating it, but have found that since I did the first attempt that, password auth has been added. That adds a complication for enabling replica sets as that then requires a certificate to be used as well. The place I'm getting stuck on is that I can generate a cert, but getting it into the container is weird.
|
|
next action here seems to be a question for @eerhardt and I know he's pretty engaged right now. |
|
@twsouthwick @davidfowl and I met offline and had some ideas how to move this forward. We think if we had something like #6481, we could make this simpler. |
|
OK I'm going to close this as it sounds like we're blocked on that. Feel free to reopen, or create a new PR, when ready to continue @twsouthwick |
Description
This change adds support for replica sets in MongoDb. In order to support this a couple changes are required:
Fixes #5238
Checklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow