Skip to content

Filter enrich policy index deletes to just the policy's associated indices #82568

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

Merged
merged 7 commits into from
Jan 14, 2022

Conversation

joegallo
Copy link
Contributor

@joegallo joegallo commented Jan 13, 2022

If you have two enrich policies, and the name of the one plus a dash is a prefix for the name of the other (e.g. "policy-test" and "policy-test-more"), then if you delete the prefix policy the indices associated with the other policy will get deleted, too.

If you delete a policy whose name is a prefix of another policy, it
shouldn't delete the indices that are associated with that policy.
Either generating an index name from a policy name, or asking whether
a given index name would have been generated from a given policy name.
They need to actually match the naming pattern relationship between a
policy and a index.
@joegallo joegallo added >bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Jan 13, 2022
@joegallo joegallo requested a review from martijnvg January 13, 2022 18:10
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jan 13, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@joegallo
Copy link
Contributor Author

I did a little bit of spelunking back to when enrich was first added to master via #48039.

    private void prepareAndCreateEnrichIndex() {
         long nowTimestamp = nowSupplier.getAsLong();
         String enrichIndexName = EnrichPolicy.getBaseName(policyName) + "-" + nowTimestamp;
         Settings enrichIndexSettings = Settings.builder()
     public static String getBaseName(String policyName) {
         return ENRICH_INDEX_NAME_BASE + policyName;
     }
    public static final String ENRICH_INDEX_NAME_BASE = ".enrich-";

The important thing is that it's always been the case that the pattern is prefix-policy_name-timestamp -- if that had ever changed, then there'd maybe be some concern here that I was making it possible that some backing indices would never get deleted (like, say, if the pattern had at one point been prefix-policy_name-BACKING-timestamp -- the current code would have still deleted that, but my new code would not).

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM, good catch!

@@ -82,12 +83,13 @@ protected void masterOperation(
ClusterState state,
ActionListener<AcknowledgedResponse> listener
) throws Exception {
EnrichPolicy policy = EnrichStore.getPolicy(request.getName(), state); // ensure the policy exists first
String policyName = request.getName();
EnrichPolicy policy = EnrichStore.getPolicy(policyName, state); // ensure the policy exists first
Copy link
Member

Choose a reason for hiding this comment

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

maybe make these variables explicitly immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅, d1658cf

@joegallo joegallo merged commit 6ff8b7b into elastic:master Jan 14, 2022
@joegallo joegallo deleted the enrich-policy-delete-prefix branch January 14, 2022 15:49
@joegallo
Copy link
Contributor Author

@hendry-lim hasn't it? I'm seeing bae4e11 for 8.0 and 6bab477 for 7.17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants