Skip to content

Log stream the build and pull.#4535

Merged
mitchdenny merged 5 commits intomainfrom
mitchdenny/logstream-build-and-pull
Jun 19, 2024
Merged

Log stream the build and pull.#4535
mitchdenny merged 5 commits intomainfrom
mitchdenny/logstream-build-and-pull

Conversation

@mitchdenny
Copy link
Copy Markdown
Member

@mitchdenny mitchdenny commented Jun 17, 2024 •
edited by dotnet-policy-service Bot
Loading

This PR addresses #4522

This is still a draft because we have a few issues:

  1. docker build/pull output is showing up as stderr.
  2. The output isn't actually streaming ... seems to arrive all at once.
  3. We probably want to tweak the way we process the streams to make sure it is impossible to interleave build/pull output with regular logs.
Microsoft Reviewers: Open in CodeFlow

@mitchdenny mitchdenny requested a review from karolz-ms June 17, 2024 06:59
@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 17, 2024
@mitchdenny mitchdenny requested review from danegsta and davidfowl June 17, 2024 06:59
@mitchdenny mitchdenny self-assigned this Jun 17, 2024
@mitchdenny mitchdenny added this to the 8.1 milestone Jun 17, 2024
@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 17, 2024

@danegsta it seems like the output from docker build is coming through stderr ... and it looks like it isn't streaming.

Based on what I'm observing it looks like you are observing the error code and piping into stderr the entire output?

karolz-ms
karolz-ms requested changes Jun 17, 2024
View reviewed changes
Comment thread src/Aspire.Hosting/Dcp/ResourceLogSource.cs Outdated
Comment thread src/Aspire.Hosting/Dcp/ResourceLogSource.cs Outdated
@dotnet-policy-service dotnet-policy-service Bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 17, 2024
@karolz-ms
Copy link
Copy Markdown
Contributor

karolz-ms commented Jun 17, 2024

@danegsta it seems like the output from docker build is coming through stderr ... and it looks like it isn't streaming.

I will let David investigate but it might be a limitation of Docker/Podman CLI in the sense that if they detect they do not write to actual console, with its control character capabilities, they buffer the output until the build process is done. Just a theory. Because we do stream whatever is coming from the build command, at least that is the intent.

stderr is not for "errors only". Stderr is for human-readable content. Stdout is for machine-readable content. That is the original Unix design. Sadly it has not been followed much. But sending diagnostic/informational output to stderr is the right thing to do.

@dotnet-policy-service dotnet-policy-service Bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 17, 2024
@mitchdenny mitchdenny marked this pull request as ready for review June 18, 2024 00:56
@mitchdenny mitchdenny requested a review from karolz-ms June 18, 2024 00:59
@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 18, 2024 •
edited
Loading

@karolz-ms could you reset your vote? (assuming you are ok with my changes)

eerhardt
eerhardt reviewed Jun 18, 2024
View reviewed changes
Comment thread playground/withdockerfile/WithDockerfile.AppHost/Program.cs
eerhardt
eerhardt reviewed Jun 18, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Are there any tests that can be written for this?

@danegsta
Copy link
Copy Markdown
Member

danegsta commented Jun 18, 2024

@mitchdenny I've got a small change to DCP that'll land soon to pass --progress plain to docker build. That should improve the verbosity of build output vs. the default setting when running without a TTY session.

@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 18, 2024

@mitchdenny I've got a small change to DCP that'll land soon to pass --progress plain to docker build. That should improve the verbosity of build output vs. the default setting when running without a TTY session.

Will that also help with streaming.

@danegsta
Copy link
Copy Markdown
Member

danegsta commented Jun 18, 2024

Will that also help with streaming.

It should, but I'll need to test with your branch to make sure.

@radical
Copy link
Copy Markdown
Member

radical commented Jun 19, 2024

Are there any tests that can be written for this?

Would testing with a simple custom dockerfile be useful here? 1. purge the image; 2. WithDockerFile with some custom messages(?); 3. start; 4. check the logs?
I'm suggesting a custom dockerfile so we could maybe have one that would result in a smaller image, and wouldn't affect any of the other real ones.

@mitchdenny mitchdenny merged commit 87e79ea into main Jun 19, 2024
@mitchdenny mitchdenny deleted the mitchdenny/logstream-build-and-pull branch June 19, 2024 01:44
radical
radical reviewed Jun 19, 2024
View reviewed changes
Comment thread src/Aspire.Hosting/Dcp/ResourceLogSource.cs

var timestamps = resource is Container; // Timestamps are available only for Containers as of Aspire P5.

if (resource is Container)
Copy link
Copy Markdown
Member

@radical radical Jun 19, 2024

Choose a reason for hiding this comment

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

nit:

Suggested change
if (resource is Container)
if (timestamps)

Copy link
Copy Markdown
Member Author

@mitchdenny mitchdenny Jun 19, 2024

Choose a reason for hiding this comment

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

Will look at this as a follow up. You would be surprised how much I agonized over that.

Comment thread src/Aspire.Hosting/Dcp/ResourceLogSource.cs
var startupStdoutStreamTask = Task.Run(() => StreamLogsAsync(startupStdoutStream, isError: false), cancellationToken);
streamTasks.Add(startupStdoutStreamTask);

var startupStderrStreamTask = Task.Run(() => StreamLogsAsync(startupStderrStream, isError: false), cancellationToken);
Copy link
Copy Markdown
Member

@radical radical Jun 19, 2024

Choose a reason for hiding this comment

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

Why is this using isError: false?

Copy link
Copy Markdown
Member Author

@mitchdenny mitchdenny Jun 19, 2024

Choose a reason for hiding this comment

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

Deliberate. docker build outputs vritually everything to stderr which @karolz-ms points out is the right thing for it to do from a unix philosophy point of view. But given it prepends the err text to ever line its quite noisy.

I think we might want to have a chat to the dashboard folks about a slightly different visual treatment here.

@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 19, 2024

Are there any tests that can be written for this?

Eric I accidentally ignored this comment. I'll look at what our coverage for this change looks like. Mostly this is prepending existing logs so existing test coverage should make sure its working.

But I'll have a think about what if anything extra we can do.

@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 19, 2024 •
edited
Loading

Are there any tests that can be written for this?

Would testing with a simple custom dockerfile be useful here? 1. purge the image; 2. WithDockerFile with some custom messages(?); 3. start; 4. check the logs? I'm suggesting a custom dockerfile so we could maybe have one that would result in a smaller image, and wouldn't affect any of the other real ones.

We do have live tests already which take an image from MCR (which is pretty small) inject some custom files and then fetch the files from the web-server built into the image just to confirm the build args and build secrets are working.

I'll look at what we can do to have a test that reads the log messages.

@mitchdenny
Copy link
Copy Markdown
Member Author

mitchdenny commented Jun 19, 2024

#4579

This was referenced Jun 19, 2024
Open
Closed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radical radical radical left review comments

@eerhardt eerhardt eerhardt left review comments

@karolz-ms karolz-ms karolz-ms approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl davidfowl is a code owner

@danegsta danegsta Awaiting requested review from danegsta danegsta is a code owner

Assignees

@mitchdenny mitchdenny

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mitchdenny @karolz-ms @danegsta @radical @eerhardt