Skip to content

Conversation

@ywangd
Copy link
Member

@ywangd ywangd commented Jul 6, 2020

This PR adds error catching when filling up the queue of the crypto thread pool.

Test testAuthenticationReturns429WhenThreadPoolIsSaturated has seen failure on CI when it tries to push 1000 tasks into the queue (setup phase). Since multiple tests share the same internal test cluster, it may be possible that there are lingering requests not fully cleared out from the queue. When it happens, we will not be able to push all 1000 tasks into the queue. But since what we need is just queue saturation, so as long as we can be sure that the queue is fully filled, it is safe to ignore rejection error and just move on.

A number of 1000 tasks also take some to clear out, which could cause the test suite to time out. This PR change the queue to 10 so the tests would have better chance to complete in time.

Build scan:

@ywangd ywangd added >test Issues or PRs that are addressing/adding tests :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.9.0 labels Jul 6, 2020
@ywangd ywangd requested a review from tvernum July 6, 2020 14:18
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Authentication)

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jul 6, 2020
@ywangd
Copy link
Member Author

ywangd commented Jul 7, 2020

@tvernum I came to realise that we can configure the queue size to be smaller, e.g. 10. With the smaller queue, things should complete relatively soon. So I added what we have discussed about "waiting for the future of the last task to complete at the end of the 429 test" so that it will not interfere with other tests.

@ywangd ywangd requested a review from albertzaharovits July 7, 2020 10:56
@ywangd ywangd merged commit ec6f79f into elastic:master Jul 7, 2020
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 7, 2020
Adds error handling when filling up the queue of the crypto thread pool. Also reduce queue size of the crypto thread pool to 10 so that the queue can be cleared out in time.

Test testAuthenticationReturns429WhenThreadPoolIsSaturated has seen failure on CI when it tries to push 1000 tasks into the queue (setup phase). Since multiple tests share the same internal test cluster, it may be possible that there are lingering requests not fully cleared out from the queue. When it happens, we will not be able to push all 1000 tasks into the queue. But since what we need is just queue saturation, so as long as we can be sure that the queue is fully filled, it is safe to ignore rejection error and just move on.

A number of 1000 tasks also take some to clear out, which could cause the test suite to time out. This PR change the queue to 10 so the tests would have better chance to complete in time.
ywangd added a commit that referenced this pull request Jul 7, 2020
Adds error handling when filling up the queue of the crypto thread pool. Also reduce queue size of the crypto thread pool to 10 so that the queue can be cleared out in time.

Test testAuthenticationReturns429WhenThreadPoolIsSaturated has seen failure on CI when it tries to push 1000 tasks into the queue (setup phase). Since multiple tests share the same internal test cluster, it may be possible that there are lingering requests not fully cleared out from the queue. When it happens, we will not be able to push all 1000 tasks into the queue. But since what we need is just queue saturation, so as long as we can be sure that the queue is fully filled, it is safe to ignore rejection error and just move on.

A number of 1000 tasks also take some to clear out, which could cause the test suite to time out. This PR change the queue to 10 so the tests would have better chance to complete in time.
@cbuescher
Copy link
Member

@ywangd these still seem to time out now, see https://gradle-enterprise.elastic.co/s/4gfed632tmtcc/tests/:x-pack:plugin:security:test/org.elasticsearch.xpack.security.authc.ApiKeyIntegTests/testAuthenticationReturns429WhenThreadPoolIsSaturated#1
That should be after you merged the backport into 7.x if I read the commit log right. Let me know if this looks related and we should reopen this or another issue?

@cbuescher
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v7.9.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants