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.

Sign up for GitHub

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

Jump to bottom

Implement rfc 0003 #95

Merged
merged 3 commits into from
Aug 26, 2021
Merged

Implement rfc 0003 #95

merged 3 commits into from
Aug 26, 2021

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Aug 25, 2021

Summary

This is a partial implementation of RFC #0003.

It implements steps #1 and #2 in the parity section.

Use Cases

  • removes the kill agent
  • uses the JVM flag to instruct the JVM to exit on OOME
  • uses the JVM flags to trigger a heap dump on OOME

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).

Daniel Mikusa added 2 commits August 24, 2021 14:48
Remove JVM Kill Agent
0f7fa9b 
Signed-off-by: Daniel Mikusa <[email protected]>
Force JVM to exit on error & add trigger for heap dumps on error
e2c5e58 
- Set `-XX:+ExitOnOutOfMemoryError` for all apps, it is automatically appended to `JAVA_TOOL_OPTIONS`
- If `$BPL_HEAP_DUMP_PATH` is set:
   - Take the contents as the path to which heap dumps will be written (must be a directory!)
   - Append a UUID to the end of the path, this creates a unique directory into which the dump will be written. This is necessary for environments like Kubernetes where many instances of an application could all be running at once. The UUID is logged so a user can look up the directory name into which the heap dump will be written.
   - If not set already, it adds `-XX:+HeapDumpOnOutOfMemoryError` to `JAVA_TOOL_OPTIONS`
   - If not set already, it adds `-XX:HeapDumpPath=<path>`, where `<path>` is sets to the path of the directory calculated in the first two steps above, to `JAVA_TOOL_OPTIONS`.

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa dmikusa added semver:minor A change requiring a minor version bump type:enhancement A general enhancement labels Aug 25, 2021
@dmikusa dmikusa requested a review from a team August 25, 2021 03:44
@dmikusa
Copy link
Contributor Author

dmikusa commented Aug 25, 2021

@pivotal-david-osullivan any chance you could take a look at this?

I think this is sufficient for the first step of implementing RFC #3. It gets us most of the way to parity with the JVM kill agent & it's enough so that Java can run on the Tiny stack.

To test:

  1. Add replace github.com/paketo-buildpacks/libjvm v1.28.0 => /Users/dmikusa/Code/JavaBuildpacks/paketo-buildpacks/libjvm to the bottom of a JVM provider buildpack's go.mod file.
  2. Run go mod download all
  3. package_buildpack <provider/buildpack>
  4. pack build something. I used this test app because I happened to have it available and it's easy to trigger OOMEs. Any app that you can use to trigger an OOME should work though.

Let me know your thoughts. Thanks

Changes to heap dump path:
1e5afb9 
- Pass in a file, not a directory
- Do not use a UUID, instead use an RFC3339 formatted date

The date is easier to correlate than a UUID, doesn't require a dependency, and should be unique enough. If we need something that's more unique, we can revisit this down the road.

In addition, we switch from passing a folder as the heap path, because each time a container runs it will at most only ever write one file. Passing the full path to a file instead of a directory allows us to put the date into the file name itself, which seems a little more useful.

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa
Copy link
Contributor Author

dmikusa commented Aug 26, 2021

Based on feedback, I switch from using UUIDs to using an RFC3339 formatted date.

This change allows you to retrieve dumps in at least two different ways:

  1. docker run -it -p 8080:8080 -m 1G -e BPL_HEAP_DUMP_PATH=/dumps -v $PWD/dumps:/dumps apps/memory-waster which will write to the mounted volume.
  2. docker run -it -p 8080:8080 -m 1G -e BPL_HEAP_DUMP_PATH=/tmp apps/memory-waster which will write into the container (/tmp is convenient cause we can write there, you could use /workspace or anywhere else you can write). You can then run docker cp <container-id>:/tmp/java_2021-08-26T19:03:13Z.hprof ./dumps/ to pull the file out of the exited container.

Similar options are available when running on K8s.

@dmikusa dmikusa merged commit 1d3a5bb into main Aug 26, 2021
@dmikusa dmikusa deleted the implement-rfc-0003 branch August 26, 2021 19:13
@dmikusa dmikusa mentioned this pull request Aug 26, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@dmikusa