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

Jump to bottom

[ILM] Fix detection of "migrate.enabled: false" #81642

Merged
merged 2 commits into from
Oct 26, 2020
Merged

[ILM] Fix detection of "migrate.enabled: false" #81642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a63d9a
correctly detect migrate.enabled === false!
jloleysens Oct 26, 2020
30e2c60
added jest test
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
55 changes: 55 additions & 0 deletions ...k/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,58 @@ export const DELETE_PHASE_POLICY: PolicyFromES = {
},
name: POLICY_NAME,
};

export const POLICY_WITH_NODE_ATTR_AND_OFF_ALLOCATION: PolicyFromES = {
version: 1,
modified_date: Date.now().toString(),
policy: {
phases: {
hot: {
min_age: '0ms',
actions: {
rollover: {
max_size: '50gb',
},
},
},
warm: {
actions: {
allocate: {
require: {},
include: { test: '123' },
exclude: {},
},
},
},
cold: {
actions: {
migrate: { enabled: false },
},
},
},
name: POLICY_NAME,
},
name: POLICY_NAME,
};

export const POLICY_WITH_NODE_ROLE_ALLOCATION: PolicyFromES = {
version: 1,
modified_date: Date.now().toString(),
policy: {
phases: {
hot: {
min_age: '0ms',
actions: {
rollover: {
max_size: '50gb',
},
},
},
warm: {
actions: {},
},
},
name: POLICY_NAME,
},
name: POLICY_NAME,
};
2 changes: 1 addition & 1 deletion ...ndex_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const testBedConfig: TestBedConfig = {

const initTestBed = registerTestBed(EditPolicy, testBedConfig);

export interface EditPolicyTestBed extends TestBed<TestSubjects> {
export interface EditPolicyTestBed extends TestBed<TestSubjects | string> {
actions: {
setWaitForSnapshotPolicy: (snapshotPolicyName: string) => void;
savePolicy: () => void;
Expand Down
61 changes: 60 additions & 1 deletion ...ns/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ import { setupEnvironment } from '../helpers/setup_environment';
import { EditPolicyTestBed, setup } from './edit_policy.helpers';

import { API_BASE_PATH } from '../../../common/constants';
import { DELETE_PHASE_POLICY, NEW_SNAPSHOT_POLICY_NAME, SNAPSHOT_POLICY_NAME } from './constants';
import {
DELETE_PHASE_POLICY,
NEW_SNAPSHOT_POLICY_NAME,
SNAPSHOT_POLICY_NAME,
POLICY_WITH_NODE_ATTR_AND_OFF_ALLOCATION,
POLICY_WITH_NODE_ROLE_ALLOCATION,
} from './constants';

window.scrollTo = jest.fn();

Expand Down Expand Up @@ -134,4 +140,57 @@ describe('<EditPolicy />', () => {
expect(testBed.find('policiesErrorCallout').exists()).toBeTruthy();
});
});

describe('data allocation', () => {
describe('node roles', () => {
beforeEach(async () => {
httpRequestsMockHelpers.setLoadPolicies([POLICY_WITH_NODE_ROLE_ALLOCATION]);
httpRequestsMockHelpers.setListNodes({
isUsingDeprecatedDataRoleConfig: false,
nodesByAttributes: { test: ['123'] },
nodesByRoles: { data: ['123'] },
});

await act(async () => {
testBed = await setup();
});

const { component } = testBed;
component.update();
});
test('showing "default" type', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I had to read this a couple times to connect it to the assertions being made. Might be clearer if it's something like, "interprets absence of allocate and migrate configurations as default node role allocation".

const { find } = testBed;
expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).toContain(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling find('warm-dataTierAllocationControls.dataTierSelect').text() three times can we store its return value in a variable and refer to that in our assertions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using toContain can we use toBe and be 100% explicit about the expected text value in each assertion, too?

And if we do this then we won't need to the last two assertions, I think?

'recommended'
);
expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).not.toContain(
'Custom'
);
expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).not.toContain('Off');
});
});
describe('node attr and none', () => {
beforeEach(async () => {
httpRequestsMockHelpers.setLoadPolicies([POLICY_WITH_NODE_ATTR_AND_OFF_ALLOCATION]);
httpRequestsMockHelpers.setListNodes({
isUsingDeprecatedDataRoleConfig: false,
nodesByAttributes: { test: ['123'] },
nodesByRoles: { data: ['123'] },
});

await act(async () => {
testBed = await setup();
});

const { component } = testBed;
component.update();
});

test('showing "custom" and "off" types', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar nit here. I think I'd find the intention behind these assertions easier to understand if they were two separate test cases, each with an individual assertion, with descriptions like "interprets presence of allocate config as custom node attribute allocation" and "interprets presence of migrate.enabled configuration as disabling allocation".

const { find } = testBed;
expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).toContain('Custom');
expect(find('cold-dataTierAllocationControls.dataTierSelect').text()).toContain('Off');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use toBe in these cases too?

});
});
});
});
10 changes: 10 additions & 0 deletions ...k/plugins/index_lifecycle_management/__jest__/client_integration/helpers/http_requests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { fakeServer, SinonFakeServer } from 'sinon';
import { API_BASE_PATH } from '../../../common/constants';
import { ListNodesRouteResponse } from '../../../common/types';

export const init = () => {
const server = fakeServer.create();
Expand Down Expand Up @@ -38,8 +39,17 @@ const registerHttpRequestMockHelpers = (server: SinonFakeServer) => {
]);
};

const setListNodes = (body: ListNodesRouteResponse) => {
server.respondWith('GET', `${API_BASE_PATH}/nodes/list`, [
200,
{ 'Content-Type': 'application/json' },
JSON.stringify(body),
]);
};

return {
setLoadPolicies,
setLoadSnapshotPolicies,
setListNodes,
};
};
19 changes: 10 additions & 9 deletions x-pack/plugins/index_lifecycle_management/common/types/policies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ export interface SerializedPhase {
};
}

export interface MigrateAction {
/**
* If enabled is ever set it will probably only be set to `false` because the default value
* for this is `true`. Rather leave unspecified for true when serialising.
*/
enabled: boolean;
}

export interface SerializedHotPhase extends SerializedPhase {
actions: {
rollover?: {
Expand All @@ -59,7 +67,7 @@ export interface SerializedWarmPhase extends SerializedPhase {
set_priority?: {
priority: number | null;
};
migrate?: { enabled: boolean };
migrate?: MigrateAction;
};
}

Expand All @@ -70,7 +78,7 @@ export interface SerializedColdPhase extends SerializedPhase {
set_priority?: {
priority: number | null;
};
migrate?: { enabled: boolean };
migrate?: MigrateAction;
};
}

Expand All @@ -92,13 +100,6 @@ export interface AllocateAction {
require?: {
[attribute: string]: string;
};
migrate?: {
/**
* If enabled is ever set it will only be set to `false` because the default value
* for this is `true`. Rather leave unspecified for true.
*/
enabled: false;
};
}

export interface ForcemergeAction {
Expand Down
23 changes: 14 additions & 9 deletions ...index_lifecycle_management/public/application/lib/data_tiers/determine_allocation_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,33 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { DataTierAllocationType, AllocateAction } from '../../../../common/types';
import { DataTierAllocationType, AllocateAction, MigrateAction } from '../../../../common/types';

/**
* Determine what deserialized state the policy config represents.
*
* See {@DataTierAllocationType} for more information.
*/
export const determineDataTierAllocationType = (
allocateAction?: AllocateAction
actions: {
allocate?: AllocateAction;
migrate?: MigrateAction;
} = {}
): DataTierAllocationType => {
if (!allocateAction) {
return 'default';
}
const { allocate, migrate } = actions;

if (allocateAction.migrate?.enabled === false) {
if (migrate?.enabled === false) {
return 'none';
}

if (!allocate) {
return 'default';
}

if (
(allocateAction.require && Object.keys(allocateAction.require).length) ||
(allocateAction.include && Object.keys(allocateAction.include).length) ||
(allocateAction.exclude && Object.keys(allocateAction.exclude).length)
(allocate.require && Object.keys(allocate.require).length) ||
(allocate.include && Object.keys(allocate.include).length) ||
(allocate.exclude && Object.keys(allocate.exclude).length)
) {
return 'custom';
}
Expand Down
6 changes: 2 additions & 4 deletions x-pack/plugins/index_lifecycle_management/public/application/services/policies/cold_phase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ export const coldPhaseFromES = (phaseSerialized?: SerializedColdPhase): ColdPhas

phase.phaseEnabled = true;

if (phaseSerialized.actions.allocate) {
phase.dataTierAllocationType = determineDataTierAllocationType(
phaseSerialized.actions.allocate
);
if (phaseSerialized.actions) {
phase.dataTierAllocationType = determineDataTierAllocationType(phaseSerialized.actions);
}

if (phaseSerialized.min_age) {
Expand Down
6 changes: 2 additions & 4 deletions x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,8 @@ export const warmPhaseFromES = (phaseSerialized?: SerializedWarmPhase): WarmPhas

phase.phaseEnabled = true;

if (phaseSerialized.actions.allocate) {
phase.dataTierAllocationType = determineDataTierAllocationType(
phaseSerialized.actions.allocate
);
if (phaseSerialized.actions) {
phase.dataTierAllocationType = determineDataTierAllocationType(phaseSerialized.actions);
}

if (phaseSerialized.min_age) {
Expand Down