plugin enhancement

[zhouwei0115]

file system service path enabled to save heap dump file;
error message given when required tools not present;
container path and local path to save dump file

[TimGerlach]

Thank you very much for this Pull-Request and the additional features it introduces.

I have made some comments. Please make sure to:

• use gofmt for code formatting
• remove all unnecessary comments
• remove .DS_Store from the PR (you can add it to the .gitignore)
• add unit tests for cfutils.go
• check out other comments I made in the PR

[TimGerlach]

It looks like the code wasn't formatted with a code formatter. Can you please apply gofmt?

[zhouwei0115]

formatted

[TimGerlach]

I've added some more comments to go through.
In general, we're missing to test the whole utils/cfutils.go. However, since I know that you're currently assigned with limited time, we can take this as a follow up improvement.

Before approving I'd like to quickly try out this build of the cf plugin myself with a demo Java app. I just saw that we have never added to the README how one can test the plugin locally. Can you maybe share how you've tested the plugin so that I do the test myself?

[zhouwei0115]

I've added some more comments to go through.
In general, we're missing to test the whole utils/cfutils.go. However, since I know that you're currently assigned with limited time, we can take this as a follow up improvement.

Before approving I'd like to quickly try out this build of the cf plugin myself with a demo Java app. I just saw that we have never added to the README how one can test the plugin locally. Can you maybe share how you've tested the plugin so that I do the test myself?

part 1, required tools present

java-app: spring-music
file system mounted path: /var/fsjavadev

test case 1 - without any parameters set:

command: cf java heap-dump spring-music
output:
heap dump will not be copied as parameter local-dir was not set
heap dump filed deleted in app container
behind the ouput: monitor the container by ssh command 'cf ssh spring-music', will find during the plugin running, there will be one dump file with name 'spring-music-heapdump-.hprof' generated in /var/fsjavadev, and removed when the plugin exit running.

test case 2 - with parameter keep set:

command: cf java heap-dump spring-music -k
output:
heap dump will not be copied as parameter local-dir was not set
behind the ouput: dump file should be kept in container under directory: /var/fsjavadev, we can check with command: 'cf ssh spring-music -c "ls -lrt /var/fsjavadev/*hprof"' and got result:
-rw------- 1 vcap vcap 62270162 Aug 11 02:50 /var/fsjavadev/spring-music-heapdump-b9faef01-f061-4d9b-a0d0-785f741e4205.hprof

test case 3 - with invalid container dir set:

command: cf java heap-dump spring-music -container-dir /var/notexists
output:
FAILED
the container path specified doesn't exist or have no read and write access, please check and try again later

test case 4 - with valid container dir set:

command: cf java heap-dump spring-music -container-dir /var/fsjavadev
heap dump will not be copied as parameter local-dir was not set
heap dump filed deleted in app container
behind the ouput: monitor the container by ssh command 'cf ssh spring-music', will find during the plugin running, there will be one dump file with name 'spring-music-heapdump-.hprof' generated in /var/fsjavadev, and removed when the plugin exit running.

test case 5 - with valid container dir and keep set:

command: cf java heap-dump spring-music -container-dir /var/fsjavadev -keep
heap dump will not be copied as parameter local-dir was not set
behind the ouput: dump file should be kept in container under directory: /var/fsjavadev, we can check with command: 'cf ssh spring-music -c "ls -lrt /var/fsjavadev/*hprof"' and got result:
-rw------- 1 vcap vcap 62270162 Aug 11 02:50 /var/fsjavadev/spring-music-heapdump-b9faef01-f061-4d9b-a0d0-785f741e4205.hprof <--from test case 2
-rw------- 1 vcap vcap 62662145 Aug 11 02:59 /var/fsjavadev/spring-music-heapdump-9fd5d28c-3a60-4eda-8998-2f794d9fce29.hprof

test case 6 - with valid container dir and local-dir set:

command: cf java heap-dump spring-music -container-dir /tmp -local-dir /Users/i318048
heap dump filed saved to: /Users/i318048/spring-music-heapdump-492b171a-5969-4b53-ae26-f4f0b2770c2c.hprof
heap dump filed deleted in app container
behind the output: dump file was generated in /tmp and get deleted when plugin exits running

test case 7 - with valid container dir and invalid local-dir set:

command: cf java heap-dump spring-music -container-dir /tmp -local-dir /Users/invalid
output:
FAILED
Error creating local file at /Users/invalid/spring-music-heapdump-91b34f7e-a3fd-42b0-af73-eb1f07f714e6.hprof. Please check that you are allowed to create files at the given local path.
behind the output: the dump file was generated under specified path /tmp, and user can copy the via a command like 'cf ssh spring-music -c "cat /tmp/spring-music-heapdump-.hprof" > /valid/local/path/file_name '

test case 8 - with local-dir set:

command: cf java heap-dump spring-music -local-dir /Users/i318048
output:
heap dump filed saved to: /Users/i318048/spring-music-heapdump-860f7175-e42d-4f40-b7ee-9a651946735b.hprof
heap dump filed deleted in app container

part 2, required tools not ready:

java-app: firstappzw
file system: not present

test case 1: ssh disabled

command: cf java heap-dump firstappzw -k
output:
FAILED
ssh is not enabled for app: 'firstappzw', please run below 2 shell commands to enable ssh and try again(please note application should be restarted before take effect):
cf enable-ssh firstappzw
cf restart firstappzw

test case 2: jvmmon and jmap missing(currently remove these two tools from container):

command: cf java heap-dump firstappzw -k
output:
FAILED
jvmmon or jmap are required for generating heap dump, you can modify your application manifest.yaml on the 'JBP_CONFIG_OPEN_JDK_JRE' environment variable. This could be done like this:
---
applications:
- name: <APP_NAME>
memory: 1G
path: <PATH_TO_BUILD_ARTIFACT>
buildpack: https://github.com/cloudfoundry/java-buildpack
env:
JBP_CONFIG_OPEN_JDK_JRE: '{ jre: { repository_root: "https://java-buildpack.cloudfoundry.org/openjdk-jdk/bionic/x86_64", version: 11.+ } }'

test case 3: with valid container dir and local dir set:

command: cf java heap-dump firstappzw -container-dir /tmp -local-dir /Users/i318048
output:
heap dump filed saved to: /Users/i318048/firstappzw-heapdump-923e9722-423c-4724-702f-3e2b17132b5a.hprof
heap dump filed deleted in app container
behind the output: dump file was generated in /tmp and get deleted when plugin exits running

test case 4: with valid local dir set:

command: cf java heap-dump firstappzw -local-dir /Users/i318048
output:
heap dump filed saved to: /Users/i318048/firstappzw-heapdump-923e9722-423c-4724-702f-3e2b17132b5a.hprof
heap dump filed deleted in app container
behind the output: without container directory specified, the dump file will be saved to file system attached, and to /tmp if file system not present

[TimGerlach]

I think this is still a copy paste error, isn't it? I think this should be something like:

module github.com/SAP/cf-cli-java-plugin

[zhouwei0115]

its generated by go mod init

[TimGerlach]

I see. According to the go mod documentation, however, this should be set to the source code repository:

The module path should be a path from which Go tools can download the module source. In practice, this is typically the module source's repository domain and path to the module code within the repository. The go command relies on this form when downloading module versions to resolve dependencies on the module user's behalf.
Even if you're not at first intending to make your module available for use from other code, using its repository path is a best practice that will help you avoid having to rename the module if you publish it later.

[TimGerlach]

Can you please also test this with the sap_java_buildpack? I tried to test it and it didn't work as per my comment.

[TimGerlach]

I think we're almost done. Only few minor things left before we can merge it.
The plugin will significantly benefit from your valuable contribution, it's pretty cool to use it with the new options :)
Thank you for your ongoing support and patience with my slow reviews.

Please note that I will be on vacation until August 25 and will look at the PR again after my return.

[zhouwei0115]

