-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Move ILM out of legacy #61915
Move ILM out of legacy #61915
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
edbb42e
to
8b7712c
Compare
ed09a44
to
184e374
Compare
…dexManagementPluginSetup.
- Convert index management extensions to NP and remove hacks.
184e374
to
48eaad8
Compare
@@ -1,3 +1,6 @@ | |||
// Import the EUI global scope so we can use EUI constants | |||
@import 'src/legacy/ui/public/styles/_styling_constants'; | |||
|
|||
.policyTable__horizontalScrollContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things:
- Selectors should not be directly in
index
files. You'll need another file that the index imports. - You don't need the styling constants imports when importing the SASS in the plugin .ts file. That's automatically added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cchaos! Changes are in-flight to address your comments.
Selectors should not be directly in index files. You'll need another file that the index imports.
Could you explain the problem this solves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it solves the problem of scaleability of the styles. If more UI components and files are added to this plugin, each would get it's own SASS file and get imported via the index file. Then if any are also removed, it's easy to remove the entire folder and a single import line vs having to comb through a list of selectors to find which ones should be removed (or risk keeping dead code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context! That is very helpful.
@cchaos I took a closer look at the styles and they weren't needed so I removed them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed walkthrough @cjcenizal ! I encountered a few anomalies as I was testing and left a number of code comments, none of which need to be blocked on.
Also I am really happy to see a lot of code being removed but it is not super clear where most of the deletions originated from - do you have an idea?
Ran through your instructions and was able to perform almost all the actions you described without running into an issue.
Batch delete multiple policies.
Not sure where in the UI I am able to do this? I expected 👇🏻but I was not able to select multiple policies to delete
click Manage > Retry lifecycle step
For this step, the first click resulted in a 400 Bad Request response - subsequent clicks resulted in a 200 that dismissed the error state. Not sure what caused the transitory error state.
I saw the following EUI warning in regarding MinAgeInput:
@@ -8,6 +8,6 @@ import * as legacyElasticsearch from 'elasticsearch'; | |||
|
|||
const esErrorsParent = legacyElasticsearch.errors._Abstract; | |||
|
|||
export function isEsError(err: Error): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is best to leave explicit annotations for return values, it ensures that in future we don't accidentally return something else from this function.
|
||
async setup({ http }: CoreSetup, { licensing, indexManagement }: Dependencies): Promise<void> { | ||
const router = http.createRouter(); | ||
const config = await this.config$.pipe(first()).toPromise(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change this to:
this.config$.pipe(first()).toPromise().then(() => ...)
Then we can make the setup function synchronous. Also, we will probably need a way for the routes to call getConfig
so they can request config more lazily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I synced with @joshdover and you're right that we should be using sync lifecycle methods, but the platform team expects to introduce a sync config API. It will be easier to migrate the code to use it with the way things are currently set up, so he suggested we leave this as-is for now.
); | ||
|
||
// Per https://www.elastic.co/guide/en/elasticsearch/reference/current/_actions.html | ||
const bodySchema = schema.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on thoroughly documenting these!
@jloleysens Thanks for the review!
I'm a little hazy on this, but I seem to recall the removed styles were originally an attempt to let the table scroll horizontally within its container div, when the window was made narrow enough to occlude it. I tested this and the styles don't have a discernible effect now: So I decided to remove them. Without them, the table responds nicely to the narrow window. It's missing labels for the content in each card, so I created #63189 to track this bug. I discovered that the API tests used the getTemplate API. I have to assume that either this endpoint was created to support this tests or it was written in anticipation of a use case that never materialized, and along the way we ended up using it in the test. These are just guesses. Bill originally built ILM and he said it was probably dead code when I asked him about it.
Great catch! This isn't possible via the UI, but the API is written in a way that implies it's possible. 🤷♂️
I will dig into this, thank you for calling it out.
I was able to repro this in master so I'm going to exclude it from this PR's scope. |
I was able to reproduce this and the full error message indicates that the index no longer has an ILM error associated with it when you retry the action: {
"statusCode": 400,
"error": "Bad Request",
"message": "[illegal_argument_exception] cannot retry an action for an index [kibana_sample_data_ecommerce] that has not encountered an error when running a Lifecycle Policy"
} It seems like the error is transitory. If I repeatedly reload the indices, the error will appear, disappear, repeat. So my guess is that the UI state is just stale when we click the "Retry" button. I was able to reproduce the behavior you mentioned and this transitory behavior in master, so I don't think we need to worry about it in this PR. |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Rename IndexMgmtSetup to IndexManagementPluginSetup. * Remove unused fetch index template route and related tests. * Remove unnecessary custom styles.
Other changes
Testing
Run ES with a custom node attribute so you can configure allocation and test the
node
API.Settings
With the above setting, verify that ILM isn't visible in the UI and you can't filter by lifecycle status or phase in Index Management.
With the above setting, verify the same effect.
With the above setting, verify that the
r1
node attribute isn't available for selection under the warm phase's shard allocation when creating a policy.Create index lifecycle policy
Create variations of an index lifecycle policy, ensuring that the API validation is appropriately lenient and correctly typed.
Phase variations
Hot phase variations
Warm phase variations
Note: All of these variations assume the hot phase is configured with rollover enabled.
r1
node attribute selected for shard allocation. Before you save this policy, click the "View list of nodes attached to this configuration" button and verify that information shows up in the flyout.Cold phase variations
Note: All of these variations assume the hot phase is configured with rollover enabled.
r1
node attribute selected for shard allocation.Delete phase variations
None, because this phase can't be configured. As long as you test that a policy can be created with the phase enabled, we're good.
Actions
Index Management
Test that the
Frozen
badge, phase filtering, and lifecycle information is surfaced in Index Management by running this series of requests in Console:Then go into Index Management and, after about 1 minute, verify that there's a frozen index and that you can filter by the various lifecycle phases and statuses.
Next, add the Kibana sample data and attach the
full
policy to the index that gets created. After about a minute, there should be an error on this index. Click the index, verify that there's ILM information in the detail panel as well as an error, and clickManage > Retry lifecycle step
. Make sure this action dismisses the error.Index and index template actions