-
Notifications
You must be signed in to change notification settings - Fork 728
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
Add criu tests to verify JITServer with SSL #17985
Conversation
f0e8b25
to
d450f87
Compare
@mpirvu Can you review this? I will then also ask Lan also to review. |
@SajinaKandy Before reviewing it, does the PR work in your private testing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that the test will capture an unsuccessful connection between client and server.
In particular I would like the following common scenario to be tested: Client starts with no SSL pre-checkpoint and a snapshot is created. During the restore, SSL options are provided and the client must successfully connect to the server.
This is the scenario that Liberty team identified as not working correctly.
<output type="failure" caseSensitive="yes" regex="no">CRIU is not enabled</output> | ||
<output type="failure" caseSensitive="yes" regex="no">Operation not permitted</output> | ||
<output type="success" caseSensitive="yes" regex="no">JITServer: JITServer Client Mode.</output> | ||
<output type="success" caseSensitive="yes" regex="no">Successfully initialized SSL context</output> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sufficient to guarantee a successful connection? In out troubleshooting guide it says that if client enables SSL but server does not the vlog will show:
#JITServer: Successfully initialized SSL context (OpenSSL 3.0.2 15 Mar 2022)
….
#JITServer: Error accepting SSL connection: errno=0
Similarly, is the certificates/keys at client/server are not matching, there will be some messages like:
#JITServer: Successfully initialized SSL context (OpenSSL 3.0.2 15 Mar 2022)
….
#FAILURE: JITServer::StreamFailure: Failed to SSL_connect for java/lang/System.getSysPropBeforePropertiesInitialized(I)Ljava/lang/String; @ cold
#JITServer: t= 10 Could not connect to a server. Next attempt in 2000 ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 2 more tests to check failure conditions
<output type="success" caseSensitive="no" regex="yes" javaUtilPattern="yes">(java|openjdk|semeru) version</output> | ||
<output type="required" caseSensitive="no" regex="no">JITServer Client Mode.</output> | ||
<output type="required" caseSensitive="no" regex="no">Successfully initialized SSL context</output> | ||
<output type="success" caseSensitive="no" regex="no">Connected to a server</output> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with the testing infra. Should we change "success" to "required" here?
The client can still show Successfully initialized SSL context
and still not connect successfully to the server. We must see both "Successfully initialized SSL context" and "Connected to a server" to guarantee a successful connection with SSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now changed the test conditions in the new change.
@dsouzai would you know when we should use "success"
vs "required"
for the test conditions?
After further discussion, I understand that the current tests for criu would pass options to the |
When trying to run the following commands outside the test framework, using the
The same will pass without crash if the certificate option is removed:
|
Does the test pass if you pass any other option instead of the SSL one?
If that |
On the JITServer
|
I have spent some time looking into the crash on
I ran another tests with the option
This doesn't crash, however on the server side I see the following errors which matches the one received during the crash at restore when run with Options File:
|
The latest code works fine without the crash with SSL options. |
40d6aea
to
600e60e
Compare
@dsouzai Can you please review my code and see if the tests have been added correctly as per the current design? |
@mpirvu I have now split the work into 2 parts. The first part is submitted now with I ran the tests on the existing code here and see the tests are running fine. The logs show:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yeah normally you shouldn't see that message. That message exists because sometimes a restore will fail for known reasons. Instead of having the test fail, the test framework just pretends it passed. That's why we have the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM; minor changes requested.
test/functional/cmdLineTests/criu/criu_jitserverPostRestore.xml
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/criu/criu_jitserverPostRestore.xml
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/criu/criu_jitserverPostRestore.xml
Outdated
Show resolved
Hide resolved
Add tests to the existing criu and jitserver tests under cmdLineTests for checking/verifying SSL connections with JITServer. Closes: #eclipse-openj9#17967 Signed-off-by:SajinaKandy <[email protected]>
600e60e
to
48c4fc4
Compare
@mpirvu I have now made all the requested changes. |
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk17 |
The build failure in Z shows:
Looks to be related to #17382. |
jenkins test sanity zlinuxjit jdk17 |
Add tests to the existing jitserver tests under cmdLineTest for checking /verifying SSL connections with JITServer. This is part 2 for the work done in eclipse-openj9#17985 . Closes: #eclipse-openj9#17967 Signed-off-by: SajinaKandy <[email protected]>
Add tests to the existing jitserver tests under cmdLineTest for checking /verifying SSL connections with JITServer. This is part 2 for the work done in eclipse-openj9#17985 . Closes: #eclipse-openj9#17967 Signed-off-by: SajinaKandy <[email protected]>
Add tests to the existing
criu
andjitserver
tests undercmdLineTest
for checking/verifying SSL connections with JITServer.Closes: ##17967