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

[Fleet] add ilm policy per data stream #85492

Merged

Conversation

nnamdifrankie
Copy link
Contributor

@nnamdifrankie nnamdifrankie commented Dec 9, 2020

Summary

This PR adds the installation flow for ilm_policy per data_stream. This feature will be used to define a custom ilm_policy for a data_stream that will override the one taken based on type.

Example package PR that will use this field: elastic/endpoint-package#118

A screenshot showing the ILM policy that's installed from the Endpoint package.
image

Here's the Index Template showing that it's lifecycle points to the installed ILM policy
image

  • add ilm_policy for data streams and override the default type

Checklist

Delete any items that are not applicable to this PR.

@@ -30,6 +30,7 @@ import { deleteKibanaSavedObjectsAssets } from './remove';
import { installTransform } from '../elasticsearch/transform/install';
import { createInstallation, saveKibanaAssetsRefs, updateVersion } from './install';
import { saveArchiveEntries } from '../archive/save_to_es';
import { installIlmForDataStream } from '../elasticsearch/datastream_ilm/install';
Copy link
Contributor

Choose a reason for hiding this comment

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

is this file missing from the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, updated

@@ -24,6 +24,7 @@ import { packagePolicyService, appContextService } from '../..';
import { splitPkgKey } from '../registry';
import { deletePackageCache } from '../archive';
import { removeArchiveEntries } from '../archive/save_to_es';
import { deleteIlms } from '../elasticsearch/datastream_ilm/remove';
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, is this file missing from the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, updated

@ruflin ruflin added the Team:Fleet Team label for Observability Data Collection Fleet team label Dec 10, 2020
@ruflin
Copy link
Contributor

ruflin commented Dec 10, 2020

Again, would have preferred to have 2 separate PR's for ilm policy and hidden to keep the features separate.

@ruflin
Copy link
Contributor

ruflin commented Dec 10, 2020

What is the name of the ILM policies that are loaded?

@kevinlog
Copy link
Contributor

kevinlog commented Dec 11, 2020

@ruflin

What is the name of the ILM policies that are loaded?

Currently, the one included in the data_stream for the endpoint package will be this: logs-endpoint.collection-diagnostics

Screenshot from running the code + package locally:
image

There's another example in a couple of the tests.
https://github.com/elastic/kibana/pull/85492/files#diff-fa07cac51b6a49bf1e4824bc2250c9a77dac6c7d6b0a56020f559ef1ff9be25fR463

I don't have a strong opinion on the naming, happy to change it.

cc/ @tsg

@kevinlog kevinlog changed the title add hidden datastream and ilm policy [Fleet] add ilm policy per data stream Dec 11, 2020
@kevinlog kevinlog marked this pull request as ready for review December 11, 2020 19:11
@kevinlog kevinlog requested a review from a team December 11, 2020 19:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@kevinlog kevinlog added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 11, 2020
@kevinlog
Copy link
Contributor

@ruflin

Again, would have preferred to have 2 separate PR's for ilm policy and hidden to keep the features separate.

I opened this PR to separate ilm_policy and hidden #85703

@ruflin
Copy link
Contributor

ruflin commented Dec 14, 2020

@kevinlog On the naming, I would assume the ilm policy has a similar / same name as the data stream(s) it applies to?

@ruflin ruflin requested a review from skh December 14, 2020 08:23
@kevinlog
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

Happy with the code as long as @ruflin is happy with the implemented functionality.

@skh skh requested a review from ruflin December 14, 2020 13:14
@kevinlog
Copy link
Contributor

@ruflin

@ kevinlog On the naming, I would assume the ilm policy has a similar / same name as the data stream(s) it applies to?

The names are similar, however we can change it so that they are the same.

Index template: logs-endpoint.diagnostics.collection

current ILM Policy name: logs-endpoint.collection-diagnostics
suggested ILM Policy name: logs-endpoint.diagnostics-collection

Let me know if the above looks good. Happy to make adjustments

@ruflin
Copy link
Contributor

ruflin commented Dec 14, 2020

I think from a user perspective it would be easier if the name would be indentical so logs-endpoint.diagnostics.collection but I guess I miss why there is a variation.

@kevinlog
Copy link
Contributor

@ruflin

I think from a user perspective it would be easier if the name would be indentical so logs-endpoint.diagnostics.collection but I guess I miss why there is a variation.

I tend to agree with you, however the existing ilm_policy's I see all use - as opposed to . I don't know if there's a reason.

@ruflin
Copy link
Contributor

ruflin commented Dec 14, 2020

@kevinlog Are these ILM policies related to Packages / New Indexing Strategy?

@kevinlog
Copy link
Contributor

@ruflin

@ kevinlog Are these ILM policies related to Packages / New Indexing Strategy?

yes, these ILM policies will be shipped with the package, here's an example with the Endpoint package: elastic/endpoint-package#118

@kevinlog
Copy link
Contributor

update from the Fleet sync - we'll merge this as-is. We can continue to talk about the naming after FF and before 7.11 ships and make adjustments.

cc/ @ph @ruflin @skh

@kevinlog kevinlog requested a review from pzl December 14, 2020 16:22
Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Some questions about the regex patterns.

}

const isDataStreamIlm = (path: string) => {
return new RegExp('(?<package>.*)/data_stream/(?<dataset>.*)/elasticsearch/ilm/*.*').test(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we need the ?<package> or the ?<dataset> ? I believe those are so we can extract out those specific fields.

Do we need to escape all the /?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions

Similarly, if you're writing a regular expression literal and need to match a slash ("/"), you need to escape that (otherwise, it terminates the pattern). For instance, to search for the string "/example/" followed by one or more alphabetic characters, you'd use //example/[a-z]+/i—the backslashes before each slash make them literal.

I was playing around with it here:
https://regex101.com/r/GLnjJN/1
https://regex101.com/r/GLnjJN/2

I think we might need one of these:

(?<package>.*)\/data_stream\/(?<dataset>.*)\/elasticsearch\/ilm\/.*
.*\/data_stream\/.*\/elasticsearch\/ilm\/.*

Copy link
Member

Choose a reason for hiding this comment

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

Do we need? No, probably not. (?<package>.*) vs .* is adding a named capture group.

It doesn't look like we access or use the groups, so that's unnecessary. Adding the label, though, somewhat acts as in-regex-documentation of what we're looking at. I do like that.

We do not need to escape the slashes here because it is a new Regexp constructor using a string. Not using slash-literals.

};

const isDatasetIlm = (path: string, datasetName: string) => {
return new RegExp(`(?<package>.*)/data_stream\\/${datasetName}/elasticsearch/ilm/*.*`).test(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions about groups and escaping as above

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 950.4KB 950.5KB +48.0B

Distributable file count

id before after diff
default 47130 47892 +762

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 376.5KB 376.5KB +71.0B

History

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

@kevinlog kevinlog merged commit 23c5daa into elastic:master Dec 14, 2020
kevinlog added a commit to kevinlog/kibana that referenced this pull request Dec 14, 2020
Co-authored-by: kevinlog <kevin.logan@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kevinlog added a commit that referenced this pull request Dec 14, 2020
Co-authored-by: kevinlog <kevin.logan@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: nnamdifrankie <56440728+nnamdifrankie@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants