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.

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] Add support for frozen phase in UI #75968

Merged
merged 10 commits into from
Aug 31, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Aug 26, 2020

Summary

Support configuring the frozen phase in both server and UI of ILM. This also updates the index management plugins extension to support querying for "frozen" phase in the index table.

How to test

  1. Create a new ILM policy that consists of Hot and Frozen phases only (to simplify testing). In the UI, make sure that you use all options for frozen phase except for setting required replicas as this will cause the phase to get stuck there if the cluster cannot create a replica of the shards.
  2. Follow this tutorial to set up an index template that will use the ILM policy and create an initial write index (making sure to keep your naming consistent). Make sure to set any timing values to one second (1 second) so that you do not have to wait for phase rollover too long.
  3. Run the following in Dev Tools to make ILM check progress more frequently:
PUT _cluster/settings
{
  "transient": {
    "indices.lifecycle.poll_interval": "5s"  
  }
}

Make sure to set this back to the default of 10m so that ILM doesn't continue doing unnecessary work after testing.

What you should see in index management is new indices being created every 5 seconds as rollover happens and they should have the "Frozen" label applied to them in the index management UI.

Fix elastic/elasticsearch#61345

Screenshots

Screenshot 2020-08-26 at 12 56 47

Checklist

Delete any items that are not applicable to this PR.

This is a very faithful duplication of the cold phase. We are
also excluding the snapshot action as well as the unfollow action
as these are features that require higher than basic license
privilege.
@jloleysens jloleysens added chore Feature:ILM Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes labels Aug 26, 2020
@jloleysens jloleysens requested a review from a team as a code owner August 26, 2020 10:57
@elasticmachine
Copy link
Contributor

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

@jloleysens jloleysens requested review from yuliacech and removed request for cjcenizal August 27, 2020 06:54
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech
Copy link
Contributor

Hi @jloleysens ,
I tested the code locally with a data stream and the frozen phase worked for me.
I noticed some issues with validation and typings, I will add comments below.

<Fragment>
{phaseData.phaseEnabled ? (
<Fragment>
<MinAgeInput<FrozenPhaseInterface>
Copy link
Contributor

Choose a reason for hiding this comment

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

MinAgeInput's props are currently typed to accept warm, cold and delete phases. I'm not sure why the type check is not picking it up, but it might be good to add frozen phase to props generics.

/>
<EuiSpacer />

<NodeAllocation<FrozenPhaseInterface>
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar here, the props generics don't include frozen phase, but type check is not showing it.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

jloleysens commented Aug 27, 2020

@yuliacech Thanks for the review and great catch on frozen phase validation! I've addressed your PR feedback, would you mind taking another look?

I did a slight restructure to the phase types. Because there is so much overlap I thought it would be easiest to use inheritance for the phase interfaces. So now when we create a new "shared" form UI component when can just extend the relevant phases and structurally the phases that extend that type should fit into the component type requirements. Let me know what you think!

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Hi @jloleysens ,
Great addition to policy types with common phase interfaces! Tested locally and all looks good to me.
I just found a copy-paste error from my commit, so I wont block you on it. But if you want to fix it with this PR, here is the info: both validateColdPhase and validateFrozenPhase when checking phase.selectedMinimumAge actually setting phaseErrors.phaseIndexPriority.

@jloleysens
Copy link
Contributor Author

Thanks Yulia! I will include the fix and merge once CI is green.

@jloleysens jloleysens mentioned this pull request Aug 27, 2020
11 tasks
}
}

if (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see these kinds of rules extracted into functions that have descriptive names, a big block comment explaining the intention (with links to relevant docs), and then covered with unit tests to define the desired behavior. I feel like this would make the code easier to scan, and it'd also be easier to dig into them to understand the why behind these rules. Anybody else feel similarly?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be indeed very helpful 👍

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 agree, I think we should also move the files to directory called lib in that process that lives in application. For now, the logic is a carbon-copy of that for cold phase, so at least we know it has been working. I will open an issue for this!

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
indexLifecycleManagement 159 +2 157

async chunks size

id value diff baseline
indexLifecycleManagement 276.0KB +16.1KB 259.9KB

page load bundle size

id value diff baseline
indexLifecycleManagement 230.2KB +430.0B 229.8KB

History

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

@jloleysens jloleysens merged commit aac57fb into elastic:master Aug 31, 2020
@jloleysens jloleysens deleted the ilm/add-frozen-phase branch August 31, 2020 09:46
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 31, 2020
* ILM public side changes to support frozen phase

This is a very faithful duplication of the cold phase. We are
also excluding the snapshot action as well as the unfollow action
as these are features that require higher than basic license
privilege.

* added "frozen" to the server side schema

* add frozen phases component

* fix i18n and update jest tests

* Slight restructuring to phase types and fix copy paste issues.

- also fixed TS error introduced after TS v4 upgrade (delete)

* fix hot phase type and remove type constraint from error messages

* update validation logic

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 31, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (43 commits)
  [APM] Chart units don't update when toggling the chart legends (elastic#74931)
  [ILM] Add support for frozen phase in UI (elastic#75968)
  Hides advanced json for count metric (elastic#74636)
  add client-side feature usage API (elastic#75486)
  [Maps] add drilldown support map embeddable (elastic#75598)
  [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106)
  [Resolver] model `location.search` in redux (elastic#76140)
  [APM] Prevent imports of public in server code (elastic#75979)
  fix eslint issue
  skip flaky suite (elastic#76223)
  [APM] Transaction duration anomaly alerting integration (elastic#75719)
  [Transforms] Avoid using "Are you sure" (elastic#75932)
  [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145)
  [plugin-helpers] improve 3rd party KP plugin support (elastic#75019)
  [docs/getting-started] link to yarn v1 specifically (elastic#76169)
  [Security_Solution][Resolver] Resolver loading and error state (elastic#75600)
  Fixes App Search documentation links (elastic#76133)
  Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079)
  [Resolver] Fix useSelector usage (elastic#76129)
  [Enterprise Search] Migrate util and components from ent-search (elastic#76051)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts
#	x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts
jloleysens added a commit that referenced this pull request Aug 31, 2020
* ILM public side changes to support frozen phase

This is a very faithful duplication of the cold phase. We are
also excluding the snapshot action as well as the unfollow action
as these are features that require higher than basic license
privilege.

* added "frozen" to the server side schema

* add frozen phases component

* fix i18n and update jest tests

* Slight restructuring to phase types and fix copy paste issues.

- also fixed TS error introduced after TS v4 upgrade (delete)

* fix hot phase type and remove type constraint from error messages

* update validation logic

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:ILM release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] Support "frozen" phase in ILM UI
6 participants