Add API for resetting state of a `SystemIndexPlugin` #69469

williamrandolph · 2021-02-23T17:26:56Z

When we disable access to system indices, plugins will still need a way to erase their state. The obvious and most pressing use case for this is in tests, which need to be able to clean up the state of a cluster in between groups of tests.

In this first draft of the feature state reset API, a user with snapshot admin privileges POSTs a request to /_reset_feature_state. The node handling the endpoint then iterates through a list of features and executes their cleanup callbacks. By default, these callbacks use a client and a clusterState to delete all system indexes and associated indexes for a feature. The endpoint then returns a list of objects that include a feature name and a report of whether the reset operation succeeded or failed.

My initial cut used a TransportMasterNodeAction, which requires code that carefully manipulates cluster state. At least for the first cut and testing, it seems like it will be much easier to use a client within a HandledTransportAction, which effectively makes the TransportResetFeatureStateAction a class that dispatches other transport actions to do the real work. This could leave cleanup vulnerable to interference from other actions, e.g., indices deleted between lookup and deletion actions, but I can try to tighten that up at a later stage if I need to.

williamrandolph · 2021-02-23T17:28:01Z

I still have some TODOs scattered in the code. I'll keep working on those, but I'm interested in feedback on the overall approach.

gwbrown

Left a couple comments, approach looks good though. Nice work!

server/src/main/java/org/elasticsearch/plugins/SystemIndexPlugin.java

...r/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java

client/rest-high-level/src/main/java/org/elasticsearch/client/FeaturesClient.java

client/rest-high-level/src/main/java/org/elasticsearch/client/FeaturesRequestConverters.java

...java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateRequest.java

jaymode · 2021-03-11T19:56:33Z

server/src/main/java/org/elasticsearch/indices/SystemIndices.java

+ List<String> systemIndices = indexDescriptors.stream()
+ .map(sid -> sid.getMatchingIndices(clusterService.state().getMetadata()))
+ .flatMap(List::stream)
+ .collect(Collectors.toList());
+
+ List<String> associatedIndices = new ArrayList<>(associatedIndexPatterns);
+
+ List<String> allIndices = Stream.concat(systemIndices.stream(), associatedIndices.stream())
+ .collect(Collectors.toList());


Suggested change

List<String> systemIndices = indexDescriptors.stream()

.map(sid -> sid.getMatchingIndices(clusterService.state().getMetadata()))

.flatMap(List::stream)

.collect(Collectors.toList());

List<String> associatedIndices = new ArrayList<>(associatedIndexPatterns);

List<String> allIndices = Stream.concat(systemIndices.stream(), associatedIndices.stream())

.collect(Collectors.toList());

Stream<String> systemIndices = indexDescriptors.stream()

.map(sid -> sid.getMatchingIndices(clusterService.state().getMetadata()))

.flatMap(List::stream);

List<String> allIndices = Stream.concat(systemIndices, associatedIndexPatterns.stream())

.collect(Collectors.toUnmodifiableList());

We're unnecessarily creating objects here, so I provided a suggestion to clean it up. Also should associatedIndexPatterns be resolved from patterns to indices?

I made the suggested change, but you're probably right about associatedIndexPatterns. I'll take a look and report back.

This works as written, because we can pass a valid index pattern to the delete request, which will then handle the resolution logic when executed. We can't do the same thing with system index descriptor patterns because they can use character-class regexes, for example .test-[ab]*.

It might be worth resolving the associated indices here so that we can skip submitting the delete request when no indices exist. Do you think this is important to do?

I think it is fine as it is unless we discover issues

server/src/main/java/org/elasticsearch/indices/SystemIndices.java

Out of an abundance of caution, I think the "reset" part of this path should have a leading underscore, so that if there's ever a reason to implement "GET _features/<feature_id>" we won't have to worry about distinguishing "reset" from a feature name.

jaymode

LGTM

williamrandolph · 2021-03-17T19:13:12Z

@elasticmachine update branch

williamrandolph · 2021-03-17T19:39:50Z

@elasticmachine run elasticsearch-ci/1

When we disable access to system indices, plugins will still need a way to erase their state. The obvious and most pressing use case for this is in tests, which need to be able to clean up the state of a cluster in between groups of tests. * Use a HandledTransportAction for reset action My initial cut used a TransportMasterNodeAction, which requires code that carefully manipulates cluster state. At least for the first cut and testing, it seems like it will be much easier to use a client within a HandledTransportAction, which effectively makes the TransportResetFeatureStateAction a class that dispatches other transport actions to do the real work. * Clean up code by using a GroupedActionListener * ML feature state cleaner * Implement Transform feature state reset * Change _features/reset path to _features/_reset Out of an abundance of caution, I think the "reset" part of this path should have a leading underscore, so that if there's ever a reason to implement "GET _features/<feature_id>" we won't have to worry about distinguishing "reset" from a feature name. Co-authored-by: Gordon Brown <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

elasticmachine · 2021-03-17T20:45:44Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

