Included a new MXBean to dynamically configure dump options #718

chandrams · 2017-11-30T08:37:46Z

Included a new MXBean to dynamically configure dump options. Internally it reuses the methods in com.ibm.jvm.Dump APIs to trigger dumps.

Issue #153
Signed-off-by: Chandrakala chandra-ms@in.ibm.com

pshipton · 2017-12-13T22:05:58Z

jcl/src/com.ibm.management/share/classes/com/ibm/lang/management/DiagnosticsMXBean.java

+ * </table>
+ */
+
+public interface DiagnosticsMXBean extends PlatformManagedObject {


This would be better named OpenJ9DiagnosticsMXBean

pshipton · 2017-12-13T22:11:28Z

...com.ibm.management/share/classes/com/ibm/lang/management/internal/DiagnosticsMXBeanImpl.java

+	 */
+	@Override
+	public void triggerDump(String dumpAgent) throws IllegalArgumentException {
+                switch (dumpAgent) {


I think there should be a security check here. Although these Dump APIs contain security checks, they are only enabled when the com.ibm.jvm.enableLegacyDumpSecurity system property is set to true, which it is not by default. Considering you can attach to a running JVM and invoke this, the proper security checks need to be in place.

pshipton · 2017-12-13T22:16:10Z

...com.ibm.management/share/classes/com/ibm/lang/management/internal/DiagnosticsMXBeanImpl.java

+	 * {@inheritDoc}
+	 */
+	@Override
+	public void configureDumpOptions(String dumpOptions) throws InvalidDumpOptionException, DumpConfigurationUnavailableException {


Should this be called setDumpOptions instead of configureDumpOptions? According to the MXBean naming conventions, a setter should be called set...
As a practical example, jconsole handles the APIs differently when they are getters, setters and boolean APIs,

pshipton · 2017-12-13T22:17:11Z

...com.ibm.management/share/classes/com/ibm/lang/management/internal/DiagnosticsMXBeanImpl.java

+	 */
+	@Override
+	public void configureDumpOptions(String dumpOptions) throws InvalidDumpOptionException, DumpConfigurationUnavailableException {
+		System.out.println("Dump Options = " + dumpOptions);


This PR is marked WIP, but its been sitting for a while with no updates, not sure if you are ready for a review? The println() needs to be removed before merging.

chandrams · 2018-01-15T11:42:49Z

@pshipton Sorry for the delay I was on vacation. I have a query,

Dump.setDumpOptions() did not work when I passed the below option:
"java:events=throw,filter=java/lang/NullPointerException#DumpTest.main*,range=1..1"

After investigation, I found -Xdump:dynamic has to be specified if throw/catch events are configured through Dump.setDumpOptions() when JIT is ON, whereas this option is not required when we set the above option on command line using -Xdump.

Shouldn't this be documented in Dump API or somewhere else? I could not find any documentation on this.

pshipton · 2018-01-17T03:56:57Z

Shouldn't this be documented in Dump API ?

Yes it should. I've seen this mentioned elsewhere recently as well, but can't remember where. Can you please request an update to the User Guide.

chandrams · 2018-01-17T06:39:51Z

Thanks Peter. I had asked the same query here too - #153

chandrams · 2018-01-23T06:14:42Z

@pshipton - I have implemented the review comments and also fixed some synchronization issues in the test case. Please review. Thanks.

pshipton · 2018-01-23T20:04:22Z

...k.management/share/classes/com/ibm/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

 import com.ibm.jvm.DumpConfigurationUnavailableException;
 import com.ibm.jvm.InvalidDumpOptionException;

 import com.ibm.jvm.Trace;
 import com.ibm.lang.management.OpenJ9DiagnosticsMXBean;

 public class OpenJ9DiagnosticsMXBeanImpl implements OpenJ9DiagnosticsMXBean {
+	private static final DumpPermission DUMP_PERMISSION = new DumpPermission();


Checking security here is no longer required. Since my original comment, OpenJ9 has turned on the legacy security checks by default, in #979.

pshipton · 2018-01-23T20:19:28Z

jcl/src/jdk.management/share/classes/com/ibm/lang/management/OpenJ9DiagnosticsMXBean.java

+	 *
+	 * @throws DumpConfigurationUnavailableException - if the configuration cannot be changed because a dump is already in progress.
+	 */
+	public void resetDumpOption() throws DumpConfigurationUnavailableException;


This should be resetDumpOptions with a 's', to match the jvm.Dump API being called.

pshipton · 2018-01-23T20:20:30Z

jcl/src/jdk.management/share/classes/com/ibm/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * This function sets options for the dump subsystem.
+	 * The dump option is passed in as a String. Use the same syntax as the -Xdump command-line option, with the 
+	 * initial -Xdump: omitted. See Using the -Xdump option as described in the section on dump agents in the 
+	 * documentation for the IBM JVM. This method may throw a DumpConfigurationUnavailableException if the dump


This should not refer to IBM

pshipton · 2018-01-23T20:21:12Z

jcl/src/jdk.management/share/classes/com/ibm/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * This function triggers the specified dump agent.
+	 * The JVM will attempt to write the file to the specified file name. This may
+	 * include replacement tokens as documented in the section on dump agents
+	 * in the documentation for the IBM JVM.


Should not refer to IBM

pshipton · 2018-01-23T20:21:57Z

...src/jdk.management/share/classes/com/ibm/lang/management/internal/PlatformMBeanProvider.java

@@ -32,6 +32,7 @@
 import com.ibm.lang.management.JvmCpuMonitorMXBean;
 import com.ibm.virtualization.management.internal.GuestOS;
 import com.ibm.virtualization.management.internal.HypervisorMXBeanImpl;
+import com.ibm.lang.management.OpenJ9DiagnosticsMXBean;

 /**
 * This class implements the service-provider interface to make IBM-specific


Please change IBM to OpenJ9 here.

pshipton · 2018-01-23T20:24:44Z

jcl/src/jdk.management/share/classes/com/ibm/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * @param dumpAgent - the dump agent to be triggered
+	 * @throws IllegalArgumentException - if the specified dump agent is invalid or unsupported by this method.
+	 * @throws java.lang.RuntimeException - if the vm does not contain RAS dump support
+	 * @throws java.lang.SecurityException - if there is a security manager and it doesn't allow the checks required to take this dump


please use different wording, perform instead of take would work.

or trigger

@pshipton I have taken this description from Dump.java, Should I change take to trigger in that file too?

yes, please change it there too

pshipton · 2018-01-23T20:32:10Z

...k.management/share/classes/com/ibm/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+                                Dump.SnapDump();
+				break;
+                        default:
+                                throw new IllegalArgumentException("Invalid or Unsupported Dump Agent Option, cannot be triggered");


Please externalize this message. For an example, search for /*[MSG and also see openj9/jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties

pshipton · 2018-01-23T20:32:23Z

...k.management/share/classes/com/ibm/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+                                fileName = Dump.snapDumpToFile(fileNamePattern);
+				break;
+                        default:
+                                throw new IllegalArgumentException("Invalid or Unsupported Dump Agent Option, cannot be triggered");


Please externalize the message

pshipton · 2018-01-23T20:34:27Z

test/JLM_Tests/src/org/openj9/test/java/lang/management/TestOpenJ9DiagnosticsMXBean.java

+ *
+ */
+
+@Test(groups = { "level.sanity" })


I don't think this needs to be a sanity test.

I have changed it to level.extended, is this alright or is there any other group that I need to include it in?

I think its fine. The test team can comment on this.

pshipton · 2018-01-23T20:41:01Z

@llxia please have someone review the tests

chandrams · 2018-03-09T12:27:04Z

pull request for openjdk8 - ibmruntimes/openj9-openjdk-jdk8#60

Submitted a personal build, openjdk8 build was created and all sanity tests passed. So, I think it is fine to merge the changes required in the make. Have asked the same in the above pull request too.

keithc-ca · 2018-03-12T18:38:43Z

jcl/src/jdk.management/share/classes/openj9/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * @throws IllegalArgumentException if the specified dump agent is invalid or unsupported by this method
+	 * @throws RuntimeException if the vm does not contain RAS dump support
+	 * @throws SecurityException if there is a security manager and it doesn't allow the checks required to trigger this dump
+	 */


Please document NPE (if dumpAgent is null).

keithc-ca · 2018-03-12T18:41:35Z

jcl/src/jdk.management/share/classes/openj9/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * @throws IllegalArgumentException if the specified dump agent is invalid or unsupported by this method
+	 * @throws SecurityException if there is a security manager and it doesn't allow the checks required to trigger this dump
+	 */
+	public String triggerDumpToFile(String dumpAgent, String fileNamePattern) throws IllegalArgumentException, InvalidOptionException;


Please document NPE (if dumpAgent is null).

keithc-ca · 2018-03-12T18:43:12Z

...dk.management/share/classes/openj9/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+		}
+/*[ELSE]
+		try {
+			Dump.resetDumpOptions();


Can you please move the conditional boundaries as only the body of the try statement is different (and to similar situations in other methods).

llxia · 2018-03-12T20:03:28Z

test/JLM_Tests/testng.xml

@@ -56,6 +56,7 @@
 			<class name="org.openj9.test.java.lang.management.TestMisc" />
 			<class name="org.openj9.test.java.lang.management.TestManagementUtils" />
 			<class name="org.openj9.test.java.lang.management.TestLoggingMXBean" />
+			<class name="org.openj9.test.java.lang.management.TestOpenJ9DiagnosticsMXBean" />


Sorry for late review. I had issue with my email filtering.

TestOpenJ9DiagnosticsMXBean has annotation level.extended and it is added into JLM_Tests_IBMinternal in testng.xml.
In playlist.xml, we only run sanity for JLM_Tests_IBMinternal, which means we will not run tests in TestOpenJ9DiagnosticsMXBean.

chandrams · 2018-03-14T14:02:52Z

@llxia @keithc-ca

Implemented the review comments, please review again. Thanks

llxia · 2018-03-14T16:34:18Z

test/JLM_Tests/playlist.xml

+			<variation>Mode100</variation>
+			<variation>Mode101</variation>
+		</variations>
+		<command>$(ADD_JVM_LIB_DIR_TO_LIBPATH) \


Please run test in REPORTDIR as the test may generate core file.

$(MKTREE) $(REPORTDIR); \ cd $(REPORTDIR); \

@llxia I have made these changes, please review. Thanks

pshipton · 2018-03-22T21:20:33Z

...dk.management/share/classes/openj9/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+	 */
+	@Override
+	public void resetDumpOptions() throws ConfigurationUnavailableException {
+		try {


Although the dump APIs already check security permissions, it may be useful to have an extra layer of permission checking for the MXBean, since the MXBean can be used remotely. Please add a security check for java.lang.ManagementPermission("control") (i.e. ManagementPermissionHelper.MPCONTROL) to each of the APIs. This allows the local application to have DumpPermssion, but if ManagementPermission("control") is not granted then the MXBean APIs cannot be used.

ManagementPermissionHelper is an internal class in java.management module, not sure if there would be any restrictions in accessing internal classes using reflection from another module in future. Instead can I just use ManagementPermission("control") directly?

Yes, you can use ManagementPermission("control") directly, but you should capture that object somewhere (like ManagementPermissionHelper does) so that you can reuse it.

java.management already exports com.ibm.java.lang.management.internal to jdk.management; so I think ManagementPermissionHelper can be used directly.

pshipton · 2018-03-27T17:49:49Z

fyi #1551, similar changes are required here

chandrams · 2018-04-02T05:12:27Z

@pshipton @keithc-ca

I have made the changes as suggested, please review. Thanks.

keithc-ca · 2018-04-03T15:03:24Z

jcl/src/jdk.management/share/classes/openj9/lang/management/OpenJ9DiagnosticsMXBean.java

+
+/**
+ * <p>
+ * This interface provides APIs to dynamically trigger Dump agents. APIs are also available to


Please don't captialize 'Dump'.

keithc-ca · 2018-04-03T15:04:34Z

jcl/src/jdk.management/share/classes/openj9/lang/management/OpenJ9DiagnosticsMXBean.java

+public interface OpenJ9DiagnosticsMXBean extends PlatformManagedObject {
+	/**
+	 * Reset the JVM dump options to the settings specified when the JVM was started removing any additional
+	 * configuration done since then. This method may throw a DumpConfigurationUnavailableException if the dump 


I think this should be 'ConfigurationUnavailableException' instead of 'DumpConfigurationUnavailableException'.

keithc-ca · 2018-04-03T15:05:06Z

jcl/src/jdk.management/share/classes/openj9/lang/management/OpenJ9DiagnosticsMXBean.java

+	 * This function sets options for the dump subsystem.
+	 * The dump option is passed in as a String. Use the same syntax as the -Xdump command-line option, with the 
+	 * initial -Xdump: omitted. See Using the -Xdump option as described in the section on dump agents in the 
+	 * documentation for the OpenJ9 JVM. This method may throw a DumpConfigurationUnavailableException if the dump


DumpConfigurationUnavailableException -> ConfigurationUnavailableException

keithc-ca · 2018-04-03T15:15:11Z

...dk.management/share/classes/openj9/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+			ObjectName name = new ObjectName("openj9.lang.management:type=OpenJ9Diagnostics");  //$NON-NLS-1$
+			return name;
+		} catch (MalformedObjectNameException e) {
+			return null;


This should probably throw an Error rather than return null.

Other MXBeans such as JVMCpuMonitor, ProcessorMXBeanImpl all have similar logic and return null on MalformedObjectNameException. Hence used the same logic here. Should I change it to throw an error?

I think they all should throw an Error, but I'm fine leaving that cleanup for later.

Thanks Keith. I have made the other changes as per your review comments. Please review again.

keithc-ca · 2018-04-03T15:17:23Z

...dk.management/share/classes/openj9/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+/*[IF Sidecar19-SE]*/
+			try {
+				Object controlPermission = managementPermission.get(null);
+				manager.checkPermission((java.security.Permission)controlPermission);


Please pull the call to checkPermission() out of the try statement so it doesn't flow through handleError and is common for all Java versions.

keithc-ca · 2018-04-03T15:19:11Z

test/JLM_Tests/playlist.xml

+		</groups>
+		<subsets>
+			<subset>SE80</subset>
+			<subset>SE90</subset>


This test should apply to all Java versions: can we just remove the <subsets> element?

No, in able to support special subsets like Panama and Valhalla, we are caught in the position of explicitly stating the subset. If you do remove < subsets >, it will generate make targets for 5-6 supported subsets (SE80, SE90, SE100, SE110, Panama, Valhalla) and test compilation will fail, as Panama and Valhalla have special requirements for build. (At some point we should address this in a more satisfying way, in order not to play this game of continuously updating these... perhaps Panama & Valhalla off in a separate branch of openj9? but currently explicit statement is the way)

So, in this case, you may want to add SE100 to run this test also against Java10.

Signed-off-by: Chandrakala <chandra-ms@in.ibm.com>

keithc-ca · 2018-04-09T16:33:30Z

...dk.management/share/classes/openj9/lang/management/internal/OpenJ9DiagnosticsMXBeanImpl.java

+			dumpConfigurationUnavailableExClass = classes[2];
+			Class<?> managementPermissionHelperClass = classes[3];
+
+			managementPermission= managementPermissionHelperClass.getField("MPCONTROL"); //$NON-NLS-1$


I don't think access to ManagementPermissionHelper.MPCONTROL needs to be via reflection. That package is exported to this module so it should be able to use the class and field directly in all Java versions. That change can wait for a separate PR though.

keithc-ca · 2018-04-09T16:36:28Z

jenkins test sanity plinux

pshipton added comp:vm enhancement labels Nov 30, 2017

pshipton reviewed Dec 13, 2017

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch 4 times, most recently from ed9635c to bd2d966 Compare January 15, 2018 10:50

chandrams force-pushed the diagnosticsMXBean branch 3 times, most recently from e81da25 to 1937ab8 Compare January 22, 2018 10:12

chandrams changed the title ~~WIP Included a new MXBean to dynamically configure dump options~~ Included a new MXBean to dynamically configure dump options Jan 23, 2018

pshipton reviewed Jan 23, 2018

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch from 1937ab8 to 9213ea3 Compare January 25, 2018 09:48

pshipton requested a review from llxia January 26, 2018 17:54

chandrams mentioned this pull request Mar 9, 2018

Included openj9/lang/management package ibmruntimes/openj9-openjdk-jdk8#60

Merged

keithc-ca reviewed Mar 12, 2018

View reviewed changes

llxia reviewed Mar 12, 2018

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch from 37630c1 to 4651232 Compare March 13, 2018 18:23

llxia reviewed Mar 14, 2018

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch 4 times, most recently from 7783f1f to 4922923 Compare March 19, 2018 06:32

llxia approved these changes Mar 19, 2018

View reviewed changes

pshipton reviewed Mar 22, 2018

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch 2 times, most recently from 8ddfd06 to d1d7b6a Compare March 29, 2018 15:17

keithc-ca requested changes Apr 3, 2018

View reviewed changes

chandrams force-pushed the diagnosticsMXBean branch from d1d7b6a to 7efef6b Compare April 5, 2018 12:15

Included a new MXBean to dynamically configure dump options

54c8280

Signed-off-by: Chandrakala <chandra-ms@in.ibm.com>

chandrams force-pushed the diagnosticsMXBean branch from 7efef6b to 54c8280 Compare April 5, 2018 19:28

keithc-ca reviewed Apr 9, 2018

View reviewed changes

keithc-ca approved these changes Apr 9, 2018

View reviewed changes

keithc-ca merged commit f74f463 into eclipse-openj9:master Apr 9, 2018

This was referenced Apr 11, 2018

Removed reflection code for Java9 as the internal package is exported #1664

Merged

Update documentation for com.ibm.jvm.Dump API #953

Open

pshipton mentioned this pull request Apr 19, 2018

OpenJ9DiagnosticsMXBean and other mxbean testing is not running #1721

Open

Included a new MXBean to dynamically configure dump options #718

Included a new MXBean to dynamically configure dump options #718

Conversation

chandrams commented Nov 30, 2017

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

chandrams commented Jan 15, 2018

pshipton commented Jan 17, 2018

chandrams commented Jan 17, 2018

chandrams commented Jan 23, 2018

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

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

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

pshipton commented Jan 23, 2018

chandrams commented Mar 9, 2018

keithc-ca Mar 12, 2018 • 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

chandrams commented Mar 14, 2018

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

Choose a reason for hiding this comment

pshipton commented Mar 27, 2018

chandrams commented Apr 2, 2018

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

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

Choose a reason for hiding this comment

keithc-ca commented Apr 9, 2018

keithc-ca Mar 12, 2018 •

edited

Loading