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

Modifications to make JVM Agent work on IBM JVM 8. #343

Merged
merged 1 commit into from Sep 25, 2017
Merged

Conversation

@danpfe
Copy link
Contributor

@danpfe danpfe commented Sep 6, 2017

Mods to make JVM Agent work on the IBM JVM 8. Hopefully this is useful for those that happen to run IBM's JDK and want to use the Jolokia JVM Agent.

@rhuss
Copy link
Owner

@rhuss rhuss commented Sep 10, 2017

Thanks a lot ! looks good on a first sight. Unfortunately I'm a bit busy these days, but I'm going to review and integrate this PR asap. Please ping me, if I don't come back to this within the next two weeks.

thanks again ...

Copy link
Owner

@rhuss rhuss left a comment

Sorry for the delay. Looks good to me in general, some minor remarks and questions. Especially I'm curious if we could get the tests running ?

Is it easy to test an IBM JVM on OS X ? If so I would try also. I'm especially curious whether live attaching of the agent works smoothly.

}

try {
final Method selfCertMethod = keypair.getClass().getDeclaredMethod("getSelfCertificate", clazz, Date.class, long.class);

This comment has been minimized.

@rhuss

rhuss Sep 15, 2017
Owner

I wonder, whether the original ClassUtil.applyMethod() wouldn't work here as well ? The incoming x500Name object already would be already of the proper IBM / Oracle type and applyMethod() should work in both cases. Have you tried it and got some error ?

This comment has been minimized.

@danpfe

danpfe Sep 22, 2017
Author Contributor

Yes, it fails because the applyMethod would search for a method-signature that does not exist (if I remember correctly right now it'd say that there is no method that takes Object, Date, long). Of course it would be possible to modify applyMethod but at that moment I thought it's less invasive in your code to simply do it this way.

if (keyGenClass == null) {
keyGenClass = ClassUtil.classForName(KEYGEN_CLASS_JDK7);
final Class keyGenClass;
if (ClassUtil.checkForClass(KEYGEN_CLASS_JDK8_SUN)) {

This comment has been minimized.

@rhuss

rhuss Sep 15, 2017
Owner

Since classForName already return null when the class could not be found we could save all the calls to checkForClass(): Just fetch the class and if null, try next. This could be even moved out into an extra method which makes this in a loop and returns as soon as the first has been found.

This comment has been minimized.

@danpfe

danpfe Sep 22, 2017
Author Contributor

Yes, that wouldn't be a problem, at the moment I wrote the code it just didn't look very pleasing to my eye to see if this null, if that null, etc :) I can change it if you like.

@@ -56,6 +56,11 @@ public void emptyPid() {

@Test
public void listAndAttach() throws Exception, NoSuchMethodException, IllegalAccessException {
// FIXME Fails on the IBM JVM; however, the agent seems to work. Skip for now just to get the tests to run on the IBM JVM. - Anyone?

This comment has been minimized.

@rhuss

rhuss Sep 15, 2017
Owner

Any idea why and where this fails ?

This comment has been minimized.

@danpfe

danpfe Sep 23, 2017
Author Contributor

Where it fails? At the second assert. Why it fails I am honestly not sure about (as I myself very very rarely use J9). However, my theory here is that we are running into the same "lack of attach-mode"-problem as with JMockit. I know the class exists otherwise it would fail earlier and when I tried to find a reason for this I did a fair bit of googling and it seems that the general consensus in that respect is that you simply can't attach to J9 the same way you can with OpenJDK/Oracle JDK. As I wrote in the comment, Jolokia itself seems to work just fine after a fair amount of manual testing but it's naturally a bit disturbing to have to disable a test. Checking again if I can uncover the root cause.

This comment has been minimized.

@danpfe

danpfe Sep 23, 2017
Author Contributor

Not much for my theory it seemed. "attach" worked fine, "detach" failed with IllegalAccessException, sent another commit your way. Think all your requested changes are resolved now.

try {
sc = SSLContext.getInstance(protocol);
} catch (NoSuchAlgorithmException e) {
// on the IBM JVM v8 we get an exception on SSLv3 which fails the entire test, safe-guarding for it.

This comment has been minimized.

@rhuss

rhuss Sep 15, 2017
Owner

Any idea why this fails for IBM ? If this is inevitable I would check also for the IBM VM and only continue if its on IBM (and fail like before for Oracle).

This comment has been minimized.

@danpfe

danpfe Sep 23, 2017
Author Contributor

Yeah, now that I look at it again I believe this should be changed (partially because the reason given was what I thought in the beginning to be the reason but later forgot to change it before sending the pull request). From what I found out SSLv3 is simply disabled, which is a bit confusing because the list of cipher suites all start with SSL_, but what's TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA on OpenJDK is SSL_ECDHE_RSA_WITH_AES_128_CBC_SHA on IBM.

<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<forkMode>always</forkMode>
<argLine>-javaagent:${settings.localRepository}/org/jmockit/jmockit/1.8/jmockit-1.8.jar</argLine>

This comment has been minimized.

@rhuss

rhuss Sep 15, 2017
Owner

would be cool when if we could reuse the same jmockit version as declared in the dependency. Maybe extract it into a property which then is referenced here and within the dependency ?

This comment has been minimized.

@danpfe

danpfe Sep 23, 2017
Author Contributor

Correct, my bad. I intended to change it before committing but this bad boy slipped through :)

@rhuss rhuss added this to the 1.3.4 milestone Sep 23, 2017
@rhuss
rhuss approved these changes Sep 23, 2017
Copy link
Owner

@rhuss rhuss left a comment

LGTM, thanks a lot !

A final request, though: Could you please squash the commits and add a line to changes.xml for the next version ?

Thanks a lot again ...

@rhuss
Copy link
Owner

@rhuss rhuss commented Sep 23, 2017

seems to be a minor merge conflict. would be also awesome if you could resolve this by rebasing to the current master.

Thanks!

@danpfe
Copy link
Contributor Author

@danpfe danpfe commented Sep 25, 2017

Sure, will fix tonight.

Signed-off-by: Daniel Pfeifer <daniel@pfeifer.io>
@danpfe danpfe force-pushed the danpfe:master branch from 13dcae4 to ac2d30e Sep 25, 2017
@danpfe
Copy link
Contributor Author

@danpfe danpfe commented Sep 25, 2017

Rebased, fixed conflict, squashed commits, pushed. Hope this is useful for those few of us that from time to time are on J9.

@rhuss
Copy link
Owner

@rhuss rhuss commented Sep 25, 2017

Thanks a lot ! 'hope I can get a version out soon, but very likely not this week anymore.

@rhuss rhuss merged commit c1eb663 into rhuss:master Sep 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.