Skip to content

Conversation

Raimo33
Copy link

@Raimo33 Raimo33 commented Jul 28, 2025

This PR refactors the dockerfile according to issue #8409

changes:

  • remove useless python-builder stage
  • verify GPG keys (Docker: verify GPG keys #8405)
  • only include the necessary binaries in final image
  • use of ARG instead of ENV for build-time vars
  • remove useless apt packages
  • use of ADD instead of curl
  • set global SHELL
  • reduce redundancy of multi-arch image selections
  • use of tee + heredocs instead of messy echo's
  • stripped compiled binaries for smaller image
  • remove manual tini in favor of docker --init flag
  • avoid compiling sqlite, zlib, postgres from source
  • replace poetry with uv
  • avoid downloading all history/branches for submodules
  • parallelize make and submodules cloning
  • better docker caching of layers
  • fix CPU compatibility bug (Dockerfile is not CPU agnostic #8456)
  • avoid running cargo with QEMU. true cross compilation.
  • avoid installing clippy and rust-doc components
  • any host to any target image building (not tested)
  • avoid installing manpages
  • update base image from bookworm to trixie
  • fix poetry trying to install the current project as standalone package (crashed the build before)
  • fix armhf target (crashed the build before)
Feature Before After Improvement
Build Time (amd64 to arm64) 90.6 min 10.2 min 8.8x
Image Size (amd64) 512 MB 247 MB 2.07x
Lines of Code 316 199 1.6x

build time was benchmarked on a 12th Gen Intel(R) Core(TM) i5-1235U and a 20 Mbps connection

@ShahanaFarooqui
Copy link
Collaborator

@Raimo33 As per @cdecker's suggestion here, we should modify the Dockerfile to run as a non-root user for improved security. Since we're already refactoring the Dockerfile, could we incorporate this change as well?

@Raimo33 Raimo33 force-pushed the docker-refactor branch 4 times, most recently from 18489dc to 7298f44 Compare August 6, 2025 19:14
@Raimo33 Raimo33 force-pushed the docker-refactor branch 2 times, most recently from 008f67c to ac0910c Compare August 10, 2025 13:19
@Raimo33
Copy link
Author

Raimo33 commented Aug 11, 2025

make install currently calls git describe to know the version of the program. This means that the .git/ folder needs to be copied into the Dockerfile for it to work. But copying the .git/ folder almost always invalidates the cache of the COPY . . command.

I'd like to explore ways to not include the .git/ folder (aka placing it in the .dockerignore). Maybe we can add an option to the Makefile that doesn't call git describe

@Raimo33 Raimo33 force-pushed the docker-refactor branch 5 times, most recently from e264a55 to c145582 Compare August 12, 2025 01:06
@cdecker
Copy link
Member

cdecker commented Aug 12, 2025

Nice changes, and with the switch to uv things may get even a bit simpler still. Undraft once ready, and we'll review again and merge asap 👍

@Raimo33 Raimo33 force-pushed the docker-refactor branch 6 times, most recently from 5ade525 to 7f7af77 Compare August 21, 2025 09:27
@Raimo33
Copy link
Author

Raimo33 commented Aug 21, 2025

@cdecker Adding a non-root USER directly inside the Dockerfile is not a reliable option when using mounted volumes. The reason is that file permissions on mounted volumes are determined by the UID and GID of the host system, not the container. Hardcoding a UID, for example USER 1000:1000, might work in some environments, but it is not guaranteed to match the host system’s user. On many systems, the first regular user is UID 1000, but this is not universal, and in environments like CI/CD runners, containers, or systems with different user configurations, the UID/GID may differ.

Hardcoding the UID inside the Dockerfile creates a fragile assumption and can easily lead to permission issues. Some repos such as mempool do just that, but I don't deem it robust.

I have fought with docker user permissions for years, and when there are volumes involved, unless using docker-compose, the best thing is letting the user set the user at runtime via docker run --user instead of hardcoding it in the Dockerfile. But apparently lightningd needs root access to create the data directory, so the only option is running as root.

Could not make directory /.lightning: Permission denied

@Raimo33 Raimo33 force-pushed the docker-refactor branch 3 times, most recently from 95545a5 to d0140cc Compare August 22, 2025 14:00
@Raimo33
Copy link
Author

Raimo33 commented Aug 25, 2025

The previous Dockerfile was not cross-compiling rust packages. Cargo was emulating with QEMU instead of cross compiling, despite the efforts as seen by modifying the cargo config.toml. I've now fixed it and it correctly cross compiles (much faster than QEMU), but the Makefile needs a retouch as it is searching the binaries in the wrong folder.

#53 185.3 cp: cannot stat 'target/release/cln-lsps-service': No such file or directory

@Raimo33 Raimo33 force-pushed the docker-refactor branch 2 times, most recently from 3ce46bb to a302995 Compare August 26, 2025 11:17
@Raimo33 Raimo33 marked this pull request as ready for review August 26, 2025 11:19
@Raimo33 Raimo33 changed the title [WIP] Refactor Dockerfile Refactor Dockerfile Aug 26, 2025
Raimo33 pushed a commit to Raimo33/lightning that referenced this pull request Aug 26, 2025
all these changelogs only apply to the Docker image.

Changelog-Added: added verification of GPG keys for the bitcoin and litecoin tarballs.
Changelog-Fixed: fixed compilation on all target architectures; each had their own bugs (poetry, missing packages...).
Changelog-Fixed: fixed cargo cross compilation. it was mistakenly using QEMU before.
Changelog-Fixed: fixed CPU compatibility bug described in issue 8456
Changelog-Changed: improve build time by 8.8x
Changelog-Changed: improve image size by 2.07x

more detailed changelog can be found on the PR: ElementsProject#8429
@Raimo33
Copy link
Author

Raimo33 commented Aug 26, 2025

Closes #8409 #8405 #8456 #5655

@madelinevibes madelinevibes added the Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. label Aug 26, 2025
Raimo33 pushed a commit to Raimo33/lightning that referenced this pull request Aug 29, 2025
all these changelogs only apply to the Docker image.

Changelog-Added: added verification of GPG keys for the bitcoin and litecoin tarballs.
Changelog-Fixed: fixed compilation on all target architectures; each had their own bugs (poetry, missing packages...).
Changelog-Fixed: fixed cargo cross compilation. it was mistakenly using QEMU before.
Changelog-Fixed: fixed CPU compatibility bug described in issue 8456
Changelog-Changed: improve build time by 8.8x
Changelog-Changed: improve image size by 2.07x

more detailed changelog can be found on the PR: ElementsProject#8429
all these changelogs only apply to the Docker image.

Changelog-Added: added verification of GPG keys for the bitcoin and litecoin tarballs.
Changelog-Fixed: fixed compilation on all target architectures; each had their own bugs (poetry, missing packages...).
Changelog-Fixed: fixed cargo cross compilation. it was mistakenly using QEMU before.
Changelog-Fixed: fixed CPU compatibility bug described in issue 8456
Changelog-Changed: improve build time by 8.8x
Changelog-Changed: improve image size by 2.07x

more detailed changelog can be found on the PR: ElementsProject#8429
apt-get clean && \
rm -rf /var/lib/apt/lists/*
#TODO: find a way to avoid copying the .git/ directory (it always invalidates the cache)
COPY .git/ .git/
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we add it in dockerignore?

Copy link
Author

Choose a reason for hiding this comment

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

no. because make needs it. it calls git describe to get the tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status::Ready for Review The work has been completed and is now awaiting evaluation or approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Dockerfile after the removal of all built-in Python plugins
5 participants