-
Notifications
You must be signed in to change notification settings - Fork 543
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
Enable replicaSet support for MongoDb #5712
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why throw instead of noop? Because of the name?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool - is there a tracking issue for that?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #7136, we have now introduced a new "IAspireStore" API:
aspire/src/Aspire.Hosting/ApplicationModel/IAspireStore.cs
Lines 17 to 22 in acdf501
public interface IAspireStore | |
{ | |
/// <summary> | |
/// Gets the base path of this store. | |
/// </summary> | |
string BasePath { get; } |
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 |
@@ -94,7 +95,8 @@ public static IResourceBuilder<T> WithMongoExpress<T>(this IResourceBuilder<T> b | |||
.WithImageRegistry(MongoDBContainerImageTags.MongoExpressRegistry) | |||
.WithEnvironment(context => ConfigureMongoExpressContainer(context, builder.Resource)) | |||
.WithHttpEndpoint(targetPort: 8081, name: "http") | |||
.ExcludeFromManifest(); | |||
.ExcludeFromManifest() | |||
.WaitFor(builder); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if bool includeHealthChecks
should instead be a WaitType
. We currently have WaitUntilHealthy
and WaitForCompletion
. But could add WaitUntilStarted
.
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