Use builder image's objcopy when building with quarkus.native.container-build=true and quarkus.native.debug.enabled=true #13963

zakkak · 2020-12-18T04:59:39Z

jerboaa

A couple of comments...

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

jerboaa · 2020-12-18T14:49:21Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

+                    objcopy(containerCommand, outputDir, "--only-keep-debug", executableName, symbols);
+                    objcopy(containerCommand, outputDir, String.format("--add-gnu-debuglink=%s", symbols), executableName);
+                }
+                // Strip debug symbols regardless, because the underlying JDK might contain them
+                objcopy(containerCommand, outputDir, "--strip-debug", executableName);


Wouldn't this spin up a container 3 times? Ideally, we'd stitch them together so that we can get away with a one time container invocation. I.e. podman run ... --entrypoint=/bin/bash <container> -c 'objcopy --only-keep-debug ... ; objcopy --add-gnu-debuglink=....; objcopy --strip-debug ... or podman run ... --entrypoint=/bin/bash <container> -c 'objcopy --strip-debug ... depending on the -g option.

It will, but that's fairly fast in comparison to the native-image build step. Keeping it this way we don't have to maintain two different code paths for container and host builds, thus keeping it simple and more readable.

I haven't tested this myself, but apparently running containers on non-Linux isn't as quick. Perhaps mac and windows use-cases with building with the container should be tested.

AFAIK Mac is almost as fast, and windows docker using WSL2 as the backend should also be pretty fast. I can do some testing on Windows, but unfortunately not on Mac.

This maybe an optimization to do as a later step (once we know this actually is a problem?)

FTR: I tested this on two laptops with similar specs.
I tested the "combined" (single docker container for all 3 processes) vs the "split" (1 container per process) approach and the results were:

Windows Linux

Combined ~16s ~0.8s

Split 3.5 + 13 + 6.5 = ~23s 0.5 + 0.5 + 0.6 = ~1.1s

Although the benefit of using the "combined" approach is pretty clear, the overhead of the "split" approach (~7s) is still relatively low given that the image build took ~260s.

zakkak · 2021-01-22T13:34:47Z

@gsmet can you please review?

gsmet · 2021-01-26T15:38:20Z

@zakkak can you fix the conflict?

Frankly, I can't backport it as is. We can't refactor everything in a PR we want to backport...

zakkak · 2021-01-27T13:33:05Z

@zakkak can you fix the conflict?

Done

Frankly, I can't backport it as is. We can't refactor everything in a PR we want to backport...

I understand. I should have actually added this PR to 1.11.0.Final in the first place.
How would you like to proceed with this since we would like it in 1.11.x? Is it OK if I create a new PR with branch 1.11 as the base?
Unfortunately the refactoring is necessary to make this change in a sensible way, otherwise we would have to duplicate a lot of code and some processing.

gsmet · 2021-02-03T17:43:29Z

As I said I'm uncomfortable backporting something that rewrites a whole class. So either we find a finer-grained approach or we don't backport it. The current approach is not safe for a backport. I don't want to break something else to fix an issue.

Now if someone really wants this, they will have to review this VERY closely to be sure everything is safe.

/cc @maxandersen @tqvarnst

rsvoboda · 2021-02-22T13:13:00Z

@zakkak can you please rebase?

zakkak · 2021-02-22T13:56:42Z

@zakkak can you please rebase?

@rsvoboda this is not worth rebasing before #14635 gets merged, unless it's a real blocker for you.

geoand · 2021-02-22T15:17:54Z

This need a rebase but we need a decision on

Whether or needs to be backported or not (based on what @gsmet mentioned)
If we want this or Add native-source package type #15233 to go in first (that PR is definitely not backport material either)

zakkak · 2021-02-22T15:21:16Z

This need a rebase but we need a decision on
1. Whether or needs to be backported or not (based on what @gsmet mentioned)

We have concluded that this does not need to be backported.

2. If we want this or #15233 to go in first

I would prefer #15233 to go in first.

geoand · 2021-02-22T15:22:22Z

OK, then I will take #15233 out of draft so it can get a proper review.

Furthermore, I'll add a test for it

zakkak · 2021-02-22T15:24:31Z

OK, then I will take #15233 out of draft so it can get a proper review.

Furthermore, I'll add a test for it

Can we please make sure #14635 goes in first, then #15233 and finally #13963 (this PR)?

geoand · 2021-02-22T15:29:11Z

Ouch... I just saw #14635... Rebasing my PR onto that one won't be fun :(

zakkak · 2021-02-22T15:41:48Z

Ouch... I just saw #14635... Rebasing my PR onto that one won't be fun :(

I feel you, if it eases your pain it will be more or less the same for this PR ;)

geoand · 2021-02-22T15:47:49Z

Ouch... I just saw #14635... Rebasing my PR onto that one won't be fun :(

I feel you, if it eases your pain it will be more or less the same for this PR ;)

Yeah, we are in the same boat :)

geoand · 2021-02-22T16:45:29Z

Turned out to be easier than I thought :)

zakkak · 2021-03-02T14:26:00Z

Turned out to be easier than I thought :)

I wish I could say the same :)

This is now ready for review. I tested it locally with all three configurations and GraalVM CE 21.0:

Host build with -Dquarkus.native.debug.enabled=true -Dquarkus.native.graalvm-home=/opt/jvms/graalvm-ce-java11-21.0.0
Builder image with -Dquarkus.native.debug.enabled=true
Remote buidler image with -Dquarkus.native.remote-container-build=true -Dquarkus.native.debug.enabled=true

@jonathan-meier FYI (It would also be great if you could give this a spin on your "remote" docker setup)

zakkak · 2021-03-02T14:29:01Z

...ent/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRemoteContainerRunner.java

@@ -13,6 +15,7 @@
 public class NativeImageBuildRemoteContainerRunner extends NativeImageBuildContainerRunner {

    private static final Logger log = Logger.getLogger(NativeImageBuildRemoteContainerRunner.class);
+    private static final String CONTAINER_BUILD_VOLUME_NAME = "quarkus-native-builder-image-project-volume";


Ideally I would prefer a dynamically generated unique volume name, but I am not sure it's worth the effort.
We could also do something like fixed-name-YYYY-MM-DD-HHMM to avoid potential conflicts with existing volumes.
WDYT?

It's straightforward to let docker generate the unique volume id, I quickly tested it locally:

final String[] createVolumeCommand = new String[] { containerRuntime.getExecutableName(), "volume", "create" }; volumeId = runCommandAndReadOutput(createVolumeCommand, "Failed to create temp volume.");

Using this as a first step in preBuild and replacing all CONTAINER_BUILD_VOLUME_NAME with volumeId.

Unfortunately it's not that simple. If you manually create the volume before starting the container, the volume (when mount) is owned by root:root and native-image fails to write to it in subsequent steps.

Creating the volume explicitly

$ podman volume create test-volume test-volume $ podman create --name temp -v test-volume:/project quay.io/quarkus/ubi-quarkus-native-image:21.0.0-java11 bfa2406d5fa0a003af99c91f077ed989006eb2441004135773429c425f9ce997 $ podman run --rm --entrypoint /bin/bash -it -v test-volume:/project quay.io/quarkus/ubi-quarkus-native-image:21.0.0-java11 -c "ls -la" total 0 drwxr-xr-x. 2 root root 6 Mar 3 11:40 . drwxr-xr-x. 19 root root 6 Mar 3 11:41 ..

Creating the volume implicitly

$ podman create --name temp -v test-volume:/project quay.io/quarkus/ubi-quarkus-native-image:21.0.0-java11 e30ffb76aa320448a3e645d7030c4dcf76b0efb83b68e3bc58a2e655c683d1de $ podman run --rm --entrypoint /bin/bash -it -v test-volume:/project quay.io/quarkus/ubi-quarkus-native-image:21.0.0-java11 -c "ls -la" total 0 drwxr-xr-x. 2 quarkus quarkus 6 Mar 3 11:42 . drwxr-xr-x. 19 root root 6 Mar 3 11:42 ..

I am not a containers expert though so I'll be happy to be corrected :)

Interesting! I checked the following:

docker 19.03.13 on Ubuntu 20.10

docker 20.10.2 on Ubuntu 20.10 in WSL

podman 2.0.6 on Ubuntu 20.10

podman 1.6.4 on CentOS 7.9

podman 3.0.1 on CentOS 7.9

in all but the last case, the /project folder was owned by the quarkus user also when creating the volume explicitly. So something seems to have changed for podman 3. I'm guessing you're running on podman 3 as well?

Indeed I am using podman 3.0.1 on Fedora 33.

Thanks for the additional info @jonathan-meier, I created containers/podman#9608.

jonathan-meier · 2021-03-02T21:01:34Z

Works like a charm! I tested various combinations of local/remote builds with debug enabled/disabled both natively on Windows and inside WSL.

zakkak · 2021-03-03T11:45:11Z

Works like a charm! I tested various combinations of local/remote builds with debug enabled/disabled both natively on Windows and inside WSL.

Thank you @jonathan-meier, I really appreciate it :)

