Skip to content
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

Add Azure Event Hubs persistence functional test #7477

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Feb 8, 2025

Description

This is the first functional test verifying that persistence is effective for a container. I might have missed more efficient ways to do this. Currently only checking that the same container is used when a different app is started. Once approved we can add the same thing to other containers.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

Comment on lines 607 to 611
var resourceEvent = await rns.WaitForResourceAsync("resource", e => e.Snapshot.State?.Text == KnownResourceStates.Running, cts.Token);
var ehContainerId1 = resourceEvent.Snapshot.Properties.FirstOrDefault(x => x.Name == "container.id")?.Value?.ToString();

resourceEvent = await rns.WaitForResourceAsync("resource-storage", e => e.Snapshot.State?.Text == KnownResourceStates.Running, cts.Token);
var storageContainerId1 = resourceEvent.Snapshot.Properties.FirstOrDefault(x => x.Name == "container.id")?.Value?.ToString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) it may be more readable if you refactor this code out into a helper method "GetContainerId(string resourceName)`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines +613 to +615
await app.StopAsync(cts.Token).WaitAsync(TimeSpan.FromMinutes(1));

using var builder2 = TestDistributedApplicationBuilder.Create(testOutputHelper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test could use this new functionality:

#4878 (comment)

That way we knew the app was stopped.

cc @davidfowl @karolz-ms

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's on by default. But I decided to disable it on purpose, it's not required for the test (we check we don't have a different container, the state of the previous one doesn't matter) and not waiting makes the test faster (19s -> 12s)


// NOTE: Waiting for Running state doesn't guaranty that the container-id property is set (for this resource at least)

resourceEvent = await rns.WaitForResourceAsync("resource-storage", e => GetContainerId(e) is not null, cts.Token);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolz-ms

I had some problems locally with this. I am trying to read the "container-id" property of the snapshot. However if I wait for the Running state and then read the id right after it returns null most of the time. For the previous resource (a parent) it works all the time. So now I use the actual value as the predicate. Also these containers were already running on my machine (persistent from a previous run) so the container always has an id and is running.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little surprising that a container in Running state does not have a container ID set since on DCP side this is done as one change that should be atomic. But what you are doing here certainly makes sense.

The other thing to keep in mind is that even for running containers, the sequence of events represents the lifetime of the Container object in DCP. So the first event will always be a "Created" event with no container ID, and then DCP will notice that the existing Docker container can be reused, so it will fill the ContainerID property and change its Container object state to Running. And that change will be delivered as a separate event.

var resourceEvent = await rns.WaitForResourceAsync("resource", e => e.Snapshot.HealthStatus == HealthStatus.Healthy, cts.Token);
var ehContainerId1 = GetContainerId(resourceEvent);

// NOTE: Waiting for Running state doesn't guaranty that the container-id property is set (for this resource at least)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// NOTE: Waiting for Running state doesn't guaranty that the container-id property is set (for this resource at least)
// NOTE: Waiting for Running state doesn't guarantee that the container-id property is set (for this resource at least)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants