[ILM] Update show/hide data tier logic on cloud by jloleysens · Pull Request #81455 · elastic/kibana

jloleysens · 2020-10-22T09:33:02Z

Summary

Only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present in the cluster. This could be a role like data_content which will ensure that the cluster is configured with the new data tier roles.

CC @jasontedor

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present. this will likely be a role like data_content which will ensure that the cluster is configured with the new data tier roles.

elasticmachine · 2020-10-22T09:33:05Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

cjcenizal

Code LGTM! Didn't test locally. Just had a few suggestions.

cjcenizal · 2020-10-22T22:20:11Z

x-pack/plugins/index_lifecycle_management/common/types/index.ts

+ * The "data" role can store data on any tier and the "data_content" role can store
+ * all data the ES stack uses for feature functionality like security related indices.
+ */
+export type NodeDataRoleWithCatchAll = 'data' | 'data_content' | NodeDataRole;


Nit: not sure the name makes sense now. Perhaps it would make more sense to call this AnyNodeRole and rename NodeDataRole to DataTierNodeRole?

cjcenizal · 2020-10-22T22:39:51Z

...lic/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx

    <NodesDataProvider>
      {({ nodesByRoles, nodesByAttributes, isUsingDeprecatedDataRoleConfig }) => {
+        const hasDataNodeRoles = Object.keys(nodesByRoles).some((nodeRole) =>
+          nodeRole.trim().startsWith('data_')


I can imagine someone reading this and being unsure if it's intentional that this evaluates to true for data_content, too. How about we make the intention more explicit by using a type guard?

Building off my other suggestion, types/index.ts would become this:

/** * These roles reflect how nodes are stratified into different data tiers. * */ export type DataTierNodeRole = 'data_hot' | 'data_warm' | 'data_cold'; /** * The "data_content" role can store all data the ES stack uses for feature functionality * like security related indices. */ export type DataNodeRole = 'data_content' | DataTierNodeRole; /** * The "data" role can store data on any tier. */ export type AnyNodeRole = 'data' | DataNodeRole; export const isDataNodeRole = (nodeRole: string): value is DataNodeRole => { const dataNodeRoles: string[] = ['data_hot', 'data_warm', 'data_cold', 'data_content']; return dataNodeRoles.includes(nodeRole); };

And then this becomes:

const hasDataNodeRoles = Object.keys(nodesByRoles).some(isDataNodeRole);

I like the refactor suggestion regarding the data role types! Have implemented a refactor.

I'm not sure I share the same concern regarding intent, but I added a comment for clarity to the existing startsWith code 👍

The reason I mentioned it was because I was personally unclear on whether that was the intention, until I read the PR description. :) Thanks for adding the comment.

jloleysens · 2020-10-26T08:37:38Z

@elasticmachine merge upstream

kibanamachine · 2020-10-26T10:33:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 71985c6

Metrics [docs]

async chunks size

id	before	after	diff
`indexLifecycleManagement`	269.7KB	269.9KB	+160.0B

History

💚 Build #83573 succeeded c66396f
💚 Build #83304 succeeded 0bbca57

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

* Update show/hide data tier logic only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present. this will likely be a role like data_content which will ensure that the cluster is configured with the new data tier roles. * refactor node role types for clarity * added comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Update show/hide data tier logic only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present. this will likely be a role like data_content which will ensure that the cluster is configured with the new data tier roles. * refactor node role types for clarity * added comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/index_lifecycle_management/server/routes/api/nodes/register_list_route.ts

* Update show/hide data tier logic only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present. this will likely be a role like data_content which will ensure that the cluster is configured with the new data tier roles. * refactor node role types for clarity * added comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…arm-phase-to-formlib * 'master' of github.com:elastic/kibana: [Trigger Actions UI] Properly unmount app (elastic#81436) skip flaky suite (elastic#81576) skip flaky suite (elastic#78373) [Security Solution] Fix styling of EditDataProvider content (elastic#81456) [Search] Error Alignment 2 (elastic#80965) [APM] Unskip test (elastic#81574) [ML] Fix partition value selection on the Single Metric Viewer (elastic#81585) cleaning up expression service types (elastic#80643) Fix suggestions dropdown for query input (elastic#80990) [Usage collection] Make `schema` mandatory (elastic#79999) [ILM] Update show/hide data tier logic on cloud (elastic#81455) added brace import to advanced settings (elastic#81458) chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies

* [ILM] Update show/hide data tier logic on cloud (#81455) * Update show/hide data tier logic only show data tier options on cloud if the cluster is not using the deprecated node.data config and there is a data_* role present. this will likely be a role like data_content which will ensure that the cluster is configured with the new data tier roles. * refactor node role types for clarity * added comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/index_lifecycle_management/server/routes/api/nodes/register_list_route.ts * Replace `r` with `(r)`

jloleysens added Feature:ILM v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.10.1 labels Oct 22, 2020

jloleysens requested a review from cjcenizal October 22, 2020 09:33

jloleysens requested a review from a team as a code owner October 22, 2020 09:33

cjcenizal approved these changes Oct 22, 2020

View reviewed changes

kibanamachine and others added 3 commits October 26, 2020 04:37

Merge branch 'master' into ilm/stricter-check-for-use-of-node-roles

c66396f

refactor node role types for clarity

cc9bf4d

added comment

71985c6

jloleysens merged commit 0a0773a into elastic:master Oct 26, 2020

jloleysens deleted the ilm/stricter-check-for-use-of-node-roles branch October 26, 2020 10:37

jloleysens added the v7.11.0 label Oct 26, 2020

jloleysens mentioned this pull request Oct 26, 2020

[7.x] [ILM] Update show/hide data tier logic on cloud (#81455) #81589

Merged

jloleysens mentioned this pull request Oct 26, 2020

[7.10] [ILM] Update show/hide data tier logic on cloud (#81455) #81590

Merged

cjcenizal added the NeededFor:Cloud label Mar 9, 2021

cjcenizal mentioned this pull request Mar 30, 2021

[ILM ] Fix logic for showing/hiding recommended allocation on Cloud #90592

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILM] Update show/hide data tier logic on cloud#81455

[ILM] Update show/hide data tier logic on cloud#81455
jloleysens merged 4 commits intoelastic:masterfrom
jloleysens:ilm/stricter-check-for-use-of-node-roles

jloleysens commented Oct 22, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Oct 22, 2020

Uh oh!

cjcenizal left a comment

Uh oh!

cjcenizal Oct 22, 2020

Uh oh!

cjcenizal Oct 22, 2020

Uh oh!

jloleysens Oct 26, 2020

Uh oh!

cjcenizal Oct 26, 2020

Uh oh!

jloleysens commented Oct 26, 2020

Uh oh!

kibanamachine commented Oct 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jloleysens commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Oct 22, 2020

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

cjcenizal Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

jloleysens Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

jloleysens commented Oct 26, 2020

Uh oh!

kibanamachine commented Oct 26, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jloleysens commented Oct 22, 2020 •

edited

Loading