Skip to content
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.

Sign up for GitHub

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] Update show/hide data tier logic on cloud #81455

Merged
merged 4 commits into from
Oct 26, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0bbca57
Update show/hide data tier logic
jloleysens Oct 22, 2020
c66396f
Merge branch 'master' into ilm/stricter-check-for-use-of-node-roles
kibanamachine Oct 26, 2020
cc9bf4d
refactor node role types for clarity
jloleysens Oct 26, 2020
71985c6
added comment
jloleysens Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,8 @@ describe('edit policy', () => {
test('should hide data tier option on cloud using legacy node role configuration', async () => {
http.setupNodeListResponse({
nodesByAttributes: { test: ['123'] },
nodesByRoles: { data: ['test'], data_hot: ['test'], data_warm: ['test'] },
// On cloud, if using legacy config there will not be any "data_*" roles set.
nodesByRoles: { data: ['test'] },
isUsingDeprecatedDataRoleConfig: true,
});
const rendered = mountWithIntl(component);
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/index_lifecycle_management/common/constants/data_tiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

// Order of node roles matters here, the warm phase prefers allocating data
// to the data_warm role.
import { NodeDataRole, PhaseWithAllocation } from '../types';
import { DataTierRole, PhaseWithAllocation } from '../types';

const WARM_PHASE_NODE_PREFERENCE: NodeDataRole[] = ['data_warm', 'data_hot'];
const WARM_PHASE_NODE_PREFERENCE: DataTierRole[] = ['data_warm', 'data_hot'];

const COLD_PHASE_NODE_PREFERENCE: NodeDataRole[] = ['data_cold', 'data_warm', 'data_hot'];
const COLD_PHASE_NODE_PREFERENCE: DataTierRole[] = ['data_cold', 'data_warm', 'data_hot'];

export const phaseToNodePreferenceMap: Record<PhaseWithAllocation, NodeDataRole[]> = Object.freeze({
export const phaseToNodePreferenceMap: Record<PhaseWithAllocation, DataTierRole[]> = Object.freeze({
warm: WARM_PHASE_NODE_PREFERENCE,
cold: COLD_PHASE_NODE_PREFERENCE,
});
4 changes: 2 additions & 2 deletions x-pack/plugins/index_lifecycle_management/common/types/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { NodeDataRoleWithCatchAll } from '.';
import { AnyDataRole } from '.';

export interface ListNodesRouteResponse {
nodesByAttributes: { [attributePair: string]: string[] };
nodesByRoles: { [role in NodeDataRoleWithCatchAll]?: string[] };
nodesByRoles: { [role in AnyDataRole]?: string[] };

/**
* A flag to indicate whether a node is using `settings.node.data` which is the now deprecated way cloud configured
Expand Down
17 changes: 13 additions & 4 deletions x-pack/plugins/index_lifecycle_management/common/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,17 @@ export * from './api';
export * from './policies';

/**
* 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.
* These roles reflect how nodes are stratified into different data tiers.
*/
export type NodeDataRole = 'data_hot' | 'data_warm' | 'data_cold';
export type NodeDataRoleWithCatchAll = 'data' | NodeDataRole;
export type DataTierRole = '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 DataRole = 'data_content' | DataTierRole;

/**
* The "data" role can store data allocated to any tier.
*/
export type AnyDataRole = 'data' | DataRole;
4 changes: 2 additions & 2 deletions ...ecycle_management/public/application/lib/data_tiers/get_available_node_roles_for_phase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
*/

import {
NodeDataRole,
DataTierRole,
ListNodesRouteResponse,
PhaseWithAllocation,
} from '../../../../common/types';

import { phaseToNodePreferenceMap } from '../../../../common/constants';

export type AllocationNodeRole = NodeDataRole | 'none';
export type AllocationNodeRole = DataTierRole | 'none';

/**
* Given a phase and current cluster node roles, determine which nodes the phase
Expand Down
4 changes: 2 additions & 2 deletions ...x_lifecycle_management/public/application/lib/data_tiers/is_node_role_first_preference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { NodeDataRole, PhaseWithAllocation } from '../../../../common/types';
import { DataTierRole, PhaseWithAllocation } from '../../../../common/types';
import { phaseToNodePreferenceMap } from '../../../../common/constants';

export const isNodeRoleFirstPreference = (phase: PhaseWithAllocation, nodeRole: NodeDataRole) => {
export const isNodeRoleFirstPreference = (phase: PhaseWithAllocation, nodeRole: DataTierRole) => {
return phaseToNodePreferenceMap[phase][0] === nodeRole;
};
8 changes: 4 additions & 4 deletions ...cation/sections/edit_policy/components/data_tier_allocation/default_allocation_notice.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import { i18n } from '@kbn/i18n';
import React, { FunctionComponent } from 'react';
import { EuiCallOut } from '@elastic/eui';

import { PhaseWithAllocation, NodeDataRole } from '../../../../../../common/types';
import { PhaseWithAllocation, DataTierRole } from '../../../../../../common/types';

import { AllocationNodeRole } from '../../../../lib';

const i18nTextsNodeRoleToDataTier: Record<NodeDataRole, string> = {
const i18nTextsNodeRoleToDataTier: Record<DataTierRole, string> = {
data_hot: i18n.translate('xpack.indexLifecycleMgmt.editPolicy.dataTierHotLabel', {
defaultMessage: 'hot',
}),
Expand All @@ -31,7 +31,7 @@ const i18nTexts = {
'xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.warm.title',
{ defaultMessage: 'No nodes assigned to the warm tier' }
),
body: (nodeRole: NodeDataRole) =>
body: (nodeRole: DataTierRole) =>
i18n.translate('xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.warm', {
defaultMessage:
'This policy will move data in the warm phase to {tier} tier nodes instead.',
Expand All @@ -43,7 +43,7 @@ const i18nTexts = {
'xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.cold.title',
{ defaultMessage: 'No nodes assigned to the cold tier' }
),
body: (nodeRole: NodeDataRole) =>
body: (nodeRole: DataTierRole) =>
i18n.translate('xpack.indexLifecycleMgmt.warmPhase.dataTier.defaultAllocationNotice.cold', {
defaultMessage:
'This policy will move data in the cold phase to {tier} tier nodes instead.',
Expand Down
13 changes: 9 additions & 4 deletions .../application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,19 @@ export const DataTierAllocationField: FunctionComponent<Props> = ({
return (
<NodesDataProvider>
{({ nodesByRoles, nodesByAttributes, isUsingDeprecatedDataRoleConfig }) => {
const hasDataNodeRoles = Object.keys(nodesByRoles).some((nodeRole) =>
// match any of the "data_" roles, including data_content.
nodeRole.trim().startsWith('data_')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
const hasNodeAttrs = Boolean(Object.keys(nodesByAttributes ?? {}).length);

const renderNotice = () => {
switch (phaseData.dataTierAllocationType) {
case 'default':
const isCloudEnabled = cloud?.isCloudEnabled ?? false;
if (isCloudEnabled && phase === 'cold') {
const isUsingNodeRolesAllocation = !isUsingDeprecatedDataRoleConfig;
const isUsingNodeRolesAllocation =
!isUsingDeprecatedDataRoleConfig && hasDataNodeRoles;
const hasNoNodesWithNodeRole = !nodesByRoles.data_cold?.length;

if (isUsingNodeRolesAllocation && hasNoNodesWithNodeRole) {
Expand Down Expand Up @@ -120,9 +125,9 @@ export const DataTierAllocationField: FunctionComponent<Props> = ({
phaseData={phaseData}
isShowingErrors={isShowingErrors}
nodes={nodesByAttributes}
disableDataTierOption={
!!(isUsingDeprecatedDataRoleConfig && cloud?.isCloudEnabled)
}
disableDataTierOption={Boolean(
cloud?.isCloudEnabled && !hasDataNodeRoles && isUsingDeprecatedDataRoleConfig
)}
/>

{/* Data tier related warnings and call-to-action notices */}
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/index_lifecycle_management/server/routes/api/nodes/register_list_route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { ListNodesRouteResponse, NodeDataRole } from '../../../../common/types';
import { ListNodesRouteResponse, DataTierRole } from '../../../../common/types';

import { RouteDependencies } from '../../../types';
import { addBasePath } from '../../../services';
Expand Down Expand Up @@ -39,10 +39,10 @@ export function convertSettingsIntoLists(
}
}

const dataRoles = nodeSettings.roles.filter((r) => r.startsWith('data')) as NodeDataRole[];
const dataRoles = nodeSettings.roles.filter((r) => r.startsWith('data')) as DataTierRole[];
for (const role of dataRoles) {
accum.nodesByRoles[role as NodeDataRole] = accum.nodesByRoles[role] ?? [];
accum.nodesByRoles[role as NodeDataRole]!.push(nodeId);
accum.nodesByRoles[role as DataTierRole] = accum.nodesByRoles[role] ?? [];
accum.nodesByRoles[role as DataTierRole]!.push(nodeId);
}

// If we detect a single node using legacy "data:true" setting we know we are not using data roles for
Expand Down