-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ILM] Allow multiple searchable snapshot actions #92789
[ILM] Allow multiple searchable snapshot actions #92789
Conversation
…s show replicas field
…and hot phase without rollover test
@elasticmachine merge upstream |
merge conflict between base and head |
…tiple-searchable-snapshot-actions * 'master' of github.com:elastic/kibana: [Rollup] Fix use of undefined value in JS import (elastic#92791) [ILM] Fix replicas not showing (elastic#92782) [Event Log] Extended README.md with the documentation for a REST API and Start plugin contract. (elastic#92562) [XY] Enables page reload toast for the legacyChartsLibrary setting (elastic#92811) [Security Solution][Case] Improve hooks (elastic#89580) [Security Solution] Update wordings and breadcrumb for timelines page (elastic#90809) [Security Solution] Replace EUI theme with mocks in jest suites (elastic#92462) docs: ✏️ use correct heading level (elastic#92806) [ILM ] Fix logic for showing/hiding recommended allocation on Cloud (elastic#90592) [Security Solution][Detections] Pull gap detection logic out in preparation for sharing between rule types (elastic#91966) [core.savedObjects] Remove _shard_doc tiebreaker since ES now adds it automatically. (elastic#92295) docs: ✏️ fix links in embeddable plugin readme (elastic#92778) # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
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.
Hi @jloleysens, thanks a lot for working on this!
I tested locally and noticed that when a searchable snapshot is configured in the hot phase, there is still an info callout saying that searchable snapshot in the cold phase is not possible.
Also it's not possible to save a policy with 2 different snapshot repos configured for hot and cold searchable snapshots. That is not blocking, but could be worth looking into. The API error is not very descriptive, it just says couldn't parse policy
. We could probably add validation, pre-fill the combobox or limit the options for this field in relation to the other phase.
Thanks for working on this @jloleysens! @yuliacech @jloleysens we don't support multiple searchable snapshot actions being defined for different repositories. We validate against this elastic/elasticsearch#68856 I'd like to also mention elastic/elasticsearch#68714 which introduced another option in the searchable snapshot configuration called |
@yuliacech Great catch! In the process of cherry picking I think I excluded the commit for removing this!!
This is a really good point, I think for now we can rely on ES validation for this (especially given Lee's PR description about enabling this in future: elastic/elasticsearch#68714 (comment)), but I think you are right with respect to the error message not being very helpful. I'll see what I can do to improve the situation! |
@yuliacech I think I have addressed your feedback! Would you mind taking another look? I updated the error handling logic server side to include the full caused_by chain so that we can report more detail: I spoke with @sebelga and he mentioned that we should probably enhance the form to prevent users from getting into this error state in the first place. This is something we can do for 7.13. @andreidan how likely do you think it is that we will have the ability to configure different snapshots? |
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.
Hi @jloleysens, thanks a lot for addressing my feedback! Tested locally and all worked for me. I think seeing a detailed ES error is very helpful.
@elasticmachine merge upstream |
Different repositories? Not likely as it's a niche use case. |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_threat_matching·ts.detection engine api security and spaces enabled create_threat_matching tests with auditbeat data indicator enrichment generates multiple signals with multiple matchesStandard Out
Stack Trace
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
LGTM, thanks JL! Left a minor comment regarding the error message displayed, but I might be misreading where the message is applied
@@ -254,7 +251,7 @@ export const SearchableSnapshotField: FunctionComponent<Props> = ({ phase }) => | |||
'xpack.indexLifecycleMgmt.editPolicy.searchableSnapshotCalloutBody', | |||
{ | |||
defaultMessage: | |||
'Force merge, shrink, freeze and cold phase searchable snapshots are not allowed when searchable snapshots are enabled in the hot phase.', | |||
'Force merge, shrink and freeze actions are not allowed when searchable snapshots are enabled in this phase.', |
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 think this message needs to be a bit different (maybe the validation too?) eg. shrinking in warm would not be allowed if searchable_snapshot is configured in hot (these actions are not allowed to follow searchable_snapshot, irrespective if they're configured in the same phase or a later phase)
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.
Right, at the moment the form hides force merge, shrink and freeze only in subsequent phases to hot because in hot searchable snapshot is the last action:
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.
But perhaps we need to slightly re-word to state that more clearly, force merge etc. are not allowed in subsequent phases. I might be misunderstanding you so 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.
Gotcha! Makes sense, thanks @jloleysens! (when the frozen tier comes to life we'll need to adjust this message and the validation - as the cold phase searchable_snapshot can precede the frozen phase actions)
I have added support for |
@sebelga I don't think this is a bug. My view is that we shouldn't display it at all in the UI (as the API defaults configure the mount type to "shared_cache" in the frozen phase and "full_copy" in all other phases, and other configurations shouldn't be encouraged - and are a niche use case at best), but maybe this is a question for @jethr0null |
I can bring it to the team but I am not sure the API should let you do more than the UI. If we really don't want users to change some parameters we shouldn't expose them at all IMO. But I hear you, we should probably hide it behind an "advanced" section. |
* remove logic that disables SS action in cold if no rollover and always show replicas field * update test coverage to be consistent with new form behaviour and expand hot phase without rollover test * only licensing can disable searchable snapshot field * clean up i18n * remove ss field callout * update error reporting logic to include causes chain, also update UI to show causes * updated searchable snapshot field in hot phase callout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* remove logic that disables SS action in cold if no rollover and always show replicas field * update test coverage to be consistent with new form behaviour and expand hot phase without rollover test * only licensing can disable searchable snapshot field * clean up i18n * remove ss field callout * update error reporting logic to include causes chain, also update UI to show causes * updated searchable snapshot field in hot phase callout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…bana into task-manager/docs-monitoring * 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: [ILM] Allow multiple searchable snapshot actions (elastic#92789) Improve consistency for display of management items (elastic#92694) skip flaky suite (elastic#93152) skip flaky suite (elastic#93152) [ILM] Refactor edit_policy client integration tests into separate feature files (elastic#92826) Add developer documentation about the building blocks we offer plugin developers (elastic#92743) [Security Solution] Case ui enhancement (elastic#91863) [Security Solution] [Detections] Updates warning message when no indices match provided index patterns (elastic#93094) Collect agent telemetry even when fleet server is disabled. (elastic#93198) [Lens] Fix runtime validation error message (elastic#93195) [Lens] Remove warning about ordinal x-domain (elastic#93049) [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask (elastic#93150) Cleanup Security plugin imports (elastic#93056) [Security Solution] - Bug fixes (elastic#92294) Updated doc links (elastic#92968) [ML] Transforms: Fixes chart histograms for runtime fields. (elastic#93028) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086)
* remove logic that disables SS action in cold if no rollover and always show replicas field * update test coverage to be consistent with new form behaviour and expand hot phase without rollover test * only licensing can disable searchable snapshot field * clean up i18n * remove ss field callout * update error reporting logic to include causes chain, also update UI to show causes * updated searchable snapshot field in hot phase callout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* remove logic that disables SS action in cold if no rollover and always show replicas field * update test coverage to be consistent with new form behaviour and expand hot phase without rollover test * only licensing can disable searchable snapshot field * clean up i18n * remove ss field callout * update error reporting logic to include causes chain, also update UI to show causes * updated searchable snapshot field in hot phase callout Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* master: (199 commits) Convert Canvas docs to MDX for use in Elastic Docs (elastic#91969) [Bazel] More resilient Workspace Status (elastic#93244) [Discover] Change icon of saved search in open search panel and embeddable selection (elastic#93001) [Workplace Search] Role Mappings to Kibana (elastic#93123) [Fleet] Use type-only imports where possible (elastic#92979) [Lens] Set pie chart slices sorted clockwise (elastic#92617) Remove ms label from CPU load on status page (elastic#92836) [App Search] Migrate Create Meta Engine View (elastic#92127) [Time to Visualize] Disable Visualize URL Tracker When Linked to OriginatingApp (elastic#92917) [ILM] Allow multiple searchable snapshot actions (elastic#92789) Improve consistency for display of management items (elastic#92694) skip flaky suite (elastic#93152) skip flaky suite (elastic#93152) [ILM] Refactor edit_policy client integration tests into separate feature files (elastic#92826) Add developer documentation about the building blocks we offer plugin developers (elastic#92743) [Security Solution] Case ui enhancement (elastic#91863) [Security Solution] [Detections] Updates warning message when no indices match provided index patterns (elastic#93094) Collect agent telemetry even when fleet server is disabled. (elastic#93198) [Lens] Fix runtime validation error message (elastic#93195) [Lens] Remove warning about ordinal x-domain (elastic#93049) ...
Summary
As of 7.12+, ES will allow ILM policies to specify multiple, subsequent searchable snapshot actions that all use the same snapshot repository. This PR updates the form to be in line with this behaviour.
This requirement was surfaced as @andreidan and I were reviewing the current ILM policy UI.