Hi Tim, When I tried modify it to like ‘module github.com/SAP/cf-cli-java-plugin’, compile will fail with error below: go: updates to go.mod needed; to update it: go mod tidy make: *** [compile] Error 1 Regards, Wei From: Tim Gerlach ***@***.***> Reply-To: SAP/cf-cli-java-plugin ***@***.***> Date: Friday, August 13, 2021 at 9:27 PM To: SAP/cf-cli-java-plugin ***@***.***> Cc: "Zhou, Wei" ***@***.***>, Author ***@***.***> Subject: Re: [SAP/cf-cli-java-plugin] plugin enhancement (#24) @TimGerlach commented on this pull request.

________________________________ In go.mod<#24 (comment)>:

@@ -0,0 +1,29 @@

+module cf.plugin.ref/requires I see. According to the go mod documentation<https://golang.org/doc/modules/gomod-ref#module-syntax>, however, this should be set to the source code repository: The module path should be a path from which Go tools can download the module source. In practice, this is typically the module source's repository domain and path to the module code within the repository. The go command relies on this form when downloading module versions to resolve dependencies on the module user's behalf. Even if you're not at first intending to make your module available for use from other code, using its repository path is a best practice that will help you avoid having to rename the module if you publish it later. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#24 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AVAOAUK5GJ3ELFCKDSOEBM3T4UMRZANCNFSM5BGOYJQA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.

[TimGerlach]

Hi Tim,
When I tried modify it to like ‘module github.com/SAP/cf-cli-java-plugin’, compile will fail with error below:
go: updates to go.mod needed; to update it:
go mod tidy
make: *** [compile] Error 1

Hi Wei,

I have tried to change the module name locally to:

module github.com/SAP/cf-cli-java-plugin

and ran go build without any errors. With the new module name it properly produces a binary called cf-cli-java-plugin instead of the previous binary name requires. Can you try to run go mod tidy as proposed by the go compiler?

[TimGerlach]

I just went through the test cases and it all looked fine for both buildpacks (java, sap_java_buildpack) except for one scenario.
When using the cf java plugin with your demo app spring-music which uses the sap_java_buildpack and a file system service mounted at /var/fsjavadev the plugin doesn't seem to honour the existence of the file system service by default:

$ cf java heap-dump spring-music

successfully created heap dump in application container at: ./app/java_pid7_3.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Based on your code I expected the plugin to automatically detect the file system service file path and create the heap dump at /var/fsjavadev by default, but it created the heap dump at .app/.

Even when setting the container path explicitly, it's ignored:

$ cf java heap-dump spring-music -container-dir /var/fsjavadev/timtest
successfully created heap dump in application container at: ./app/java_pid7_4.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Edit: I had another look at our code and it seems that we don't pass a path argument to jvmmon yet. I have not found this option in the manual of jvmmon so I have raised a question to the SAP JVM team to find out if it's possible to create a heap dump at a configurable path with jvmmon at all. Alternatively, we need to try whether -XX:HeapDumpPath has an influence on the heap dump path for jvmmon and need to document how to set this in case a file system service is used with the SAP JVM.

According to the SAP JVM help:

-XX:HeapDumpPath=
when HeapDumpOnOutOfMemoryError is on, the path (filename or
directory) of the dump file (defaults to java_pid.hprof
in the working directory).Please note that all user
triggered heap dumps will also be written to the this
directory by default (but with filename
java_pid@PID_@COUNT.hprof) unless HeapDumpOnDemandPath is
set explicitly

[zhouwei0115]

I just went through the test cases and it all looked fine for both buildpacks (java, sap_java_buildpack) except for one scenario.
When using the cf java plugin with your demo app spring-music which uses the sap_java_buildpack and a file system service mounted at /var/fsjavadev the plugin doesn't seem to honour the existence of the file system service by default:

$ cf java heap-dump spring-music

successfully created heap dump in application container at: ./app/java_pid7_3.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Based on your code I expected the plugin to automatically detect the file system service file path and create the heap dump at /var/fsjavadev by default, but it created the heap dump at .app/.

Even when setting the container path explicitly, it's ignored:

$ cf java heap-dump spring-music -container-dir /var/fsjavadev/timtest
successfully created heap dump in application container at: ./app/java_pid7_4.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Edit: I had another look at our code and it seems that we don't pass a path argument to jvmmon yet. I have not found this option in the manual of jvmmon so I have raised a question to the SAP JVM team to find out if it's possible to create a heap dump at a configurable path with jvmmon at all. Alternatively, we need to try whether -XX:HeapDumpPath has an influence on the heap dump path for jvmmon and need to document how to set this in case a file system service is used with the SAP JVM.

According to the SAP JVM help:

-XX:HeapDumpPath=
when HeapDumpOnOutOfMemoryError is on, the path (filename or
directory) of the dump file (defaults to java_pid.hprof
in the working directory).Please note that all user
triggered heap dumps will also be written to the this
directory by default (but with filename
java_pid@PID_@COUNT.hprof) unless HeapDumpOnDemandPath is
set explicitly

Set cf env JAVA_OPTS as -XX:HeapDumpPath=/var/fsjavadev -XX:HeapDumpOnDemandPath=/tmp
and restage it, doesn't take effect, heap dump still generated to './app/' folder.

Oliver's suggestion works,

1. log into app over ssh
2. run jvmmon without any command specified

run jvmmon without specify any command, then change command line flag with interactive way.

I just went through the test cases and it all looked fine for both buildpacks (java, sap_java_buildpack) except for one scenario.
When using the cf java plugin with your demo app spring-music which uses the sap_java_buildpack and a file system service mounted at /var/fsjavadev the plugin doesn't seem to honour the existence of the file system service by default:

$ cf java heap-dump spring-music

successfully created heap dump in application container at: ./app/java_pid7_3.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Based on your code I expected the plugin to automatically detect the file system service file path and create the heap dump at /var/fsjavadev by default, but it created the heap dump at .app/.

Even when setting the container path explicitly, it's ignored:

$ cf java heap-dump spring-music -container-dir /var/fsjavadev/timtest
successfully created heap dump in application container at: ./app/java_pid7_4.hprof
heap dump will not be copied as parameter `local-dir` was not set
heap dump filed deleted in app container

Edit: I had another look at our code and it seems that we don't pass a path argument to jvmmon yet. I have not found this option in the manual of jvmmon so I have raised a question to the SAP JVM team to find out if it's possible to create a heap dump at a configurable path with jvmmon at all. Alternatively, we need to try whether -XX:HeapDumpPath has an influence on the heap dump path for jvmmon and need to document how to set this in case a file system service is used with the SAP JVM.

According to the SAP JVM help:

-XX:HeapDumpPath=
when HeapDumpOnOutOfMemoryError is on, the path (filename or
directory) of the dump file (defaults to java_pid.hprof
in the working directory).Please note that all user
triggered heap dumps will also be written to the this
directory by default (but with filename
java_pid@PID_@COUNT.hprof) unless HeapDumpOnDemandPath is
set explicitly

Tried set -XX: HeapDumpPath in cf env and restage the app, doesn't work.
set -XX:HeapDumpOnDemandPath in container works, but it requires a interactive way to set. and seems we can't know the path set.
This will cause problem when find the dump file with 'find' command