Fix session cleanup test #126966

jportner · 2022-03-04T21:40:08Z

This PR fixes the failing test in #121482.

The test asserts that no sessions exist in the index after the cleanup routine is called, but we made a recent change to how we delete sessions (#122419) that no longer forces an index refresh.

Now, we always attempt to refresh the index after deleting expired sessions.

I used release_note:skip since this is not a bug fix, it's just a test fix.
I plan to backport this to all affected branches.

Flaky test run x50 ✅: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/219

thomheymann

Looks good. Couple comments below.

thomheymann · 2022-03-07T18:20:13Z

x-pack/plugins/security/server/session_management/session_index.ts

+    if (haveDeletedSessions) {
+      try {
+        await elasticsearchClient.indices.refresh({ index: this.indexName });
+        logger.debug(`Refreshed session index.`);
+      } catch (err) {
+        logger.error(`Failed to refresh session index: ${err.message}`);
+      }
+    }


nit: Maybe put this block in a finally statement if you want it executed irregardless of outcome? Thinks it's cleaner and you don't need to assign the thrown error.

The old code re-threw the error in the catch. If we just tacked on a finally, it wouldn't get executed in that case, would it?

thomheymann · 2022-03-07T18:27:52Z

x-pack/plugins/security/server/session_management/session_index.ts

          operations.push({ delete: { _id } });
        });
        if (operations.length > 0) {
-          const bulkResponse = await this.options.elasticsearchClient.bulk(
+          haveDeletedSessions = true;


Technically speaking we haven't actually deleted anything at this point. Is it worth being stricter here and only marking sessions as having been touched after the bulk delete operation?

I probably could have clarified this in the comments:

My reasoning for doing it here is that I'm not 100% sure if a failed bulk operation could have resulted in some of the sessions getting deleted. I figured it's safest to trigger an index refresh anyway, but I'm not sure how much of a practical impact this has tbh. I suppose haveAttemptedToDeleteSessions would have been a more accurate variable name but that sounds like a mouthful 😅

Changed here: f4be021

Let me know what you think!

…ssion-cleanup-test

Also added comments.

thomheymann

LGTM! 🥳

kibana-ci · 2022-03-07T23:00:51Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #27821 succeeded 40b7cbf

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

kibanamachine · 2022-03-07T23:03:59Z

💔 All backports failed

Status	Branch	Result
❌	8.1	Backport failed because of merge conflicts
❌	8.0	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 126966

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit 2f83c65) # Conflicts: # x-pack/plugins/security/server/session_management/session_index.ts

…ed-unexpectedly-error * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/core.ts # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

…re-browser-errors * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

Joe Portner added 2 commits March 4, 2022 16:27

Minor refactor

2336649

Refresh session index after cleanup

40b7cbf

jportner added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 v8.1.1 v8.0.2 labels Mar 4, 2022

jportner requested a review from a team as a code owner March 4, 2022 21:40

jportner requested review from azasypkin and removed request for azasypkin March 4, 2022 21:41

thomheymann reviewed Mar 7, 2022

View reviewed changes

Joe Portner added 2 commits March 7, 2022 16:24

Merge remote-tracking branch 'upstream/main' into issue-121482-fix-se…

385429e

…ssion-cleanup-test

Tweak session index refresh condition

f4be021

Also added comments.

thomheymann approved these changes Mar 7, 2022

View reviewed changes

jportner enabled auto-merge (squash) March 7, 2022 22:03

jportner merged commit 2f83c65 into elastic:main Mar 7, 2022

jportner deleted the issue-121482-fix-session-cleanup-test branch March 7, 2022 23:35

jportner mentioned this pull request Mar 7, 2022

[8.1] Fix session cleanup test (#126966) #127086

Merged

jportner added a commit to jportner/kibana that referenced this pull request Mar 7, 2022

Fix session cleanup test (elastic#126966)

c0705f2

(cherry picked from commit 2f83c65) # Conflicts: # x-pack/plugins/security/server/session_management/session_index.ts

jportner mentioned this pull request Mar 7, 2022

[8.0] Fix session cleanup test (#126966) #127087

Merged

jportner added a commit to jportner/kibana that referenced this pull request Mar 7, 2022

Fix session cleanup test (elastic#126966)

d89b92f

(cherry picked from commit 2f83c65) # Conflicts: # x-pack/plugins/security/server/session_management/session_index.ts

jportner added a commit that referenced this pull request Mar 8, 2022

Fix session cleanup test (#126966) (#127087)

066d6a2

(cherry picked from commit 2f83c65) # Conflicts: # x-pack/plugins/security/server/session_management/session_index.ts

jportner added a commit that referenced this pull request Mar 8, 2022

Fix session cleanup test (#126966) (#127086)

05d90e7

(cherry picked from commit 2f83c65) # Conflicts: # x-pack/plugins/security/server/session_management/session_index.ts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session cleanup test #126966

Fix session cleanup test #126966

jportner commented Mar 4, 2022 •

edited

Loading

thomheymann left a comment

thomheymann Mar 7, 2022

jportner Mar 7, 2022 •

edited

Loading

thomheymann Mar 7, 2022

jportner Mar 7, 2022

jportner Mar 7, 2022

thomheymann left a comment

kibana-ci commented Mar 7, 2022

kibanamachine commented Mar 7, 2022

Fix session cleanup test #126966

Fix session cleanup test #126966

Conversation

jportner commented Mar 4, 2022 • edited Loading

thomheymann left a comment

Choose a reason for hiding this comment

thomheymann Mar 7, 2022

Choose a reason for hiding this comment

jportner Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

thomheymann Mar 7, 2022

Choose a reason for hiding this comment

jportner Mar 7, 2022

Choose a reason for hiding this comment

jportner Mar 7, 2022

Choose a reason for hiding this comment

thomheymann left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 7, 2022

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Mar 7, 2022

💔 All backports failed

Manual backport

Questions ?

jportner commented Mar 4, 2022 •

edited

Loading

jportner Mar 7, 2022 •

edited

Loading