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

Improve dockerfiles #224

Merged
merged 16 commits into from
Apr 9, 2024
Merged

Improve dockerfiles #224

merged 16 commits into from
Apr 9, 2024

Conversation

akoken
Copy link
Contributor

@akoken akoken commented Apr 2, 2024

This PR contains the following changes.

  • Updated Dockerfiles to respect Docker layer caching. The current Dockerfiles copy all the files to the build context, which means every time you make a change in the codebase, the dotnet restore command runs even though you haven't changed anything in the csproj file.
  • Set linux distro as Ubuntu Jammy in Dockerfile.ubuntu. The default Linux distro for . NET container images is Debian.
  • Added Alpine based Dockerfile.
  • Added non-root user option to Dockerfiles.

@badrishc
Copy link
Contributor

badrishc commented Apr 3, 2024

@Niek - could you review this PR as well?

Dockerfile Show resolved Hide resolved
Copy link
Contributor

@Niek Niek left a comment

Choose a reason for hiding this comment

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

What about changes to the https://github.com/microsoft/garnet/blob/main/.github/workflows/docker.yml file? Caching can be enabled there, as well as adding the other docker files as separate tags.

@akoken
Copy link
Contributor Author

akoken commented Apr 3, 2024

What about changes to the https://github.com/microsoft/garnet/blob/main/.github/workflows/docker.yml file? Caching can be enabled there, as well as adding the other docker files as separate tags.

Setting up CI pipeline for docker layer caching is not part of this PR. I'm not very familiar with Github Actions, but my next PR could be about it. This PR only contains Dockerfile best practices.

@Niek
Copy link
Contributor

Niek commented Apr 3, 2024

Fair enough re GitHub workflows.

One more nitpick: the dockerignore doesn't add much value it seems, since there is no wildcard copy any longer in the dockerfiles.

@akoken
Copy link
Contributor Author

akoken commented Apr 3, 2024

Fair enough re GitHub workflows.

One more nitpick: the dockerignore doesn't add much value it seems, since there is no wildcard copy any longer in the dockerfiles.

In addition to excluding Dockerfiles, we've also omitted future possible docker-compose and README files from the dockerignore. While the few KB saved might not seem significant for GitHub Actions, the value is considerably higher in the context of the development environment. For instance, when I build the solution and then create a Docker image in my local environment, excluding the bin and obj folders from the build context, as highlighted in #20, proves to be quite beneficial.

As I mentioned before, it's a best practice and having a .dockerignore file is better than not having one at all IMHO.

@badrishc
Copy link
Contributor

badrishc commented Apr 3, 2024

Good comments and discussions, thanks to both of you!

@irinasp
Copy link
Contributor

irinasp commented Apr 4, 2024

@akoken, thank you for contributing to Garnet. I built and run all Linux dokerfiles successfully but Dockerfile.nanoserver is failed to build. I've added the comment for this file. Could you please take a look.

@akoken
Copy link
Contributor Author

akoken commented Apr 4, 2024

Hi @irinasp

It should be fixed now.

Dockerfile.nanoserver Outdated Show resolved Hide resolved
@irinasp
Copy link
Contributor

irinasp commented Apr 4, 2024

Hi @akoken, I built and run the fixed version of Dockerfile.nanoserver but it is still failing to build. I've added the comment. Could you please take a look.

@akoken
Copy link
Contributor Author

akoken commented Apr 5, 2024

Hi @irinasp

I'm sorry for inconveniencing you. Since my development environment isn't on Windows, I have been unable to replicate the issue. Would you kindly consider testing the latest change I made? If the issue persists, I believe I may need to create a virtual machine to conduct further testing.

@irinasp
Copy link
Contributor

irinasp commented Apr 5, 2024

@akoken, there are a lot of warnings while building the image from the dockerfile with your new fix. And there is the error while running the container:
docker: Error response from daemon: container e3e0ce56eb2163da9a1507ed388a21140066806acc218e8831e2944a6110b3ca encountered an error during hcs::System::CreateProcess: ./GarnetServer.exe -i 128m --port 6379: failure in a Windows system call: The system cannot find the file specified. (0x2)
[Event Detail: Provider: 00000000-0000-0000-0000-000000000000]
[Event Detail: Provider: 00000000-0000-0000-0000-000000000000]
[Event Detail: onecore\vm\compute\management\orchestration\vmhostedcontainer\processmanagement.cpp(173)\vmcomputeagent.exe!00007FF79388FDFD: (caller: 00007FF793834BB7) Exception(2) tid(3cc) 80070002 The system cannot find the file specified.
CallContext:[\Bridge_ProcessMessage\VmHostedContainer_ExecuteProcess]
Provider: 00000000-0000-0000-0000-000000000000].

Would you consider removing the update for Dockerfile.nanoserver from this PR and do separate PR for windows dockerfile after tunning the changes?

@akoken
Copy link
Contributor Author

akoken commented Apr 5, 2024

Would you consider removing the update for Dockerfile.nanoserver from this PR and do separate PR for windows dockerfile after tunning the changes?

I was thinking the same way @irinasp. I reverted the change for nanoserver. Thanks for your help!

Copy link
Contributor

@irinasp irinasp left a comment

Choose a reason for hiding this comment

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

The changes for all Linux dockerfiles are tested. Dockerfile.nanoserver is reverted to original version.

@irinasp irinasp merged commit a8570ac into microsoft:main Apr 9, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants