Speeding up tests

[peternied]

Description

Sleep statements should rarely be used, instead apis should allow for
blocking/non-blocking calling patterns. Attempting to remove and clean
up these statements to speed up PR workflows.

Github Action workflows for this repo were 75+ minutes, with this change 40 minutes. 47% reduction in time.

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov-commenter]

Codecov Report

Merging #1715 (97c1bae) into main (51e492c) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1715   +/-   ##
=========================================
  Coverage     62.86%   62.87%           
- Complexity     3262     3264    +2     
=========================================
  Files           253      253           
  Lines         18089    18096    +7     
  Branches       3243     3246    +3     
=========================================
+ Hits          11372    11377    +5     
- Misses         5063     5064    +1     
- Partials       1654     1655    +1     

Impacted Files	Coverage Δ
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	56.48% <0.00%> (-1.53%)	⬇️
...search/security/configuration/DlsFlsValveImpl.java	59.25% <0.00%> (+1.08%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51e492c...97c1bae. Read the comment docs.

[cliu123]

The sleep was introduced by the removal of TransportClient PR. The intent is to make sure all configurations are loaded into security index, which used to be done as following. Sleep is not the best idea and doesn't really ensure that. Is there a better/faster way to ensure the existense of the configurations?

            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type, "config")).actionGet().isExists());
            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"internalusers")).actionGet().isExists());
            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"roles")).actionGet().isExists());
            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"rolesmapping")).actionGet().isExists());
            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"actiongroups")).actionGet().isExists());
            Assert.assertFalse(tc.get(new GetRequest(".opendistro_security", type,"rolesmapping_xcvdnghtu165759i99465")).actionGet().isExists());
            Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"config")).actionGet().isExists());
            if (indexRequests.stream().anyMatch(i -> CType.NODESDN.toLCString().equals(i.id()))) {
                Assert.assertTrue(tc.get(new GetRequest(".opendistro_security", type,"nodesdn")).actionGet().isExists());

[cliu123]

Long running CI is a huge pain of security plugin. This is a nice area to improve! Great contribution! 👍

[peternied]

First when I noticed and removed this sleep I had the same concern about stability. In the test setup OpenSearch nodes are created on a couple of threads, these communicate via http. From what I've seen when our tests cases are running, we are making calls via http which is out of process communication.

When moving across the process boundary layer to the network the jvm stops processing work on the test thread and can resume work on any other busy node threads. This boundary layer means a busy cpu operation is very likely to complete on the nodes within that same process before the network connection of the test's inbound request is processed.

This is not reliable protection on a production system; however, this seems to be working well with our JVM selections and our different environments. We might notice issues in the future - I imagine certain classes of tests might need additional checks, but we can deal with them as they present themselves.

Is there a better/faster way to ensure the existence of the configurations?

In OpenSearch there is a task api that can be used to track work that is in progress or complete. If we needed additional assurance that the configuration was complete, I would advocate for a way to get the task id associated with this so we can use GET _tasks/<task_id>?wait_for_completion=true to ensure that work has finished making sure the system is stable.

Alternatively we could add into those nodes that would give us an in-process signaling channel such as Object.wait(...) with a plugin that is designed to make this information available.