From e307f87e1905fdbfa1f4274a2145684cd28f75b2 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Dec 2020 09:57:28 +0100 Subject: [PATCH 1/5] added fix for serializing replicas and updated validation to correctly check for non negative nr --- .../phases/cold_phase/cold_phase.tsx | 2 +- .../phases/shared_fields/forcemerge_field.tsx | 2 +- .../phases/warm_phase/warm_phase.tsx | 6 ++-- .../sections/edit_policy/form/schema.ts | 4 +-- .../serialize_migrate_and_allocate_actions.ts | 35 +++++++++++++++---- .../edit_policy/form/serializer/serializer.ts | 6 ++-- 6 files changed, 41 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx index addad5e572b70..7e63e482f165f 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx @@ -142,7 +142,7 @@ export const ColdPhase: FunctionComponent = () => { { defaultMessage: 'Set replicas' } ), initialValue: Boolean( - policy.phases.cold?.actions?.allocate?.number_of_replicas + policy.phases.cold?.actions?.allocate?.number_of_replicas != null ), }} fullWidth diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx index dd5cc1fbc8c87..72385c988bd03 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx @@ -24,7 +24,7 @@ export const ForcemergeField: React.FunctionComponent = ({ phase }) => { const { policy } = useEditPolicyContext(); const initialToggleValue = useMemo(() => { - return Boolean(policy.phases[phase]?.actions?.forcemerge); + return Boolean(policy.phases[phase]?.actions?.forcemerge != null); }, [policy, phase]); return ( diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx index 9ccfcd58f4d85..65ab3f620297a 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx @@ -162,7 +162,9 @@ export const WarmPhase: FunctionComponent = () => { 'xpack.indexLifecycleMgmt.editPolicy.warmPhase.numberOfReplicas.switchLabel', { defaultMessage: 'Set replicas' } ), - initialValue: Boolean(policy.phases.warm?.actions?.allocate?.number_of_replicas), + initialValue: Boolean( + policy.phases.warm?.actions?.allocate?.number_of_replicas != null + ), }} fullWidth > @@ -203,7 +205,7 @@ export const WarmPhase: FunctionComponent = () => { 'data-test-subj': 'shrinkSwitch', label: i18nTexts.shrinkLabel, 'aria-label': i18nTexts.shrinkLabel, - initialValue: Boolean(policy.phases.warm?.actions?.shrink), + initialValue: Boolean(policy.phases.warm?.actions?.shrink != null), }} fullWidth > diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts index cedf1cdb4d9fe..fa9def6864be0 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts @@ -202,7 +202,7 @@ export const schema: FormSchema = { }), validations: [ { - validator: ifExistsNumberGreaterThanZero, + validator: ifExistsNumberNonNegative, }, ], serializer: serializers.stringToNumber, @@ -273,7 +273,7 @@ export const schema: FormSchema = { }), validations: [ { - validator: ifExistsNumberGreaterThanZero, + validator: ifExistsNumberNonNegative, }, ], serializer: serializers.stringToNumber, diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts index d18a63d34c101..4742c5ae0f630 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts @@ -4,25 +4,40 @@ * you may not use this file except in compliance with the Elastic License. */ -import { isEmpty } from 'lodash'; +import { isEmpty, set } from 'lodash'; import { SerializedActionWithAllocation } from '../../../../../../common/types'; import { DataAllocationMetaFields } from '../../types'; export const serializeMigrateAndAllocateActions = ( + /** + * Form metadata about what tier allocation strategy to use and custom node + * allocation information. + */ { dataTierAllocationType, allocationNodeAttribute }: DataAllocationMetaFields, - newActions: SerializedActionWithAllocation = {}, - originalActions: SerializedActionWithAllocation = {} + /** + * The new configuration merged with old configuration to ensure we don't lose + * any fields. + */ + mergedActions: SerializedActionWithAllocation = {}, + /** + * The actions from the policy for a given phase when it was loaded. + */ + originalActions: SerializedActionWithAllocation = {}, + /** + * The number of replicas value to set in the allocate action. + */ + nrOfReplicas?: number ): SerializedActionWithAllocation => { - const { allocate, migrate, ...otherActions } = newActions; + const { allocate, migrate, ...otherActions } = mergedActions; // First copy over all non-allocate and migrate actions. const actions: SerializedActionWithAllocation = { ...otherActions }; - // The UI only knows about include, exclude and require, so copy over all other values. + // The UI only knows about include, exclude and require and number_of_replicas so copy over all other values. if (allocate) { - const { include, exclude, require, ...otherSettings } = allocate; + const { include, exclude, require, number_of_replicas: __, ...otherSettings } = allocate; if (!isEmpty(otherSettings)) { actions.allocate = { ...otherSettings }; } @@ -69,5 +84,13 @@ export const serializeMigrateAndAllocateActions = ( break; default: } + + if (nrOfReplicas != null) { + actions.allocate = { + ...actions.allocate, + number_of_replicas: nrOfReplicas, + }; + } + return actions; }; diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts index 2071d1be523b6..b1d0063acea5f 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts @@ -95,7 +95,8 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => ( warmPhase.actions = serializeMigrateAndAllocateActions( _meta.warm, warmPhase.actions, - originalPolicy?.phases.warm?.actions + originalPolicy?.phases.warm?.actions, + updatedPolicy.phases.warm?.actions?.allocate?.number_of_replicas ); if (!updatedPolicy.phases.warm?.actions?.forcemerge) { @@ -129,7 +130,8 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => ( coldPhase.actions = serializeMigrateAndAllocateActions( _meta.cold, coldPhase.actions, - originalPolicy?.phases.cold?.actions + originalPolicy?.phases.cold?.actions, + updatedPolicy.phases.warm?.actions?.allocate?.number_of_replicas ); if (_meta.cold.freezeEnabled) { From 591b8fd658a54aac797adea9c33837b0d740fb5b Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Dec 2020 10:18:23 +0100 Subject: [PATCH 2/5] added tests and fixed incorrect use of warm phase setting in cold phase --- .../form/deserializer_and_serializer.test.ts | 47 ++++++++++++++++++- .../edit_policy/form/serializer/serializer.ts | 2 +- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts index bafe6c15d9dca..20f8423ec24fc 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts @@ -56,11 +56,17 @@ const originalPolicy: SerializedPolicy = { shrink: { number_of_shards: 12 }, allocate: { number_of_replicas: 3, + include: { + some: 'value', + }, + exclude: { + some: 'value', + }, }, set_priority: { priority: 10, }, - migrate: { enabled: false }, + migrate: { enabled: true }, }, }, cold: { @@ -133,7 +139,8 @@ describe('deserializer and serializer', () => { const copyOfThisTestPolicy = cloneDeep(thisTestPolicy); - expect(serializer(deserializer(thisTestPolicy))).toEqual(thisTestPolicy); + const _formInternal = deserializer(thisTestPolicy); + expect(serializer(_formInternal)).toEqual(thisTestPolicy); // Assert that the policy we passed in is unaltered after deserialization and serialization expect(thisTestPolicy).not.toBe(copyOfThisTestPolicy); @@ -247,4 +254,40 @@ describe('deserializer and serializer', () => { }, }); }); + + it('sets all known allocate options correctly', () => { + formInternal.phases.warm!.actions.allocate!.number_of_replicas = 0; + formInternal._meta.warm.dataTierAllocationType = 'node_attrs'; + formInternal._meta.warm.allocationNodeAttribute = 'some:value'; + + expect(serializer(formInternal).phases.warm!.actions.allocate).toEqual({ + number_of_replicas: 0, + require: { + some: 'value', + }, + include: { + some: 'value', + }, + exclude: { + some: 'value', + }, + }); + }); + + it('sets allocate and migrate actions when defined together', () => { + formInternal.phases.warm!.actions.allocate!.number_of_replicas = 0; + formInternal._meta.warm.dataTierAllocationType = 'none'; + // This should not be set... + formInternal._meta.warm.allocationNodeAttribute = 'some:value'; + + const result = serializer(formInternal); + + expect(result.phases.warm!.actions.allocate).toEqual({ + number_of_replicas: 0, + }); + + expect(result.phases.warm!.actions.migrate).toEqual({ + enabled: false, + }); + }); }); diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts index b1d0063acea5f..211c7d263e47e 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts @@ -131,7 +131,7 @@ export const createSerializer = (originalPolicy?: SerializedPolicy) => ( _meta.cold, coldPhase.actions, originalPolicy?.phases.cold?.actions, - updatedPolicy.phases.warm?.actions?.allocate?.number_of_replicas + updatedPolicy.phases.cold?.actions?.allocate?.number_of_replicas ); if (_meta.cold.freezeEnabled) { From 0608a75b5942fbb49c10217f175e51ecd3ce55b3 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Dec 2020 10:25:09 +0100 Subject: [PATCH 3/5] remove unused import --- .../form/serializer/serialize_migrate_and_allocate_actions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts index 4742c5ae0f630..869b68a29c874 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { isEmpty, set } from 'lodash'; +import { isEmpty } from 'lodash'; import { SerializedActionWithAllocation } from '../../../../../../common/types'; From cccbae7ede0db622d9f82975f8d342fc2cbb59e9 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Dec 2020 14:49:44 +0100 Subject: [PATCH 4/5] clean up use of Boolean and rename nrOfReplicas -> numberOfReplicas --- .../edit_policy/components/phases/cold_phase/cold_phase.tsx | 5 ++--- .../components/phases/shared_fields/forcemerge_field.tsx | 2 +- .../edit_policy/components/phases/warm_phase/warm_phase.tsx | 6 ++---- .../serializer/serialize_migrate_and_allocate_actions.ts | 6 +++--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx index 7e63e482f165f..5eeb336ad1108 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx @@ -141,9 +141,8 @@ export const ColdPhase: FunctionComponent = () => { 'xpack.indexLifecycleMgmt.editPolicy.coldPhase.numberOfReplicas.switchLabel', { defaultMessage: 'Set replicas' } ), - initialValue: Boolean( - policy.phases.cold?.actions?.allocate?.number_of_replicas != null - ), + initialValue: + policy.phases.cold?.actions?.allocate?.number_of_replicas != null, }} fullWidth > diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx index 72385c988bd03..69121cc2d1252 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/forcemerge_field.tsx @@ -24,7 +24,7 @@ export const ForcemergeField: React.FunctionComponent = ({ phase }) => { const { policy } = useEditPolicyContext(); const initialToggleValue = useMemo(() => { - return Boolean(policy.phases[phase]?.actions?.forcemerge != null); + return policy.phases[phase]?.actions?.forcemerge != null; }, [policy, phase]); return ( diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx index 65ab3f620297a..b573bc6a80632 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx @@ -162,9 +162,7 @@ export const WarmPhase: FunctionComponent = () => { 'xpack.indexLifecycleMgmt.editPolicy.warmPhase.numberOfReplicas.switchLabel', { defaultMessage: 'Set replicas' } ), - initialValue: Boolean( - policy.phases.warm?.actions?.allocate?.number_of_replicas != null - ), + initialValue: policy.phases.warm?.actions?.allocate?.number_of_replicas != null, }} fullWidth > @@ -205,7 +203,7 @@ export const WarmPhase: FunctionComponent = () => { 'data-test-subj': 'shrinkSwitch', label: i18nTexts.shrinkLabel, 'aria-label': i18nTexts.shrinkLabel, - initialValue: Boolean(policy.phases.warm?.actions?.shrink != null), + initialValue: policy.phases.warm?.actions?.shrink != null, }} fullWidth > diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts index 869b68a29c874..fefff990e4b57 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts @@ -28,7 +28,7 @@ export const serializeMigrateAndAllocateActions = ( /** * The number of replicas value to set in the allocate action. */ - nrOfReplicas?: number + numberOfReplicas?: number ): SerializedActionWithAllocation => { const { allocate, migrate, ...otherActions } = mergedActions; @@ -85,10 +85,10 @@ export const serializeMigrateAndAllocateActions = ( default: } - if (nrOfReplicas != null) { + if (numberOfReplicas != null) { actions.allocate = { ...actions.allocate, - number_of_replicas: nrOfReplicas, + number_of_replicas: numberOfReplicas, }; } From 84298dc0c198b0abbd2786f4930e939cfafa52f5 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 8 Dec 2020 14:50:09 +0100 Subject: [PATCH 5/5] fix comment --- .../form/serializer/serialize_migrate_and_allocate_actions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts index fefff990e4b57..24cfec46393fc 100644 --- a/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts +++ b/x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serialize_migrate_and_allocate_actions.ts @@ -35,7 +35,7 @@ export const serializeMigrateAndAllocateActions = ( // First copy over all non-allocate and migrate actions. const actions: SerializedActionWithAllocation = { ...otherActions }; - // The UI only knows about include, exclude and require and number_of_replicas so copy over all other values. + // The UI only knows about include, exclude, require and number_of_replicas so copy over all other values. if (allocate) { const { include, exclude, require, number_of_replicas: __, ...otherSettings } = allocate; if (!isEmpty(otherSettings)) {