[ILM] Data tier notices should reflect tier preferences #78398
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- also removed the lingering "data_frozen" node role
jloleysens
added
Feature:ILM
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@elasticmachine merge upstream |
debadair
reviewed
Sep 24, 2020
There was a problem hiding this comment.
Made a few minor suggestions. Also noticed a few other things...
The Cold phase description feels like it needs to be pared down & the "because" clause doesn't quite make sense. Maybe:
In the cold phase, data is queried less frequently and query speed is less important. You can store cold data on less expensive hardware and reduce the number of replicas.
Data is repeated in the Data allocation description & doesn't need to be. Less-frequent should be hyphenated:
Move data to nodes optimized for less-frequent, read-only access.
Freeze description is missing end punctuation & I'd reword it to focus on the action:
Make the index read-only and minimize its memory footprint.
| body: (nodeRole: NodeDataRole) => | ||
| i18n.translate('xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.warm', { | ||
| defaultMessage: | ||
| 'There are no nodes in the warm tier. Data in this phase will be allocated to the {tier}.', |
There was a problem hiding this comment.
I wouldn't repeat what the heading says.
Suggested change
| 'There are no nodes in the warm tier. Data in this phase will be allocated to the {tier}.', | |
| 'Data in the warm phase will be allocated to the {tier} instead.', |
| body: (nodeRole: NodeDataRole) => | ||
| i18n.translate('xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.cold', { | ||
| defaultMessage: | ||
| 'There are no nodes in the cold tier. Data in this phase will be allocated to the {tier}.', |
There was a problem hiding this comment.
Suggested change
| 'There are no nodes in the cold tier. Data in this phase will be allocated to the {tier}.', | |
| 'Data in the cold phase will be allocated to the {tier} instead.', |
| 'xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotAvailableBody', | ||
| { | ||
| defaultMessage: | ||
| 'Assign at least one node to the warm or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no warm nodes.', |
There was a problem hiding this comment.
Suggested change
| 'Assign at least one node to the warm or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no warm nodes.', | |
| 'Assign at least one node to the warm or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no available nodes.', |
| 'xpack.indexLifecycleMgmt.coldPhase.dataTier.defaultAllocationNotAvailableBody', | ||
| { | ||
| defaultMessage: | ||
| 'Assign at least one node to the cold, warm or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no cold nodes.', |
There was a problem hiding this comment.
Suggested change
| 'Assign at least one node to the cold, warm or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no cold nodes.', | |
| 'Assign at least one node to the cold, warm, or hot tier to use role-based allocation. The policy will fail to complete allocation if there are no available nodes.', |
|
@jloleysens I'm having trouble testing this locally. When I start ES via Did you run into this? |
|
@elasticmachine merge upstream |
@cjcenizal I did not run into this... I can't recall seeing that particular error before. Does this happen with a fresh ES snapshot running without pre-existing data? |
|
@debadair Thanks for the copy review, I implemented your feedback except for the change to the freeze description. I just wanted to confirm, regarding freeze description, that you were suggesting changing: To I also spoke with @andreidan and he suggested that the UI should not speak in too certain terms about where data will be allocated because there are also index level settings that can affect allocation. What do you think about something like: That way we reference the policy in our notification? |
|
@jloleysens I chatted with Lee and he pointed out that we need to specify Otherwise the security indices' shards won't get allocated anywhere and the builtin users can't be created. |
cjcenizal
approved these changes
Sep 28, 2020
There was a problem hiding this comment.
Tested locally, code LGTM! Had a few minor readability suggestions. Can we add a section with instructions for manually testing this to the README? Just something simple, mentioning that you can try the different notice states by specifying different node roles, e.g.
yarn es snapshot --license=trial -E node.roles=data_hot,master,data_content
| /** | ||
| * These roles reflect how nodes are stratified into different data tiers. The "data" role | ||
| * is a catch-all that can be used to store data in any phase. | ||
| */ |
| * If no nodes can be identified for allocation (very special case) then | ||
| * we return "none". | ||
| */ | ||
| export const determineAllocationNodeRole = ( |
There was a problem hiding this comment.
Nit: a name that mentions "phase" would communicate the interaction between phase and node role to me, e.g. getAvailableNodeRoleForPhase.
| return phaseToNodePreferenceMap[phase][0]; | ||
| } | ||
|
|
||
| const preferredNodeRoles = phaseToNodePreferenceMap[phase]; |
There was a problem hiding this comment.
Nit: Since we refer to phaseToNodePreferenceMap[phase] on line 32, maybe we can define this variable on line 29 and refer to it on line 32?
| import { AllocationNodeRole } from '../../../../lib'; | ||
|
|
||
| const i18nTextsNodeRoleToDataTier: Record<NodeDataRole, string> = { | ||
| data_hot: i18n.translate('xpack.indexLifecycleMgmt.common.dataTier.hot', { |
There was a problem hiding this comment.
We should follow the i18n naming guidelines throughout this file: https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md
| const hasNodeAttrs = Boolean(Object.keys(nodesData.nodesByAttributes ?? {}).length); | ||
|
|
||
| const renderDefaultAllocationNotice = () => { |
There was a problem hiding this comment.
WDYT of organizing the early exits first, to help the reader scan and understand the states that don't require a notice? Something like this maybe:
const renderDefaultAllocationNotice = () => {
if (phaseData.dataTierAllocationType !== 'default') {
return null;
}
const allocationNodeRole = determineAllocationNodeRole(phase, nodesData.nodesByRoles);
const isFirstPreferredNodeRole = allocationNodeRole !== 'none' && isNodeRoleFirstPreference(phase, allocationNodeRole);
if (isFirstPreferredNodeRole) {
return null;
}
return <DefaultAllocationNotice phase={phase} targetNodeRole={allocationNodeRole} />;
};| return null; | ||
| }; | ||
|
|
||
| const renderNodeAttributesWarning = () => { |
There was a problem hiding this comment.
We could do something similar here:
const renderNodeAttributesWarning = () => {
if (phaseData.dataTierAllocationType !== 'custom') {
return null;
}
if (hasNodeAttrs) {
return null;
}
return <NoNodeAttributesWarning phase={phase} />;
};
|
@elasticmachine merge upstream |
Thanks for tracking down a solution and sharing it! |
|
Yes, that is what I was suggesting WRT to freeze. I like referencing the policy in the notification, I think that works well. |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts·ts.Actions and Triggers app alerts should delete all selectionStandard OutStack TraceMetrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Sep 30, 2020
* Refactor allocation notices for tier preferences - also removed the lingering "data_frozen" node role * added some test coverage * Implement copy feedback * Minor refactors based on PR feedback * expanded README.md with section on testing cluster state notices * Updated copy to reference policy and updated freeze description Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens
added a commit
that referenced
this pull request
Sep 30, 2020
) * Refactor allocation notices for tier preferences - also removed the lingering "data_frozen" node role * added some test coverage * Implement copy feedback * Minor refactors based on PR feedback * expanded README.md with section on testing cluster state notices * Updated copy to reference policy and updated freeze description Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
phillipb
added a commit
to phillipb/kibana
that referenced
this pull request
Sep 30, 2020
…aly-detection-partition-field * 'master' of github.com:elastic/kibana: (37 commits) [UiActions] Don't throw an error if there are no compatible actions to execute (elastic#78917) [Observability] Kibana nav when docked overlaps the content of the pages. (elastic#78593) Invalid `searchSourceJSON` causes saved object migration to fail (elastic#78535) update vega version (elastic#78390) Fix warning text doesn't get displayed on filters with custom filter name (elastic#78617) [ILM] Data tier notices should reflect tier preferences (elastic#78398) [APM] Service Map: `Not Defined` option doesn't work properly (elastic#77757) Improve invalid field error message at search.aggs.param_types.field (elastic#78587) Remove isDeprecated flag on visType (elastic#78820) Remove unused elasticsearch.preserverHost setting (elastic#78608) Fix condition and adjust tests (elastic#78898) [UX] Add percentile selector (elastic#78562) [ML] Replace use of rest_total_hits_as_int with track_total_hits (elastic#78423) expression service docs (elastic#78774) [Functional] Wait for the page to load and then click the new vis button (elastic#78725) [TSVB] No data in visualizations with annotations (elastic#78794) [kbn/ui-shared-deps] track asset sizes (elastic#78718) delete target before building (elastic#78665) Move indexPattern.popularizeField into discover (elastic#77668) [Security Solution][Resolver]Add backdrop to pills (elastic#78625) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
With the introduction of tier preferences (elastic/elasticsearch#62589) for phases in ILM. The UI should be updated to show more appropriate notices to users.
Tier preferences are a way for ILM, when migrating data, to fallback to a different tier. This is a convenience mechanism to prevent an ILM policy from getting stuck in a phase and never completing. The current UI notifies the user of when they are creating a policy that will get stuck, but does not take into consideration this new fallback - which this contribution adds.
The current UI will still warn a user when there is nowhere to assign data, but this should be very rare.
How to test
yarn es snapshot -E node.roles=data_hot,master(cluster with only data hot node)Screenshots
In this case there is only a single
data_hotnode so this phase falls back all the way to the hot tier (as should warm). If there were a warm tier, data allocation would fallback to the warm tier.Checklist
Delete any items that are not applicable to this PR.