-
Notifications
You must be signed in to change notification settings - Fork 25k
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] Remove ILM policy from fleet files indices and make them hidden #94651
[Fleet] Remove ILM policy from fleet files indices and make them hidden #94651
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
…53499) ## Summary We are having to remove the ILM policies that delete data after 30 days from 8.7.0 in elastic/elasticsearch#94651, so removing this text as well: "Fleet will automatically remove old diagnostics files after 30 days." How they look now: <img width="769" alt="Screenshot 2023-03-22 at 20 57 40" src="https://user-images.githubusercontent.com/3315046/227036926-29a12823-002f-483d-8ead-5bf759bdb7d4.png"> <img width="1247" alt="Screenshot 2023-03-22 at 20 57 33" src="https://user-images.githubusercontent.com/3315046/227036928-1dd4ae35-8d9d-4630-a769-33b943d19269.png">
…astic#153499) ## Summary We are having to remove the ILM policies that delete data after 30 days from 8.7.0 in elastic/elasticsearch#94651, so removing this text as well: "Fleet will automatically remove old diagnostics files after 30 days." How they look now: <img width="769" alt="Screenshot 2023-03-22 at 20 57 40" src="https://user-images.githubusercontent.com/3315046/227036926-29a12823-002f-483d-8ead-5bf759bdb7d4.png"> <img width="1247" alt="Screenshot 2023-03-22 at 20 57 33" src="https://user-images.githubusercontent.com/3315046/227036928-1dd4ae35-8d9d-4630-a769-33b943d19269.png"> (cherry picked from commit a3fa269)
…ys (#153499) (#153502) # Backport This will backport the following commits from `main` to `8.7`: - [[Fleet] Remove text saying file uploads are deleted after 30 days (#153499)](#153499) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Mark Hopkin","email":"mark.hopkin@elastic.co"},"sourceCommit":{"committedDate":"2023-03-22T22:05:14Z","message":"[Fleet] Remove text saying file uploads are deleted after 30 days (#153499)\n\n## Summary\r\n\r\nWe are having to remove the ILM policies that delete data after 30 days\r\nfrom 8.7.0 in elastic/elasticsearch#94651, so\r\nremoving this text as well: \"Fleet will automatically remove old\r\ndiagnostics files after 30 days.\"\r\n\r\nHow they look now:\r\n<img width=\"769\" alt=\"Screenshot 2023-03-22 at 20 57 40\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036926-29a12823-002f-483d-8ead-5bf759bdb7d4.png\">\r\n<img width=\"1247\" alt=\"Screenshot 2023-03-22 at 20 57 33\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036928-1dd4ae35-8d9d-4630-a769-33b943d19269.png\">","sha":"a3fa269e262f68d9d5bde7b6206f8f6c1e9ad834","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v8.7.0","v8.8.0"],"number":153499,"url":"https://github.com/elastic/kibana/pull/153499","mergeCommit":{"message":"[Fleet] Remove text saying file uploads are deleted after 30 days (#153499)\n\n## Summary\r\n\r\nWe are having to remove the ILM policies that delete data after 30 days\r\nfrom 8.7.0 in elastic/elasticsearch#94651, so\r\nremoving this text as well: \"Fleet will automatically remove old\r\ndiagnostics files after 30 days.\"\r\n\r\nHow they look now:\r\n<img width=\"769\" alt=\"Screenshot 2023-03-22 at 20 57 40\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036926-29a12823-002f-483d-8ead-5bf759bdb7d4.png\">\r\n<img width=\"1247\" alt=\"Screenshot 2023-03-22 at 20 57 33\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036928-1dd4ae35-8d9d-4630-a769-33b943d19269.png\">","sha":"a3fa269e262f68d9d5bde7b6206f8f6c1e9ad834"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/153499","number":153499,"mergeCommit":{"message":"[Fleet] Remove text saying file uploads are deleted after 30 days (#153499)\n\n## Summary\r\n\r\nWe are having to remove the ILM policies that delete data after 30 days\r\nfrom 8.7.0 in elastic/elasticsearch#94651, so\r\nremoving this text as well: \"Fleet will automatically remove old\r\ndiagnostics files after 30 days.\"\r\n\r\nHow they look now:\r\n<img width=\"769\" alt=\"Screenshot 2023-03-22 at 20 57 40\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036926-29a12823-002f-483d-8ead-5bf759bdb7d4.png\">\r\n<img width=\"1247\" alt=\"Screenshot 2023-03-22 at 20 57 33\"\r\nsrc=\"https://user-images.githubusercontent.com/3315046/227036928-1dd4ae35-8d9d-4630-a769-33b943d19269.png\">","sha":"a3fa269e262f68d9d5bde7b6206f8f6c1e9ad834"}}]}] BACKPORT--> Co-authored-by: Mark Hopkin <mark.hopkin@elastic.co>
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.
If this code had already gotten out into production, you would need some kind of migration to fix up existing indices created from these templates, but since the affected code isn't on the 8.6 branch, this won't be a problem.
Should we remove the ILM policies .fleet-file-data-ilm-policy
and .fleet-files-ilm-policy
as well? Or would they be unchanged in the full fix?
@williamrandolph we will need the ILM policies for the full fix, if removing them prevents us from shipping that in 8.7.x I'd rather keep them there if possible |
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 working on this Mark
I left a few comments.
@@ -10,8 +10,8 @@ | |||
}, | |||
"template": { | |||
"settings": { | |||
"index.lifecycle.name": ".fleet-files-ilm-policy", |
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.
Is the fleet-file-data-ilm-policy.json
policy still needed?
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.
we will need the ILM policies for the full fix, if removing them prevents us from shipping that in 8.7.x I'd rather keep them there if possible?
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.
Can you expand a bit on what the full fix is?
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.
Looks like the plan is here: elastic/kibana#153504
In order to set the setting on all rolled over indices, we will have to have an index template per integration. E,g instead of
.fleet-files-*
we will have to have one for.fleet-files-endpoint-*
and.fleet-files-agent-*
. This could be created dynamically by fleet, we already have the logic for creating the backing indices at integration install time and fleet start time, see here.We can still use the ILM policies hard coded into elasticsearch.
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.
Sure, its not 100% as we only discovered this yesterday, but we will likely be looking to have kibana create the index templates at package install time, which will then refer to these ILM policies. For now removing ILM from the current index template means the feature still works.
Our issue at the moment is that we were trying to use one index template for all of our integrations that will be using this feature, but as each integration has a different alias, we aren't able to set the ilm rollover_alias
on the index template, we thought setting it on the first write index works but we were wrong.
Moving to kibana creating the index templates will mean we wont have to have an index template for each integration in elasticsearch repo.
I have an issue for the long term fix here elastic/kibana#153504
@@ -10,8 +10,8 @@ | |||
}, | |||
"template": { | |||
"settings": { | |||
"index.lifecycle.name": ".fleet-files-ilm-policy", | |||
"index.auto_expand_replicas": "0-1" | |||
"index.auto_expand_replicas": "0-1", |
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 this will not have effect for deployments updating to the version where this change is released.
See https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/IndexTemplateRegistry.java#L296
You'll have to bump the template version in FleetTemplateRegistry
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/fleet/src/main/java/org/elasticsearch/xpack/fleet/FleetTemplateRegistry.java#L27
and please also document the change next to the new version
e.g.
Line 38 in 3692354
// version 5: convert to data stream |
Please also test the branch as a standalone elasticsearch run and upgrade from a previous version to this branch to see that the templates and your code behaves as expected.
You can build a tarball using ./gradlew clean :distribution:archives:darwin-tar:assemble
(change distribution accordingly) or run elasticsearch using ./gradlew run
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.
These templates were added in 8.7 so nobody should have these changes yet, so is this required?
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.
Ah gotcha, then we should be fine here
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.
My understanding is that there are no such deployments, since these templates are being newly introduced in 8.7: #91413
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.
That PR seems so distant, I can't believe it's targeting 8.7
and not an older version :)
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.
Ha it sat blocked on another team's effort for about a month if I recall. This file upload feature was slotted for 8.6 initially but was eventually punted, with a bulk of the work landing early on in the 8.7 development cycle.
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.
LGTM, thanks Mark
💚 Backport successful
|
…en (elastic#94651) * Remove ILM policy from files indices and make them hidden * use correct setting for hidden and auto_expand * fix tests * remove unused imports
Closes #94647
Part of elastic/kibana#153483.
There is a problem with the way we set up the indices that these ILM policies affect, causing the error below, it's too late to implement a full fix in 8.7.0 so for now we are removing the policy from the file upload indices.
In investgating the issue I also noticed that the index templates do not hide the indices so I have added
hidden: true
also.Testing steps
To check that diagnostic upload is still working:
gradlew run