[Security Solution] remove metadata integ test explicit timeouts #125742

joeypoon · 2022-02-16T00:24:07Z

Summary

Replace explicit timeouts from endpoint metadata integration tests with waitFors as mentioned in #125675 (comment).

For maintainers

This was checked for breaking API changes and was labeled appropriately

joeypoon · 2022-02-16T00:25:02Z

flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/159

joeypoon · 2022-02-16T00:27:23Z

x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts


  describe('test metadata apis', () => {
+    before(async () => {
+      await endpointTestResources.setMetadataTransformFrequency('1s');


ideally we'd set the united transform to 1s as well but as of 8.0.0, we get 403s when trying to edit the united transform. likely related to fleet permission changes since it utilizes the fleet-agents index.

joeypoon · 2022-02-16T00:31:08Z

x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts

          await Promise.all([
-            bulkIndex(getService, AGENTS_INDEX, generateAgentDocs(currentTime, policyId)),
+            bulkIndex(getService, AGENTS_INDEX, agentDocs),
            bulkIndex(getService, METADATA_DATASTREAM, generateMetadataDocs(currentTime)),


in the future, it'd be nice to use the data generator but since we're testing field filtering in these tests and the generator doesn't support override fields yet (some fields on the generated are read only too), it's not a straight forward replace. will consider for a separate PR.

elasticmachine · 2022-02-16T03:11:48Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

paul-tavares

Thanks Joey for making this change. Everytime I see setTimeout() I always start thinking about how we can use more reliable methods than just arbitruary timeout values, so this change should really make the tests much more reliable and run a little faster.

I left some comments that I would like to hear back on - but as it stands, it also looks good to merge if you like. Do you remember if the prior skipped test were done also on 8.1? You have this tagged only for 8.2, but I'm wondering if it should be backported to 8.1 as well.

paul-tavares · 2022-02-16T13:28:14Z

x-pack/test/security_solution_endpoint/services/endpoint.ts

-          body,
-          rest_total_hits_as_int: true,
-        });
+    await this.waitForIndex(ids, metadataCurrentIndexPattern, body, timeout);


I'm actually thinking maybe this needs to also be the united index. The goal of htis method (waitForEndpoints()) is to wait until we are sure data exists to be shown on the Endpoint LIst (or returned on the metadata api), so I think (correct me if I'm wrong) the United target index is the one that supports that api, right?

yeah, it definitely makes sense to wait for the united index. the issue I ran into here is that depending on the timing of the transform check point, it's possible that the united index doesn't get populated at all. I played around this in the test by only starting the united transform after verifying the metadata_current index was populated.

waitForEndpoints() startTransform(UNITED_TRANSFORM) waitForUnitedEndpoints()

I considered using a future timestamp for the metadata doc(s) + only checking for united index. while it works, it feels less reliable and similar to a timeout. what I think we can do in a follow up is update loadEndpointData to do:

stop united transform generate + index docs wait for metadata_current start united transform wait for metadata_united

I'm thinking we can also add an optional generateAndIndexDocs param for custom data so we can handle situations like the integration tests in this PR.

paul-tavares · 2022-02-16T13:29:48Z

x-pack/test/security_solution_endpoint/services/endpoint.ts

        }
+      : {
+          query: {
+            match_all: {},


Wondering: in this case, maybe in order to speed the search up, we should limit the result to 1 only? we're just trying to ensure data exists right? (maybe do that change above too on line 197).

that makes sense

paul-tavares · 2022-02-16T13:31:17Z

x-pack/test/security_solution_endpoint/services/endpoint.ts

-        // We ignore 404's (index might not exist)
-        if (error instanceof errors.ResponseError && error.statusCode === 404) {
-          return false;
+  async waitForUnitedEndpoints(ids: string[] = [], timeout = this.config.get('timeouts.waitFor')) {


If my comment above makes sense to you (about actualyl checking the united index rather than the other one), then maybe we don't need this method?

If you do keep it, please add JSDOCs that describe what this method does nad how its different from the one above.

put a long winded response above but I definitely forgot to update the comments. will do that.

paul-tavares · 2022-02-16T13:31:58Z

x-pack/test/security_solution_endpoint/services/endpoint.ts

@@ -137,13 +139,45 @@ export class EndpointTestResources extends FtrService {
    return deleteIndexedHostsAndAlerts(this.esClient as Client, this.kbnClient, indexedData);
  }

+  private async waitForIndex(


Nice. this is a good generic method that can be used to check for data in any index

paul-tavares · 2022-02-16T15:20:12Z

@joeypoon

from: #125127 (comment)

I'm wondering if when we call the "setMetadataTransformFrequency()` we actually should apply the new frequency across all of our transforms and not just one.

cc/ @kevinlog

joeypoon · 2022-02-16T16:26:25Z

@joeypoon

from: #125127 (comment)

I'm wondering if when we call the "setMetadataTransformFrequency()` we actually should apply the new frequency across all of our transforms and not just one.

cc/ @kevinlog

for sure. I left a comment way above so it's a bit buried but in 8.0.0 there's some fleet permission issues that prevent updating the united transform which is why I wasn't able to also change the united transform frequency.

also just wrapped up transform frequency performance testing so we might just end up dropping both frequencies down to 1s.

kibana-ci · 2022-02-16T18:58:29Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #1 / Execution context Browser apps dashboard app propagates context for Vertical bar visualization

Metrics [docs]

✅ unchanged

History

💛 Build #24491 was flaky a0f069dfe007fda56931a704dfaa19cd3a9d8bc7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…stic#125742) (cherry picked from commit ecefde8)

kibanamachine · 2022-02-16T20:51:54Z

💚 All backports created successfully

Status	Branch	Result
✅	8.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…5742) (#125871) (cherry picked from commit ecefde8) Co-authored-by: Joey F. Poon <joey.poon@elastic.co>

joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 labels Feb 16, 2022

joeypoon commented Feb 16, 2022

View reviewed changes

joeypoon marked this pull request as ready for review February 16, 2022 03:11

joeypoon requested review from a team as code owners February 16, 2022 03:11

joeypoon requested review from ashokaditya and parkiino February 16, 2022 03:11

joeypoon requested a review from paul-tavares February 16, 2022 03:12

paul-tavares approved these changes Feb 16, 2022

View reviewed changes

joeypoon added the v8.1.0 label Feb 16, 2022

[Security Solution] remove metadata integ test explicit timeouts

32a0dc2

joeypoon force-pushed the chore/remove-metadata-int-test-timeouts branch from a0f069d to 32a0dc2 Compare February 16, 2022 16:35

joeypoon merged commit ecefde8 into elastic:main Feb 16, 2022

joeypoon deleted the chore/remove-metadata-int-test-timeouts branch February 16, 2022 20:48

kibanamachine mentioned this pull request Feb 16, 2022

[8.1] [Security Solution] remove metadata integ test explicit timeouts (#125742) #125871

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 16, 2022

[Security Solution] remove metadata integ test explicit timeouts (ela…

c32c8ff

…stic#125742) (cherry picked from commit ecefde8)

kibanamachine added a commit that referenced this pull request Feb 16, 2022

[Security Solution] remove metadata integ test explicit timeouts (#12…

fc6d6e3

…5742) (#125871) (cherry picked from commit ecefde8) Co-authored-by: Joey F. Poon <joey.poon@elastic.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] remove metadata integ test explicit timeouts #125742

[Security Solution] remove metadata integ test explicit timeouts #125742

joeypoon commented Feb 16, 2022

joeypoon commented Feb 16, 2022

joeypoon Feb 16, 2022

joeypoon Feb 16, 2022 •

edited

Loading

elasticmachine commented Feb 16, 2022

paul-tavares left a comment

paul-tavares Feb 16, 2022

joeypoon Feb 16, 2022

paul-tavares Feb 16, 2022

joeypoon Feb 16, 2022

paul-tavares Feb 16, 2022

joeypoon Feb 16, 2022

paul-tavares Feb 16, 2022

paul-tavares commented Feb 16, 2022

joeypoon commented Feb 16, 2022 •

edited

Loading

kibana-ci commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

[Security Solution] remove metadata integ test explicit timeouts #125742

[Security Solution] remove metadata integ test explicit timeouts #125742

Conversation

joeypoon commented Feb 16, 2022

Summary

For maintainers

joeypoon commented Feb 16, 2022

Choose a reason for hiding this comment

joeypoon Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 16, 2022

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares commented Feb 16, 2022

joeypoon commented Feb 16, 2022 • edited Loading

kibana-ci commented Feb 16, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

History

kibanamachine commented Feb 16, 2022

💚 All backports created successfully

Questions ?

joeypoon Feb 16, 2022 •

edited

Loading

joeypoon commented Feb 16, 2022 •

edited

Loading