Dotnet-docker: [mcr.microsoft.com/dotnet/sdk:5.0] "dotnet watch run" doesn't reload the project when files are changed

[elafarge]

Describe the Bug

Our local docker-compose development stack consists in a set of containers running our ASP.NET Core APIs where we mount our code and run it with dotnet watch run.

We upgraded our services to .NET 5 yesterday, and therefore, our base images to mcr.microsoft.com/dotnet/sdk:5.0 and noticed a bug: files aren't reloaded when changed on the host.

A similar issue seems to have been reported here around a month ago.

Steps to Reproduce

Environment:
I'm running Docker on a Linux host but colleagues had the same issue on OSX. I have no information about docker on Windows or in the WSL (but I suspect it impacts at least docker running in the WSL).

Requirements:

• bash, docker and the .NET 5 CLI

1. Create a new simple "Hello world" webapp

mkdir /tmp/test && cd /tmp/test
dotnet new web

1. Run it with dotnet watch run and the code mounted in a container

docker run -it --rm --workdir /app -v ${PWD}/:/app mcr.microsoft.com/dotnet/sdk:5.0 dotnet watch run

You'll notice that when opening, altering and saving the ./Startup.cs file, the service isn't reloaded.

Other Information

The issue doesn't occur on the Ubuntu Focal image (5.0-focal). Neither does it on the alpine-based image but for some unknown reason, (re)build times are significantly slower on Alpine :thinking: .

If that can help, I had the same issue when building a custom .NET Core SDK image with the ./dotnet-install.sh scripts on debian:buster-slim, debian:slim and debian:bullseye, but not when basing the same custom image off ubuntu:focal. Oh and one last thing, it used to work well with the 3.1 image. Maybe a missing dependency in the base debian images ?

