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

jloleysens · 2020-10-26T16:36:07Z

Summary

Fix for correctly detecting when migration.enabled = false has been set in an ILM policy (introduced in this PR #76126)

This bug will cause policies with "migrate.enabled: false" set to not be correctly detected in the UI. Users are still able set this value in the UI, but they are not able to see it as being again on subsequent viewings of the policy with "migrate.enabled=false" set.

Gif of fixed behaviour

Sample JSON for creating a policy with migration "off"

curl --request PUT \
  --url http://localhost:9200/_ilm/policy/my-test-policy-1 \
  --header 'authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
  --header 'content-type: application/json' \
  --data '{
  "policy": {
    "phases": {
      "hot": {
        "min_age": "0ms",
        "actions": {
          "rollover": {
            "max_size": "50gb",
            "max_age": "30d"
          }
        }
      },
      "warm": {
        "min_age": "0d",
        "actions": {
          "migrate": {
            "enabled": false
          }
        }
      },
      "cold": {
        "min_age": "0d",
        "actions": {}
      },
      "delete": {
        "min_age": "90d",
        "actions": {
          "delete": {
            "delete_searchable_snapshot": true
          }
        }
      }
    }
  }
}'

Checklist

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

- with jest tests see origin here: elastic#81642

cjcenizal

Tested locally, code LGTM. I'll merge this once CI is green so it can ship with 7.10. @jloleysens I think the comments I left are worth considering incorporating into master and 7.x.

cjcenizal · 2020-10-26T16:49:03Z

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

+      });
+      test('showing "default" type', () => {
+        const { find } = testBed;
+        expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).toContain(


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?

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?

cjcenizal · 2020-10-26T16:59:35Z

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

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


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".

cjcenizal · 2020-10-26T17:06:16Z

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

+        component.update();
+      });
+
+      test('showing "custom" and "off" types', () => {


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".

cjcenizal · 2020-10-26T17:07:52Z

...ugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts

+      test('showing "custom" and "off" types', () => {
+        const { find } = testBed;
+        expect(find('warm-dataTierAllocationControls.dataTierSelect').text()).toContain('Custom');
+        expect(find('cold-dataTierAllocationControls.dataTierSelect').text()).toContain('Off');


Can we use toBe in these cases too?

kibanamachine · 2020-10-26T18:06:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 30e2c60

Metrics [docs]

async chunks size

id	before	after	diff
`indexLifecycleManagement`	236.2KB	236.2KB	-58.0B

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

elasticmachine · 2020-10-26T19:40:57Z

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

* migrate all fields on warm phase except, data alloc, replicas and shrink * introduce edit policy context to share original policy and migrate shrink and replicas fields * Refactored biggest field; data allocation Copied the entire field for now duplicating all of the components * remove unused import * complete migration of new serialization * Remove last vestiges of legacy warm phase - also removed policy serialization tests for warm phase * fix existing test coverage and remove use of "none" for node attribute * added policy serialization tests * remove unused translations * Fix use of useFormData after update - also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes. * fix import path * simplify serialization snapshot tests * type phases: string -> phases: Phases * Addressed some PR review items - refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options. * updated data tier callout logic after new changes * getPolicy -> updatePolicy Also rather deconstruct the validate fn from the form object * fix detection of migrate false and refactor serialization to pure function * fix type issue * fix for correctly detecting policy data tier type - with jest tests see origin here: #81642 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* migrate all fields on warm phase except, data alloc, replicas and shrink * introduce edit policy context to share original policy and migrate shrink and replicas fields * Refactored biggest field; data allocation Copied the entire field for now duplicating all of the components * remove unused import * complete migration of new serialization * Remove last vestiges of legacy warm phase - also removed policy serialization tests for warm phase * fix existing test coverage and remove use of "none" for node attribute * added policy serialization tests * remove unused translations * Fix use of useFormData after update - also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes. * fix import path * simplify serialization snapshot tests * type phases: string -> phases: Phases * Addressed some PR review items - refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options. * updated data tier callout logic after new changes * getPolicy -> updatePolicy Also rather deconstruct the validate fn from the form object * fix detection of migrate false and refactor serialization to pure function * fix type issue * fix for correctly detecting policy data tier type - with jest tests see origin here: elastic#81642 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* migrate all fields on warm phase except, data alloc, replicas and shrink * introduce edit policy context to share original policy and migrate shrink and replicas fields * Refactored biggest field; data allocation Copied the entire field for now duplicating all of the components * remove unused import * complete migration of new serialization * Remove last vestiges of legacy warm phase - also removed policy serialization tests for warm phase * fix existing test coverage and remove use of "none" for node attribute * added policy serialization tests * remove unused translations * Fix use of useFormData after update - also minor refactor to use useCallback in policy flyout now that getFormData changes when the form data changes. * fix import path * simplify serialization snapshot tests * type phases: string -> phases: Phases * Addressed some PR review items - refactor toggle click to take a boolean arg - refactor selection options in data tier component to use a func to get select options. * updated data tier callout logic after new changes * getPolicy -> updatePolicy Also rather deconstruct the validate fn from the form object * fix detection of migrate false and refactor serialization to pure function * fix type issue * fix for correctly detecting policy data tier type - with jest tests see origin here: #81642 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jloleysens added 2 commits October 26, 2020 17:00

correctly detect migrate.enabled === false!

0a63d9a

added jest test

30e2c60

jloleysens added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.10.1 labels Oct 26, 2020

jloleysens requested a review from cjcenizal October 26, 2020 16:36

jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2020

fix for correctly detecting policy data tier type

b85f492

- with jest tests see origin here: elastic#81642

cjcenizal approved these changes Oct 26, 2020

View reviewed changes

cjcenizal added bug Fixes for quality problems that affect the customer experience Feature:ILM regression Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Oct 26, 2020

cjcenizal merged commit 595e59e into elastic:7.10 Oct 26, 2020

jloleysens deleted the fix/migrate-action-deserialization branch October 27, 2020 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jloleysens commented Oct 26, 2020 •

edited

Loading

cjcenizal left a comment

cjcenizal Oct 26, 2020

cjcenizal Oct 26, 2020

cjcenizal Oct 26, 2020

cjcenizal Oct 26, 2020

cjcenizal Oct 26, 2020

kibanamachine commented Oct 26, 2020

elasticmachine commented Oct 26, 2020

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

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

Conversation

jloleysens commented Oct 26, 2020 • edited Loading

Summary

Gif of fixed behaviour

Sample JSON for creating a policy with migration "off"

Checklist

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

cjcenizal Oct 26, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 26, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

elasticmachine commented Oct 26, 2020

jloleysens commented Oct 26, 2020 •

edited

Loading