Make feature reset API response more informative

client/rest-high-level/src/test/java/org/elasticsearch/client/FeaturesIT.java

@@ -12,9 +12,15 @@
 import org.elasticsearch.client.feature.GetFeaturesResponse;
 import org.elasticsearch.client.feature.ResetFeaturesRequest;
 import org.elasticsearch.client.feature.ResetFeaturesResponse;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.search.SearchModule;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.stream.Collectors;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.notNullValue;
 
@@ -34,13 +40,25 @@ public void testGetFeatures() throws IOException {
     public void testResetFeatures() throws IOException {
         ResetFeaturesRequest request = new ResetFeaturesRequest();
 
+        // need superuser privileges to execute the reset
+        RestHighLevelClient adminHighLevelClient = new RestHighLevelClient(
+            adminClient(),
+            (client) -> {},
+            new SearchModule(Settings.EMPTY, Collections.emptyList()).getNamedXContents());
         ResetFeaturesResponse response = execute(request,
-            highLevelClient().features()::resetFeatures, highLevelClient().features()::resetFeaturesAsync);
+            adminHighLevelClient.features()::resetFeatures,
+            adminHighLevelClient.features()::resetFeaturesAsync);
 
         assertThat(response, notNullValue());
         assertThat(response.getFeatures(), notNullValue());
         assertThat(response.getFeatures().size(), greaterThan(1));
         assertTrue(response.getFeatures().stream().anyMatch(
             feature -> "tasks".equals(feature.getFeatureName()) && "SUCCESS".equals(feature.getStatus())));
+
+        Set<String> statuses = response.getFeatures().stream()
+            .map(ResetFeaturesResponse.ResetFeatureStateStatus::getStatus)
+            .collect(Collectors.toSet());
+
+        assertThat(statuses, contains("SUCCESS"));
     }
 }

server/src/internalClusterTest/java/org/elasticsearch/snapshots/FeatureStateResetApiIT.java

@@ -8,10 +8,13 @@
 
 package org.elasticsearch.snapshots;
 
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse;
 import org.elasticsearch.action.admin.indices.get.GetIndexResponse;
+import org.elasticsearch.client.Client;
+import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.indices.SystemIndexDescriptor;
@@ -35,6 +38,7 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
         List<Class<? extends Plugin>> plugins = new ArrayList<>(super.nodePlugins());
         plugins.add(SystemIndexTestPlugin.class);
         plugins.add(SecondSystemIndexTestPlugin.class);
+        plugins.add(EvilSystemIndexTestPlugin.class);
         return plugins;
     }
 
@@ -63,9 +67,10 @@ public void testResetSystemIndices() throws Exception {
         // call the reset API
         ResetFeatureStateResponse apiResponse = client().execute(ResetFeatureStateAction.INSTANCE, new ResetFeatureStateRequest()).get();
         assertThat(apiResponse.getItemList(), containsInAnyOrder(
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("SystemIndexTestPlugin", "SUCCESS"),
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("SecondSystemIndexTestPlugin", "SUCCESS"),
-            new ResetFeatureStateResponse.ResetFeatureStateStatus("tasks", "SUCCESS")
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("SystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("SecondSystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("EvilSystemIndexTestPlugin"),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success("tasks")
         ));
 
         // verify that both indices are gone
@@ -94,6 +99,21 @@ public void testResetSystemIndices() throws Exception {
         assertThat(response.getIndices(), arrayContaining("my_index"));
     }
 
+    /**
+     * Evil test - what if the cleanup method fails?
+     */
+    public void testFailFastOnStateCleanupFailure() throws Exception {
+        try {
+            EvilSystemIndexTestPlugin.beEvil = true;
+            ResetFeatureStateResponse resetFeatureStateResponse = client().execute(ResetFeatureStateAction.INSTANCE,
+                new ResetFeatureStateRequest()).get();
+
+            assertTrue(resetFeatureStateResponse.hasSomeFailures());
+        } finally {
+            EvilSystemIndexTestPlugin.beEvil = false;
+        }
+    }
+
     /**
      * A test plugin with patterns for system indices and associated indices.
      */
@@ -145,4 +165,34 @@ public String getFeatureDescription() {
             return "A second test plugin";
         }
     }
+
+    /**
+     * An evil test plugin to test failure cases.
+     */
+    public static class EvilSystemIndexTestPlugin extends Plugin implements SystemIndexPlugin {
+
+        private static boolean beEvil = false;
+
+        @Override
+        public String getFeatureName() {
+            return "EvilSystemIndexTestPlugin";
+        }
+
+        @Override
+        public String getFeatureDescription() {
+            return "a plugin that can be very bad";
+        }
+
+        @Override
+        public void cleanUpFeature(
+            ClusterService clusterService,
+            Client client,
+            ActionListener<ResetFeatureStateResponse.ResetFeatureStateStatus> listener) {
+            if (beEvil == false) {
+                listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.success(getFeatureName()));
+            } else {
+                listener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(getFeatureName(), "problem!"));
+            }
+        }
+    }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponse.java