We just switched our local development environment to the official focal image, which completely solved our issue (so we basically don't care if and when this issue gets fixed :angel: ).
Since the 5.0 (the "main" image tag line) image is impacted, I thought this bug was worth reporting though.

Output of docker version

Client:
 Version:           19.03.13-ce
 API version:       1.40
 Go version:        go1.15.2
 Git commit:        4484c46d9d
 Built:             Sat Sep 26 12:04:46 2020
 OS/Arch:           linux/amd64
 Experimental:      false

Server:
 Engine:
  Version:          19.03.13-ce
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.15.2
  Git commit:       4484c46d9d
  Built:            Sat Sep 26 12:03:35 2020
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.4.1.m
  GitCommit:        c623d1b36f09f8ef6536a057bd658b3aa8632828.m
 runc:
  Version:          1.0.0-rc92
  GitCommit:        ff819c7e9184c13b7c2607fe6c30ae19403a7aff
 docker-init:
  Version:          0.18.0
  GitCommit:        fec3683

Output of docker info

Client:
 Debug Mode: false

Server:
 Containers: 58
  Running: 19
  Paused: 0
  Stopped: 39
 Images: 331
 Server Version: 19.03.13-ce
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: c623d1b36f09f8ef6536a057bd658b3aa8632828.m
 runc version: ff819c7e9184c13b7c2607fe6c30ae19403a7aff
 init version: fec3683
 Security Options:
  seccomp
   Profile: default
 Kernel Version: 5.9.6-arch1-1
 Operating System: Arch Linux
 OSType: linux
 Architecture: x86_64
 CPUs: 16
 Total Memory: 15.55GiB
 Name: garland
 ID: HIY6:PX3H:AZLZ:JIGM:EOGA:XI26:WGLU:4ZD4:WZEZ:RPAX:QPRF:KMAH
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Username: elafarge
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false

[MichaelSimons]

@elafarge, thanks for logging this issue. dotnet watch seems to work for a simple console app such as the .NET Docker Sample, but as you indicated it doesn't seem to work for ASP.NET projects w/Debian based images.

It looks like this is caused by Breaking Change: Removal of buildpack-deps in 5.0 SDK Docker images. Specifically in my quick test, it seems like this scenario has a dependency on the procps package which is no longer included in the debian-slim based 5.0 SDK image.

I was able to get dotnet watch to work with a ASP.NET project by using an image produced from the following Dockerfile.

FROM mcr.microsoft.com/dotnet/sdk:5.0

RUN apt-get update \
    && apt-get install -y --no-install-recommends \
        procps \
    && rm -rf /var/lib/apt/lists/*

[elafarge]

@MichaelSimons thanks a lot for the quick troubleshooting :) I had the feeling it was due to a missing package but I didn't run the investigation far enough to figure out which one.

Would you like me to contribute a patch to the https://github.com/dotnet/dotnet-docker/tree/master/eng/dockerfile-templates/runtime-deps/3.1 image ? I've had to get familiar to understand how the sdk images are built, it should be within my reach now :)
Also if I contribute such a patch, would you like me to add an end-to-end test to check that dotnet watch's behaviour meets expectations here ?

[MichaelSimons]

@elafarge, Yes we would appreciate your help with fixing this. Before we begin with a fix, we want to ensure that we have the correct set of native dependencies identified. Are there additional dependencies that are missing? @mthalman is going to follow up with the SDK folks on this and will respond here with his findings.

In addition to the code fix, there are a two other follow-ups @mthalman will investigate:

1. Does the SDK have their native dependencies documented? How are customer's supposed to know what dependencies are required beyond the .NET runtime's?
2. Is the UX acceptable when the procps dependency is missing? Ideally the dotnet watch code would provide some type of indication that a dependency it has is missing so that the user can easily self-diagnose why things aren't working.

Regarding the code changes, we do not want to add this native dependency to the runtime-deps image, rather it belongs in the sdk image. This dependency is not required by the runtime rather it is tooling specific. We don't want to bloat the runtime-deps image with SDK specific dependencies. Based on this, these are the templates that would need to be changed.

To answer your testing question, yes I think it would be ideal if we had a utest in place. The question is whether we want to validate the dependencies are present or do a scenario test to ensure dotnet watch works. The scenario test will get complicated quickly I'm afraid and be slow.

Thanks for your help here.

[mthalman]

I've confirmed that it is possible to repro this with a console app project. You just need to have a console app that runs long enough to have time to modify and save a file.

Here's my repro:

1. Use the sample from https://github.com/dotnet/dotnet-docker/tree/master/samples/dotnetapp
2. Add a Console.ReadLine(); line here: https://github.com/dotnet/dotnet-docker/blob/0c3cedc7cc5c6679edbba3a0fdf717caeefe02bb/samples/dotnetapp/Program.cs#L14
3. docker run --rm -it -v ${pwd}:/app/ -w /app mcr.microsoft.com/dotnet/sdk:5.0 dotnet watch run
4. After the bot is outputted, modify Program.cs with some edit.

Expected: Polling file watcher should output that it detected a change.
Actual: No change is detected

So there's nothing specific about ASP.NET here that causes this behavior.

[tobbbe]

Would it be possible to include procps in all sdk-images if that solves the problem? It is very confusing+frustrating for a new developer to setup dotnet with Docker when the watch command cannot run inside a container without adding some obscure(oh well..) RUN-script in the Dockerfile. :)

Im guessing almost everyone want to have watch running when developing. And people who are new to Docker+dotnet will probably start out with the mcr.microsoft.com/dotnet/sdk:5.0 image for developing.

[MichaelSimons]

Would it be possible to include procps in all sdk-images if that solves the problem

Yes, that is the plan.

[elafarge]

Here's a bugfix proposal for this issue @MichaelSimons @mthalman : https://github.com/dotnet/dotnet-docker/pull/2402 .

I'll open a separate PR for the new test scenario in the coming days.

[MichaelSimons]

@elafarge, FYI, we are still investigating this to ensure that the correct set of native dependencies have been identified.

[elafarge]

And here's a proposal for a test checking that the procps package is present on Debian based images.

@MichaelSimons No problem. I'll be happy to update the PR(s) if dependencies need to be updated.

[tmds]

it seems like this scenario has a dependency on the procps package

@MichaelSimons @elafarge in what way does dotnet-watch depend on procps?

[tmds]

Ah, I missed the comment that says you are still checking the correct native dependencies.

afaik dotnet-watch in containers falls back to polling based on DOTNET_USE_POLLING_FILE_WATCHER being set.

[MichaelSimons]

in what way does dotnet-watch depend on procps?

This is what I found with a quick investigation.

https://github.com/dotnet/aspnetcore/blob/master/src/Tools/dotnet-watch/src/Internal/ProcessRunner.cs#L164

_process.KillTree();

https://github.com/dotnet/aspnetcore/blob/master/src/Shared/Process/ProcessExtensions.cs#L20

public static void KillTree(this Process process, TimeSpan timeout)
{
    var pid = process.Id;
    if (_isWindows)
    {
        ...
    }
    else
    {
        var children = new HashSet<int>();
        GetAllChildIdsUnix(pid, children, timeout);
        foreach (var childId in children)
        {
            KillProcessUnix(childId, timeout);
        }
        KillProcessUnix(pid, timeout);
    }
}

private static void GetAllChildIdsUnix(int parentId, ISet<int> children, TimeSpan timeout)
{
    try
    {
        RunProcessAndWaitForExit(
            "pgrep",
            $"-P {parentId}",
            timeout,
            out var stdout);

@pranavkm can hopefully talk about this and why System.Diagnostics.Process.Kill isn't used here.

[tmds]

"pgrep",

Ah, procps provides pgrep.

[tmds]

@pranavkm can hopefully talk about this and why System.Diagnostics.Process.Kill isn't used here.

pgrep is used to find the children.

Since .NET 3.1, Process allows you to kill the entire tree: public void Kill (bool entireProcessTree);.

[tmds]

This implementation sends the SIGTERM signal, while Process.Kill sends SIGKILL.

[MichaelSimons]

Yes, this would eliminate the pgrep dependency all together which would be very nice.

[MichaelSimons]

FYI, in discussing this with @pranavkm, the current plan is to fix this in dotnet-watch. The native dependency will be removed so there is no need to add procps to the SDK images. You can follow the fix/discussion at https://github.com/dotnet/aspnetcore/issues/27950.

[MichaelSimons]

Because the pgrep dependency is going to be removed from dotnet-watch, we do not want to add the procps package to the sdk images as a temporary patch. The primary reason is that it would be a breaking change to remove it once dotnet-watch is fixed. Additionally there are a couple workarounds that are fairly easy to use.

[elafarge]

That would sound like a much more appropriate and robust fix indeed :)
Thanks a lot for forwarding the bug report to the SDK team.

[MichaelSimons]

@elafarge, kudos for opening this issue, providing the workaround of using the alpine and focal images, as well as your willingness to contribute the fix!

[MichaelSimons]

The fix for dotnet/aspnetcore#27950 was pushed to 6.0. As a result, we will need to add the procps package to the 5.0 images

[mthalman]

Fixed as part of https://github.com/dotnet/dotnet-docker/pull/2496

[tobbbe]

@mthalman Does this mean procps is added to 5.0 SDK too? mcr.microsoft.com/dotnet/sdk:5.0 for example.
since the commit says "Add procps package to 5.0 SDK #2487" (but @MichaelSimons said it was pushed to 6.0)

im confused :) Thanks!

[mthalman]

@tobbbe - procps was added to the 5.0 SDK, see here: https://github.com/dotnet/dotnet-docker/blob/fc0269dcc5f9b8fcbedf6ff8d83514eb9a75486e/src/sdk/5.0/buster-slim/amd64/Dockerfile#L19

For 6.0, the intention is to not require the procps package by changing the implementation of the watch tool to not make use of the pgrep utility. That's what @MichaelSimons meant by pushing the change to 6.0. Our original goal was that we would make that watch tool change in 5.0 but that didn't work out.

[tobbbe]

okey I see! thank you for the explanation