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

Fix launch toggle for Native Image Java applications #259

Merged

Conversation

PerfectSlayer
Copy link
Contributor

@PerfectSlayer PerfectSlayer commented Nov 27, 2023

Summary

This PR will disable toggle and its launch configuration for Java Native Image configuration.
It will also update the README and the Buildpack descriptor accordingly.

Use Cases

When building a native java application container, it will refuse to start assuming you inserted the (Java) toggle layer on a NodeJS application as JAVA_TOOL_OPTIONS is not used for Java native application (the agent being used at build time, not run/launch time).

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@PerfectSlayer PerfectSlayer requested a review from a team as a code owner November 27, 2023 14:03
@PerfectSlayer PerfectSlayer changed the title Bbujon/fix native image toggle Fix launch toggle for Native Image Java applications Nov 27, 2023
@dmikusa dmikusa added semver:patch A change requiring a patch version bump type:bug A general bug labels Dec 2, 2023
@dmikusa
Copy link
Contributor

dmikusa commented Dec 2, 2023

Thanks for this PR. I'm not totally sure I follow the problem that it is trying to address though. Can you expand on the issue you're seeing and include steps to reproduce the problem? I'll need to know how I can replicate the issue to do validation test. Thanks

datadog/build.go Outdated Show resolved Hide resolved
@PerfectSlayer
Copy link
Contributor Author

Hi @dmikusa

Sure, I will provide you a way to reproduce and understand the issue.

Reproducing

  1. First, let's get a Spring Boot application with Native Image support from start.spring.io.

  2. Once downloaded, edit the build.gradle file to enable the Databdog buildpack by adding at the end of the file:

tasks.named('bootBuildImage') {
	environment['BP_DATADOG_ENABLED'] = 'true'
}
  1. Once the configuration done, build the project using: ./gradlew bootBuildImage
  2. Finally, run the application using: docker run --rm -p 8080:8080 docker.io/library/demo:0.0.1-SNAPSHOT

You should have the following output:

disabling Datadog at launch time is unsupported for Node
ERROR: failed to launch: exec.d: failed to execute exec.d file at path '/layers/paketo-buildpacks_datadog/helper/exec.d/toggle': exit status 1

Investigating

disabling Datadog at launch time is unsupported for Node means the toggle app believe it is running a NodeJS application.
Why? Because it did not find the JAVA_TOOL_OPTIONS environment variable (code).

This behavior is bogus as JAVA_TOOL_OPTIONS is not expected to be present when using Native Image build.
So my pull request is disabling the addition of the toggle layer when Native Image is used. I removed it as it makes no sense to try to disable the Datadog tracing library once it is baked into the application at build time.

Testing

From here? How do you think we can test it and ensure the toggle feature won't break native image builds again?

Best,
Bruce

@dmikusa
Copy link
Contributor

dmikusa commented Dec 4, 2023

This behavior is bogus as JAVA_TOOL_OPTIONS is not expected to be present when using Native Image build.
So my pull request is disabling the addition of the toggle layer when Native Image is used. I removed it as it makes no sense to try to disable the Datadog tracing library once it is baked into the application at build time.

I'm not familiar with the native-image support for agents like Datadog, but if they cannot be toggled without rebuilding the binary then what you're saying makes perfect sense.

From here? How do you think we can test it and ensure the toggle feature won't break native image builds again?

For testing, I think we just want to make sure that the toggle helper isn't added given the circumstances you mentioned.

If you start with a copy of this test that should get you most of the way there. Just wrap your new test in a Context and set up the test conditions in the Before method. See here for an example of test setup.

It seems like one test with JAVA_TOOL_OPTIONS set to ensure the helper is added, and then one test with it unset to ensure the helper is not added would be sufficient. You can use t.Setenv to set an env safely, it'll automatically be unset after the test. If you just want to ensure the env variable isn't set then, you can os.Unsetenv in the Before block.

@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-native-image-toggle branch from 54a2c97 to 82d63a9 Compare December 5, 2023 08:55
@PerfectSlayer
Copy link
Contributor Author

Thanks for the clear directions to help me to write tests!

I went with a test that ensures the toggle layer is not added when Native Image build are enabled.
I would rather not relying on JAVA_TOOL_OPTIONS as is will be deprecated in a near future and is related to the toggle logic, not the build logic for whose the built_tests is. Is it okay with you?

@dmikusa
Copy link
Contributor

dmikusa commented Dec 6, 2023

Yes, that sounds good. Everything looks good. The branch is just out of date. Normally, GH lets me update it but I can't on this PR. Can you update the branch? Then I can get this merged. Thanks!

@PerfectSlayer PerfectSlayer force-pushed the bbujon/fix-native-image-toggle branch from 82d63a9 to 239a0cc Compare December 7, 2023 09:59
@PerfectSlayer
Copy link
Contributor Author

PerfectSlayer commented Dec 7, 2023

Can you update the branch?

Sure! Just did 👌

@dmikusa dmikusa merged commit 2e4c590 into paketo-buildpacks:main Dec 7, 2023
5 checks passed
@PerfectSlayer PerfectSlayer deleted the bbujon/fix-native-image-toggle branch December 7, 2023 14:23
@dmikusa
Copy link
Contributor

dmikusa commented Dec 7, 2023

To follow up. This has been released in the 4.6.0 version of the Datadog buildpack.

@PerfectSlayer
Copy link
Contributor Author

Great! Thanks for your help getting this fixed, merged and published 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants