-
Notifications
You must be signed in to change notification settings - Fork 729
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
Fix SSL Test Failure due to client exiting too soon during remote compile #18393
Conversation
@mpirvu Could you review the changes. |
The additional output lines will be printed as below: When the verbose log contains the string:
When the verbose log does not contains the string:
|
If the server has a mismatching key, then it will initialize the context, but the connection still fails. |
@mpirvu If you are refering about the test case where Server initializes its SSL context with whatever keys and then a client tries to connect with a mis-matched key. Then in that test case, the connection to the server will fail. And hence no compilations will be performed at the server for the client.
Didn't understand the objective of this. So currently the SSL test cases(and the existing tests) only check for connection success or failure as expected based on the scenario we are testing. |
What happens in the following scenario:
Is this test passing because it saw the server initializing the context? |
Without a compilation attempt we cannot establish if the client can successfully connect to the server. |
In the original test failure we see the following:
So, the client connected to the server ( |
I ran some local tests:
So, one way to improve the test is to watch for one of these earlier messages at the client. This reduces the possibility of a test failing due to wrong reason but does not eliminate it: if the client does not have time to issue a compilation, there is no way to check whether the connection would be successful or not. Using a small count (e.g. count=1) increases the chances of the client issuing a compilation. Moreover, if the test sees that no compilation was issued by the client (those lines with "compiling"), then we should not fail the test because it was inconclusive. |
@mpirvu Thanks for the explanation. I agree to what you are saying. During a successful SSL connection with correct certificates the client prints:
With mis-matched certificates:
Hence in the existing tests we check for the string As an addition we can introduce a |
Currently the tests are failing for the success scenario where the certificates are matching and the connection is expected to be successful. The client side relevant logs during connection are:
At the server we can see:
So accordingly do you think its good to check for |
I don't this so. This option makes all synchronous compilations remote, but in the small test case you are using you should not see synchronous compilations. |
No. That message comes even later than the moment the connection is established. The example you have earlier in comment #18393 (comment) should not have failed because the client has issued: "Connected to a server". |
This is correct. The logs are from a successful connection case. I was trying to see what string I should use to make the test case
And convert the following from
If we change the code like above, then in that case we establish a successful connection with matching SSL certificates(on both sides) only when we match the string I think the |
I disagree. |
e49b7a5
to
8eef9d3
Compare
@mpirvu I have changed the fix now. Please review the changes. |
It may be better to add |
Due to time delays between SSL context initialization at the server and and the client program exiting too soon, the SSL tests were failing. The failure happens due to: Required condition was not found: [Output match: Connected to a server] As the client did not connect to the server as expected. This fix reads the jitserver verbose log to confirm if the SSL context is initialized before starting the client. Closes: #eclipse-openj9#18308 Signed-off-by: SajinaKandy <[email protected]>
8eef9d3
to
5f6edc8
Compare
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
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk17 |
jenkins test sanity plinux,xlinux,zlinux jdk17 |
There are multiple failures due to different issues:
Issue: Raised infra issue #18418
Issue: #18144
Issue: #17844 (comment)
Issue: #18399
Issue: #18399
Issue: #18384 |
@SajinaKandy Is there anything that could have been caused by this PR? I don't see any connection so far. |
@mpirvu The existing test failures I posted above are unrelated to the changes here. |
@mpirvu The last So we can conclude none of these failures are happening due to the changes introduced in the SSL tests here. |
Merging the PR based on the comments above |
Due to time delays between SSL context initialization at the server and and the client program exiting too soon, the SSL tests were failing. The failure happens due to:
Required condition was not found: [Output match: Connected to a server] As the client did not connect to the server as expected.
This fix will check the condition for the connection established with the server and compile synchronously.
Closes: ##18308