gsmet · 2021-04-01T12:50:45Z

@zakkak @jonathan-meier I rebased this PR and also added an additional commit to fix (again) the confusion between native image name and executable name. Could you both have a look?

zakkak · 2021-04-01T14:33:08Z

Thanks @gsmet. It looks like the issue (which appears only in podman 3.0.x) I am working around in this PR (see #13963 (comment)) is fixed in podman 3.1.0 (which was released 2 days ago) so I was thinking about waiting for it to land in fedora and rework the PR to avoid the work-around. WDYT?

gsmet · 2021-04-01T14:46:52Z

If the workaround doesn't break Podman 3.1, I would get it in anyway. People don't always keep things updated.

zakkak · 2021-04-01T15:22:56Z

If the workaround doesn't break Podman 3.1, I would get it in anyway. People don't always keep things updated.

OK, I added a comment to explain that we follow this approach due to the Podman 3.0.x issue. It looks good to me (at least the linux part). It would be nice if someone could confirm that everything works as expected on windows as well.

jonathan-meier · 2021-04-05T16:04:08Z

Looks good to me too! I again tested various combinations of containerized local/remote docker builds with debug enabled/disabled both natively on Windows and inside WSL.

Please note that I did not test direct local builds on Windows producing a Windows binary (i.e. without docker, as I don't have GraalVM installed on Windows). I did not do any podman tests either.

This patch creates a volume for /project where we can copy files to/from using `docker cp`. The volume is create in the prebuild step, then used in the build step, and finally removed in the postbuild step. The new approach allows us to execute more than one commands using `docker run --rm` on the same files in /project.

zakkak · 2021-04-12T10:35:37Z

@gsmet I rebased this (cleanly). I think it's good to go (after the CI completes).

quarkus-bot · 2021-04-12T13:45:44Z

Failing Jobs - Building `8008ceb`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 8	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 8 #

📦 integration-tests/kafka

✖ io.quarkus.it.kafka.SaslKafkaConsumerTest.testReception line 48 - More details - Source on GitHub

✖ io.quarkus.it.kafka.SslKafkaConsumerTest.testReception line 56 - More details - Source on GitHub

zakkak · 2021-04-22T10:34:05Z

CI looks good @gsmet @geoand can you please merge?

PR quarkusio#13963 mistakenly stopped stripping debug information from the native executable when debug info generation is enabled resulting in both the native executable and the corresponding `.debug` file containing the debug information. Which also results in the native executable being larger when `-Dquarkus.native.debug.enabled` is used: ``` $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-nodebug 108M quarkus-integration-test-main-999-SNAPSHOT-runner-nodebug $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-debug 182M quarkus-integration-test-main-999-SNAPSHOT-runner-debug $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-debug.debug 75M quarkus-integration-test-main-999-SNAPSHOT-runner-debug.debug ``` Inspecting `quarkus-integration-test-main-999-SNAPSHOT-runner-debug` and `quarkus-integration-test-main-999-SNAPSHOT-runner-debug` with `readelf --debug-dump=info` we observe that both files are containing debug information while only the latter should contain them.

PR quarkusio#13963 mistakenly stopped stripping debug information from the native executable when debug info generation is enabled resulting in both the native executable and the corresponding `.debug` file containing the debug information. Which also results in the native executable being larger when `-Dquarkus.native.debug.enabled` is used: ``` $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-nodebug 108M quarkus-integration-test-main-999-SNAPSHOT-runner-nodebug $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-debug 182M quarkus-integration-test-main-999-SNAPSHOT-runner-debug $ du -hs quarkus-integration-test-main-999-SNAPSHOT-runner-debug.debug 75M quarkus-integration-test-main-999-SNAPSHOT-runner-debug.debug ``` Inspecting `quarkus-integration-test-main-999-SNAPSHOT-runner-debug` and `quarkus-integration-test-main-999-SNAPSHOT-runner-debug` with `readelf --debug-dump=info` we observe that both files are containing debug information while only the latter should contain them. (cherry picked from commit f69d98d)

ghost added the area/core label Dec 18, 2020

zakkak force-pushed the objcopy-incontainer branch from c7e95e5 to 4d66b88 Compare December 18, 2020 05:07

zakkak marked this pull request as ready for review December 18, 2020 12:33

jerboaa reviewed Dec 18, 2020

View reviewed changes

zakkak requested a review from gsmet December 22, 2020 02:03

zakkak force-pushed the objcopy-incontainer branch from 6b8f277 to eaf9955 Compare January 22, 2021 11:30

zakkak added the triage/backport? label Jan 22, 2021

zakkak force-pushed the objcopy-incontainer branch from eaf9955 to ac8e60e Compare January 27, 2021 13:25

jonathan-meier mentioned this pull request Jan 29, 2021

Containerized native image build on remote docker daemons (issue #1610) #14635

Merged

gsmet added the triage/backport-1.11? label Feb 10, 2021

zakkak mentioned this pull request Feb 22, 2021

Add native-source package type #15233

Merged

zakkak removed triage/backport-1.11? labels Feb 22, 2021

zakkak force-pushed the objcopy-incontainer branch from ac8e60e to efe041c Compare March 2, 2021 14:22

zakkak commented Mar 2, 2021

View reviewed changes

Base automatically changed from master to main March 12, 2021 15:55

gsmet force-pushed the objcopy-incontainer branch from efe041c to 57328b0 Compare April 1, 2021 12:49

gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Apr 1, 2021

zakkak and others added 5 commits April 12, 2021 12:39

Update documentation of container-build property

d0c0b53

Run objcopy in builder-image when using it

1bc84bc

Fix confusion between native image name and executable name

36ec894

Add comment to explain the usage of fixed volume name

8008ceb

zakkak force-pushed the objcopy-incontainer branch from ae176ad to 8008ceb Compare April 12, 2021 10:34

gsmet approved these changes Apr 22, 2021

View reviewed changes

gsmet merged commit 7299fd0 into quarkusio:main Apr 22, 2021

quarkus-bot bot added this to the 2.0 - main milestone Apr 22, 2021

zakkak deleted the objcopy-incontainer branch July 12, 2022 12:14

zakkak mentioned this pull request Jul 12, 2022

Strip debug information from the native executable unconditionally #26681

Merged

	Windows	Linux
Combined	~16s	~0.8s
Split	3.5 + 13 + 6.5 = ~23s	0.5 + 0.5 + 0.6 = ~1.1s

Use builder image's objcopy when building with quarkus.native.container-build=true and quarkus.native.debug.enabled=true #13963

Use builder image's objcopy when building with quarkus.native.container-build=true and quarkus.native.debug.enabled=true #13963

Conversation

zakkak commented Dec 18, 2020 • edited Loading

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakkak Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakkak commented Jan 22, 2021

gsmet commented Jan 26, 2021

zakkak commented Jan 27, 2021

gsmet commented Feb 3, 2021

rsvoboda commented Feb 22, 2021

zakkak commented Feb 22, 2021

geoand commented Feb 22, 2021 • edited Loading

zakkak commented Feb 22, 2021

geoand commented Feb 22, 2021 • edited Loading

zakkak commented Feb 22, 2021

geoand commented Feb 22, 2021

zakkak commented Feb 22, 2021

geoand commented Feb 22, 2021

geoand commented Feb 22, 2021

zakkak commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Creating the volume explicitly

Creating the volume implicitly

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-meier commented Mar 2, 2021

zakkak commented Mar 3, 2021

gsmet commented Apr 1, 2021

zakkak commented Apr 1, 2021

gsmet commented Apr 1, 2021

zakkak commented Apr 1, 2021

jonathan-meier commented Apr 5, 2021

zakkak commented Apr 12, 2021

quarkus-bot bot commented Apr 12, 2021

Failing Jobs - Building 8008ceb

Test Failures

⚙️ JVM Tests - JDK 8 #

📦 integration-tests/kafka

zakkak commented Apr 22, 2021

zakkak commented Dec 18, 2020 •

edited

Loading

zakkak Dec 18, 2020 •

edited

Loading

geoand commented Feb 22, 2021 •

edited

Loading

geoand commented Feb 22, 2021 •

edited

Loading

Failing Jobs - Building `8008ceb`