-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Lock Manager] Handle alias resolution when checking lock index mappings #244559
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
[Lock Manager] Handle alias resolution when checking lock index mappings #244559
Conversation
src/platform/packages/shared/kbn-lock-manager/src/setup_lock_manager_index.ts
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-lock-manager/src/setup_lock_manager_index.ts
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| describe('when lock index is accessed via alias (simulating reindexed scenario)', () => { | ||
| const reindexedIndexName = `${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10`; |
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.
Thanks for covering this scenario!
| // This should NOT throw "Cannot destructure property 'mappings' of 'res[LOCKS_CONCRETE_INDEX_NAME]' as it is undefined" | ||
| try { | ||
| await setupLockManagerIndex(es, logger); | ||
| } catch (error) { | ||
| expect().fail( | ||
| `setupLockManagerIndex should not throw when index is accessed via alias, but got: ${error.message}` | ||
| ); | ||
| } |
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.
I don't understand this test. Do you expect it to throw or not?
The comment seems to say that it should not throw, but the code assumes that it throws because the expectation is in the catch block
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.
Ah, I never used expect().fail() but I suppose that fails the test if it reaches the catch block
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.
btw. if the index ${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10 exists, and setupLockManagerIndex is called, will that create a new index called ${LOCKS_INDEX_ALIAS}-000001?
Based on this I think it will:
kibana/src/platform/packages/shared/kbn-lock-manager/src/setup_lock_manager_index.ts
Line 92 in 75d918c
| await esClient.indices.create({ index: LOCKS_CONCRETE_INDEX_NAME }); |
Can you double check if that's the case? Because that could be problematic
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.
Ah, I never used expect().fail() but I suppose that fails the test if it reaches the catch block
Yeah.. it's kinda like re-throwing an exception and enriching it with more context. Happy to remove and just let fail if that's not our common practice.
btw. if the index ${LOCKS_CONCRETE_INDEX_NAME}-reindexed-for-10 exists, and setupLockManagerIndex is run, will that create a new index called ${LOCKS_INDEX_ALIAS}-000001?
About this, you're good to question this and it's what I've noticed when running the integration tests (hence why I left the PR still in draft).
After resolving the issue with mappings, I found the integ tests still to fail with invalid_index_name_exception:
Error: setupLockManagerIndex should not throw when index is accessed via alias, but got: invalid_index_name_exception
Root causes:
invalid_index_name_exception: Invalid index name [.kibana_locks-000001], already exists as aliasI guess this happens within the migration tooling during index migration (aliasing on the concrete index name) and will affect the index creation as you mention.
I got around this locally by catching the exception and skipping creation similarly to resource_already_exists_exception, as a usable alias already exists? Does that workaround sound good to you?
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.
Happy to remove and just let fail if that's not our common practice.
No, I like it 👍
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.
I got around this locally by catching the exception and skipping creation similarly to resource_already_exists_exception, as a usable alias already exists? Does that workaround sound good to you?
Yes, I think if an alias exists and it points to a concrete index we should handle (swallow) the error, and skip error creation
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.
Great, thanks for confirming - I've added that change already (might just get lost in all the auto-generated moon changes 🌔 )
…anager_index.ts Co-authored-by: Søren Louv-Jansen <[email protected]>
…anager_index.ts Co-authored-by: Søren Louv-Jansen <[email protected]>
|
Pinging @elastic/obs-ai-team (Team:obs-ai) |
| // Handle the case where the index name already exists as an alias (e.g., after a reindex operation during cluster upgrade) | ||
| const isIndexNameExistsAsAliasError = | ||
| error instanceof errors.ResponseError && | ||
| error.body.error.type === 'invalid_index_name_exception'; | ||
|
|
||
| if (isIndexAlreadyExistsError || isIndexNameExistsAsAliasError) { |
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.
@sorenlouv I applied the changes I suggested for what you mentioned in this comment. Thought it's easier to iterate on it if it's seen in the works (and I no longer see the integ test errors). Let me know what you think!
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.
Does the existing integration test cover this? Does it fail without this change? (it should)
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.
Does it fail without this change? (it should)
Yes, confirmed it fails with:
2) Stateful Observability - Deployment-agnostic AI Assistant API integration tests
observability AI Assistant
LockManager
index assets
when lock index is accessed via alias (simulating reindexed scenario)
should successfully acquire a lock when index is accessed via alias:
ResponseError: invalid_index_name_exception
Root causes:
invalid_index_name_exception: Invalid index name [.kibana_locks-000001], already exists as aliasThere 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.
btw. not needed now, but I wonder if ensureTemplatesAndIndexCreated should create the alias .kibana_locks pointing to .kibana_locks-000001.
And if the alias (or index) already exists we do nothing.
But we'd have to consider if this is fully backwards compatible.
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.
Sounds like a good, future-proof, suggestion, but because of the backwards compatibility considerations and my (just acquired) familiarity with the lock manager, I'd suggest to leave it out of this PR and track as a follow up?
If you're feeling strongly, happy to figure out, but may need more heads as I'm rotating out from SDH and we'll be focusing on AB offsite next week.
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.
No, I agree. Let's leave that out
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
614071f to
b95fe4a
Compare
sorenlouv
left a comment
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.
Thank you so much! Great improvement
|
Starting backport for target branches: 8.19, 9.1, 9.2 |
💚 Build Succeeded
Metrics [docs]
History
|
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
…ngs (elastic#244559) Relates to: elastic/sdh-kibana#5923 ## Summary - Handle Lock Manager's alias resolution during migration: - when checking lock index mappings - in cases when concrete index creation is attempted, but the name is already used as alias ## Testing New unit tests: ```bash yarn test:jest src/platform/packages/shared/kbn-lock-manager/src/setup_lock_manager_index.test.ts ``` Updated integration tests: ```bash node scripts/functional_tests_server --config x-pack/solutions/observability/test/api_integration_deployment_agnostic/configs/stateful/oblt.ai_assistant.stateful.config.ts # Then run in a separate terminal: node scripts/functional_test_runner --config x-pack/solutions/observability/test/api_integration_deployment_agnostic/configs/stateful/oblt.ai_assistant.stateful.config.ts --grep="LockManager" ``` ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### Identify risks - This PR aims to mitigate risks that occur during migration. The implementation makes certain assumptions on the way migration tooling functions. In the unlikely events of that changing, the logic may need to be revisited ### Reviewer notes - AI assisted coding with Cursor and Claude 4.5 Opus: - Plan generation (to help cover all relevant places - Unit test generation --------- Co-authored-by: Søren Louv-Jansen <[email protected]> Co-authored-by: kibanamachine <[email protected]>
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Relates to: https://github.com/elastic/sdh-kibana/issues/5923
Summary
Testing
New unit tests:
Updated integration tests:
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
Identify risks
Reviewer notes