-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Convert ILM and SLM histories into hidden indices #51456
Conversation
Modifies SLM's and ILM's history indices to be hidden indices for added protection against accidental querying and deletion. Also modifies the REST test cleanup to delete hidden indices.
Pinging @elastic/es-core-features (:Core/Features/ILM+SLM) |
@dimitris-athanasiou You asked a few days ago whether |
Adding template upgrading in |
BWC tests aren't enabled right now, and they were failing earlier due to some issues cleaning up hidden indices - I've got a local run going, but I'm not going to merge this until I get a green BWC run from CI. This is otherwise ready for review, @dakrone & @andreidan I originally mentioned an upgrade test - I haven't written one yet, but covered that functionality in unit tests. Do you think there's a need for additional tests 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.
LGTM, thanks Gordon! I left a question about some ordering, but it shouldn't be anything major.
@@ -59,9 +59,10 @@ public static void loadTemplateIntoMap(String resource, Map<String, IndexTemplat | |||
public static String loadTemplate(String resource, String version, String versionProperty) { | |||
try { | |||
BytesReference source = load(resource); | |||
validate(source); | |||
final String filteredJson = filter(source, version, versionProperty); | |||
validate(new BytesArray(filteredJson)); |
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'm curious about this change, why the change to validation post-filter? Was it causing an issue?
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, bare placeholders (e.g. "version": ${my.variable}
) didn't work with the validation before the variable replacement because that's not valid JSON, and version must be a bare integer, not a string. Previously, all variables were strings (e.g. "field": "${my.variable}"
, note the quotes around the variable).
Besides, it's more important that the JSON is valid after we do the variable replacement, rather than before, isn't it?
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.
The name "filter" makes it seem like the filteredJson
is a subset of the original, in which case I would think it makes sense to validate the entire JSON instead of a subset. With your explanation it definitely makes sense (the name is just misleading).
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.
It is a misleading name - I would have taken the opportunity to change it, but Dimitris is going to do so in #51765, as part of modifying IndexTemplateRegistry
(and associated classes) to allow other variable substitutions in addition to the version.
I'm very curious how these changes are causing some tests in |
Oh, for some reason the cleanup in |
I discovered an issue with the dot-index deprecation warnings (fix in #52087) while working on this, and this needs a couple minor updates after that fix goes into master. |
Ok, #52087 has been merged and I've updated this PR to work with those changes. I'll merge this as soon as we get a green CI run on the latest changes. |
Modifies SLM's and ILM's history indices to be hidden indices for added protection against accidental querying and deletion, and improves IndexTemplateRegistry to handle upgrading index templates. Also modifies the REST test cleanup to delete hidden indices.
Modifies SLM's and ILM's history indices to be hidden indices for added protection against accidental querying and deletion, and improves IndexTemplateRegistry to handle upgrading index templates. Also modifies the REST test cleanup to delete hidden indices.
After elastic#51456 `IndexTemplateRegistry` is updating tempaltes and ILM policies if necessary. However, even if the upgrade succeeds we keep trying. This commit fixes the issue by setting the `creationCheck` on success.
Modifies SLM's and ILM's history indices to be hidden indices for added
protection against accidental querying and deletion, and improves
IndexTemplateRegistry
to handle upgrading index templates.Also modifies the REST test cleanup to delete hidden indices.