* Add API for resetting state of a `SystemIndexPlugin` (#69469) When we disable access to system indices, plugins will still need a way to erase their state. The obvious and most pressing use case for this is in tests, which need to be able to clean up the state of a cluster in between groups of tests. * Use a HandledTransportAction for reset action My initial cut used a TransportMasterNodeAction, which requires code that carefully manipulates cluster state. At least for the first cut and testing, it seems like it will be much easier to use a client within a HandledTransportAction, which effectively makes the TransportResetFeatureStateAction a class that dispatches other transport actions to do the real work. * Clean up code by using a GroupedActionListener * ML feature state cleaner * Implement Transform feature state reset * Change _features/reset path to _features/_reset Out of an abundance of caution, I think the "reset" part of this path should have a leading underscore, so that if there's ever a reason to implement "GET _features/<feature_id>" we won't have to worry about distinguishing "reset" from a feature name. Co-authored-by: Gordon Brown <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

williamrandolph · 2021-03-18T00:29:43Z

Backport to 7.x: #70524

williamrandolph added 14 commits February 11, 2021 16:08

Stub out classes for reset feature states API

77adad6

Add enough code to get an integration test working

0a55d99

Improve unit test

51c8f8a

Add rough version of callback to SystemIndexPlugin

581cd0a

License changes

d6fa39c

Just use clusterService, client, and listener in callback

bf5c86a

Add javadoc and update tests

88ec70d

Fill out ResetFeatureStateResponse class

eb03955

Hook up response class in TransportAction

c5e82bc

Rename inner class

8cafd70

Add a second plugin to integration tests

e2af327

Clean up code by using a GroupedActionListener

1e4c3bd

Merge branch 'master' into si/reset-features-api

7b6c663

williamrandolph requested review from gwbrown and jaymode February 23, 2021 17:26

Add reset to list of non-operator actions

237bc63

This was referenced Feb 23, 2021

Add ML associated indices #69492

Merged

Add Transform associated indices #69493

Merged

gwbrown reviewed Feb 24, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/plugins/SystemIndexPlugin.java Show resolved Hide resolved

...r/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestResetFeatureStateAction.java Outdated Show resolved Hide resolved

williamrandolph and others added 2 commits February 24, 2021 16:38

Consolidate default deletion logic in SystemIndices.feature

02a5937

ML feature state cleaner

83817e4

gwbrown mentioned this pull request Feb 25, 2021

Ensure Reset Features API correctly resets ML configuration #69581

Closed

Implement Transform feature state reset

5ea27d6

gwbrown mentioned this pull request Feb 25, 2021

Add Transform reset logic williamrandolph/elasticsearch#2

Merged

Merge changes from master

b9b1833

jaymode linked an issue Mar 2, 2021 that may be closed by this pull request

Add an API to remove System Indices #62778

Closed

williamrandolph added 2 commits March 2, 2021 14:36

Merge changes from master

1c7d174

Change endpoint to _feature/reset

db50f4d

Merge branch 'master' into si/reset-features-api

b316a91

jaymode requested changes Mar 11, 2021

View reviewed changes

Merge branch 'master' into si/reset-features-api

b05ec26

williamrandolph mentioned this pull request Mar 15, 2021

Allow resetting only a subset of feature states #70426

Open

williamrandolph added 3 commits March 15, 2021 15:56

Respond to PR feedback

d07d8c6

Checkstyle fixes

8c97adb

williamrandolph requested a review from jaymode March 16, 2021 19:44

jaymode approved these changes Mar 16, 2021

View reviewed changes

Merge branch 'master' into si/reset-features-api

71bd331

williamrandolph merged commit 624ee45 into elastic:master Mar 17, 2021

williamrandolph added :Core/Infra/Core Core issues without another label >feature backport pending labels Mar 17, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Mar 17, 2021

williamrandolph added v7.13.0 v8.0.0 labels Mar 17, 2021

williamrandolph mentioned this pull request Apr 15, 2021

Add docs for feature reset API #71759

Merged

jaymode removed the backport pending label Apr 26, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

williamrandolph deleted the si/reset-features-api branch May 23, 2022 17:26

arteam mentioned this pull request Apr 20, 2023

[CI] MultiVersionRepositoryAccessIT testCreateAndRestoreSnapshot failing #94459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for resetting state of a `SystemIndexPlugin` #69469

Add API for resetting state of a `SystemIndexPlugin` #69469

williamrandolph commented Feb 23, 2021

williamrandolph commented Feb 23, 2021

gwbrown left a comment

jaymode Mar 11, 2021

williamrandolph Mar 15, 2021

williamrandolph Mar 16, 2021

jaymode Mar 16, 2021

jaymode left a comment

williamrandolph commented Mar 17, 2021

williamrandolph commented Mar 17, 2021

elasticmachine commented Mar 17, 2021

williamrandolph commented Mar 18, 2021

Add API for resetting state of a SystemIndexPlugin #69469

Add API for resetting state of a SystemIndexPlugin #69469

Conversation

williamrandolph commented Feb 23, 2021

williamrandolph commented Feb 23, 2021

gwbrown left a comment

Choose a reason for hiding this comment

jaymode Mar 11, 2021

Choose a reason for hiding this comment

williamrandolph Mar 15, 2021

Choose a reason for hiding this comment

williamrandolph Mar 16, 2021

Choose a reason for hiding this comment

jaymode Mar 16, 2021

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

williamrandolph commented Mar 17, 2021

williamrandolph commented Mar 17, 2021

elasticmachine commented Mar 17, 2021

williamrandolph commented Mar 18, 2021

Add API for resetting state of a `SystemIndexPlugin` #69469

Add API for resetting state of a `SystemIndexPlugin` #69469