@@ -8,6 +8,7 @@
 
 package org.elasticsearch.action.admin.cluster.snapshots.features;
 
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -47,6 +48,14 @@ public List<ResetFeatureStateStatus> getItemList() {
         return this.resetFeatureStateStatusList;
     }
 
+    public boolean hasSomeFailures() {
+        return resetFeatureStateStatusList.stream().anyMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE));
+    }
+
+    public boolean hasAllFailures() {
+        return resetFeatureStateStatusList.stream().allMatch(status -> status.getStatus().equals(ResetFeatureStateStatus.Status.FAILURE));
+    }
+
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
@@ -92,59 +101,108 @@ public String toString() {
      */
     public static class ResetFeatureStateStatus implements Writeable, ToXContentObject {
         private final String featureName;
-        private final String status;
+        private final Status status;
+        private final Exception exception;
 
-        public ResetFeatureStateStatus(String featureName, String status) {
+        public enum Status {
+            SUCCESS,
+            FAILURE
+        }
+
+        public static ResetFeatureStateStatus success(String featureName) {
+            return new ResetFeatureStateStatus(featureName, Status.SUCCESS, null);
+        }
+
+        public static ResetFeatureStateStatus failure(String featureName, Exception exception) {
+            return new ResetFeatureStateStatus(
+                featureName,
+                Status.FAILURE,
+                exception);
+        }
+
+        public static ResetFeatureStateStatus failure(String featureName, String exceptionMessage) {
+            return new ResetFeatureStateStatus(
+                featureName,
+                Status.FAILURE,
+                new ElasticsearchException(exceptionMessage));
+        }
+
+        private ResetFeatureStateStatus(String featureName, Status status, Exception exception) {
             this.featureName = featureName;
             this.status = status;
+            this.exception = exception;
         }
 
         ResetFeatureStateStatus(StreamInput in) throws IOException {
             this.featureName = in.readString();
-            this.status = in.readString();
+            this.status = Status.valueOf(in.readString());
+            this.exception = in.readBoolean() ? in.readException() : null;
         }
 
         public String getFeatureName() {
             return this.featureName;
         }
 
-        public String getStatus() {
+        public Status getStatus() {
             return this.status;
         }
 
+        public Exception getException() {
+            return this.exception;
+        }
+
         @Override
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             builder.startObject();
             builder.field("feature_name", this.featureName);
             builder.field("status", this.status);
+            if (this.exception != null) {
+                builder.field("exception", this.exception);
+            }
             builder.endObject();
             return builder;
         }
 
-        @Override
-        public void writeTo(StreamOutput out) throws IOException {
-            out.writeString(this.featureName);
-            out.writeString(this.status);
-        }
-
+        /**
+         * Without a convenient way to compare Exception equality, we consider
+         * only feature name and success or failure for equality.
+         * @param o An object to compare for equality
+         * @return True if the feature name and status are equal, false otherwise
+         */
         @Override
         public boolean equals(Object o) {
             if (this == o) return true;
             if (o == null || getClass() != o.getClass()) return false;
             ResetFeatureStateStatus that = (ResetFeatureStateStatus) o;
-            return Objects.equals(featureName, that.featureName) && Objects.equals(status, that.status);
+            return Objects.equals(featureName, that.featureName) && status == that.status;
         }
 
+        /**
+         * @return Hash code based only on feature name and status.
+         */
         @Override
         public int hashCode() {
             return Objects.hash(featureName, status);
         }
 
+        @Override
+        public void writeTo(StreamOutput out) throws IOException {
+            out.writeString(this.featureName);
+            out.writeString(this.status.toString());
+            if (exception != null) {
+                out.writeBoolean(true);
+                out.writeException(exception);
+            } else {
+                out.writeBoolean(false);
+            }
+        }
+
         @Override
         public String toString() {
             return "ResetFeatureStateStatus{" +
                 "featureName='" + featureName + '\'' +
-                ", status='" + status + '\'' +
+                ", status=" + status +
+                ", exception='" + exception + '\'' +
                 '}';
         }
     }

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

@@ -596,7 +596,7 @@ public static void cleanUpFeature(
 
             if (allIndices.isEmpty()) {
                 // if no actual indices match the pattern, we can stop here
-                listener.onResponse(new ResetFeatureStateStatus(name, "SUCCESS"));
+                listener.onResponse(ResetFeatureStateStatus.success(name));
                 return;
             }
 
@@ -605,12 +605,12 @@ public static void cleanUpFeature(
             client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, new ActionListener<>() {
                 @Override
                 public void onResponse(AcknowledgedResponse acknowledgedResponse) {
-                    listener.onResponse(new ResetFeatureStateStatus(name, "SUCCESS"));
+                    listener.onResponse(ResetFeatureStateStatus.success(name));
                 }
 
                 @Override
                 public void onFailure(Exception e) {
-                    listener.onResponse(new ResetFeatureStateStatus(name, "FAILURE: " + e.getMessage()));
+                    listener.onResponse(ResetFeatureStateStatus.failure(name, e));
                 }
             });
         }

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

@@ -10,9 +10,11 @@
 
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction;
 import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest;
+import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.rest.BaseRestHandler;
 import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.rest.action.RestToXContentListener;
 
 import java.io.IOException;
@@ -39,6 +41,19 @@ public String getName() {
     protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
         final ResetFeatureStateRequest req = new ResetFeatureStateRequest();
 
-        return restChannel -> client.execute(ResetFeatureStateAction.INSTANCE, req, new RestToXContentListener<>(restChannel));
+        return restChannel -> client.execute(
+            ResetFeatureStateAction.INSTANCE,
+            req,
+            new RestToXContentListener<>(restChannel) {
+                @Override
+                protected RestStatus getStatus(ResetFeatureStateResponse response) {
+                    if (response.hasAllFailures()) {
+                        return RestStatus.INTERNAL_SERVER_ERROR;
+                    } else if (response.hasSomeFailures()) {
+                        return RestStatus.MULTI_STATUS;
+                    }
+                    return RestStatus.OK;
+                }
+            });
     }
 }

server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/features/ResetFeatureStateResponseTests.java

@@ -29,10 +29,12 @@ protected ResetFeatureStateResponse createTestInstance() {
         List<ResetFeatureStateResponse.ResetFeatureStateStatus> resetStatuses = new ArrayList<>();
         String feature1 = randomAlphaOfLengthBetween(4, 10);
         String feature2 = randomValueOtherThan(feature1, () -> randomAlphaOfLengthBetween(4, 10));
-        resetStatuses.add(new ResetFeatureStateResponse.ResetFeatureStateStatus(
-            feature1, randomFrom("SUCCESS", "FAILURE")));
-        resetStatuses.add(new ResetFeatureStateResponse.ResetFeatureStateStatus(
-            feature2, randomFrom("SUCCESS", "FAILURE")));
+        resetStatuses.add(randomFrom(
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature1),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature1, "bad")));
+        resetStatuses.add(randomFrom(
+            ResetFeatureStateResponse.ResetFeatureStateStatus.success(feature2),
+            ResetFeatureStateResponse.ResetFeatureStateStatus.failure(feature2, "bad")));
         return new ResetFeatureStateResponse(resetStatuses);
     }
 
@@ -46,8 +48,7 @@ protected ResetFeatureStateResponse mutateInstance(ResetFeatureStateResponse ins
             .map(ResetFeatureStateResponse.ResetFeatureStateStatus::getFeatureName)
             .collect(Collectors.toSet());
         return new ResetFeatureStateResponse(randomList(minSize, 10,
-            () -> new ResetFeatureStateResponse.ResetFeatureStateStatus(
-                randomValueOtherThanMany(existingFeatureNames::contains, () -> randomAlphaOfLengthBetween(4, 10)),
-                randomAlphaOfLengthBetween(5, 10))));
+            () -> ResetFeatureStateResponse.ResetFeatureStateStatus.success(
+                randomValueOtherThanMany(existingFeatureNames::contains, () -> randomAlphaOfLengthBetween(4, 10)))));
     }
 }

x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/Transform.java

@@ -414,7 +414,7 @@ public void cleanUpFeature(
                     + (stopTransformsResponse.getTaskFailures().isEmpty()
                         ? ""
                         : "task failures: " + stopTransformsResponse.getTaskFailures());
-                unsetResetModeListener.onResponse(new ResetFeatureStateResponse.ResetFeatureStateStatus(this.getFeatureName(), errMsg));
+                unsetResetModeListener.onResponse(ResetFeatureStateResponse.ResetFeatureStateStatus.failure(this.getFeatureName(), errMsg));
             }
         }, unsetResetModeListener::onFailure);