add APIs to Maintenance Mode in ILM #31410

talevy · 2018-06-18T21:30:11Z

POST _xpack/index_lifecycle/_stop
- issues a request to be placed into STOPPED mode (maintenance mode).
  This is not immediate, since we must first verify that
  it is safe to go from STOPPING -> STOPPED.
POST _xpack/index_lifecycle/_start
- issues a request to be placed back into RUNNING mode (immediately)
GET _xpack/index_lifecycle/_status
- get back the current mode our lifecycle management is in
update task was hardened to support uninstalled metadata
if no metadata is installed, the start/stop actions will install metadata
and proceed to try and change it (default start mode is RUNNING)
rename MAINTENANCE -> STOPPED, MAINTENANCE_REQUESTED -> STOPPING, NORMAL -> RUNNING

follow-up to #31164.

- POST _xpack/index_lifecycle/maintenance/_request - issues a request to be placed into maintenenance mode. This is not immediate, since we must first verify that it is safe to go from REQUESTED -> IN maintenance mode. - POST _xpack/index_lifecycle/maintenance/_stop - issues a request to be taken out (this is immediate) - GET _xpack/index_lifecycle/maintenance - get back the current mode our lifecycle management is in - maintenance mode update task was hardened to support uninstalled metadata - if no metadata is installed, the request/stop actions will install metadata and proceed to try and change it (default start mode is NORMAL) follow-up to elastic#31164.

elasticmachine · 2018-06-18T21:30:13Z

Pinging @elastic/es-core-infra

talevy · 2018-06-19T05:24:57Z

I have some setup issues, I will fix the actions setup tomorrow morning. But, please review the rest when available!

I noticed it would be a weird experience to error because the lifecycle metadata wasn't installed... so I decided that these set APIs can install it too. That feels like the more appropriate thing to do, but I can see it being desired to keep the metadata installation to put-lifecycle. let me know what you think

colings86 · 2018-06-19T09:53:35Z

...c/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/GetMaintenanceModeAction.java

+public class GetMaintenanceModeAction
+        extends Action<GetMaintenanceModeAction.Request, GetMaintenanceModeAction.Response> {
+    public static final GetMaintenanceModeAction INSTANCE = new GetMaintenanceModeAction();
+    public static final String NAME = "cluster:admin/xpack/index_lifecycle/maintenance/get";


I think this should be cluster:admin/xpack/index_lifecycle/operation_mode/get

colings86 · 2018-06-19T09:54:02Z

...c/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/GetMaintenanceModeAction.java

+        @Override
+        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+            builder.startObject();
+            builder.field("in_maintenance_mode", mode == OperationMode.MAINTENANCE);


Do we need this given we have the mode below?

colings86 · 2018-06-19T09:54:16Z

...c/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/GetMaintenanceModeAction.java

+            builder.startObject();
+            builder.field("in_maintenance_mode", mode == OperationMode.MAINTENANCE);
+            builder.field("operation_mode", mode);
+            return builder;


missing end object?

colings86 · 2018-06-19T09:54:55Z

...src/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/SetOperationModeAction.java

+
+public class SetOperationModeAction extends Action<SetOperationModeAction.Request, SetOperationModeAction.Response> {
+    public static final SetOperationModeAction INSTANCE = new SetOperationModeAction();
+    public static final String NAME = "cluster:admin/xpack/index_lifecycle/operation_mode";


I think this should be: cluster:admin/xpack/index_lifecycle/operation_mode/set

colings86 · 2018-06-19T09:55:21Z

...src/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/SetOperationModeAction.java

+import java.io.IOException;
+import java.util.Objects;
+
+public class SetOperationModeAction extends Action<SetOperationModeAction.Request, SetOperationModeAction.Response> {


I think we should call this PutOperationModeAction

colings86 · 2018-06-19T09:57:50Z

...ifecycle/src/main/java/org/elasticsearch/xpack/indexlifecycle/MaintenanceModeUpdateTask.java

+        if (currentMetadata.getMaintenanceMode().isValidChange(mode)) {
+            newMode = mode;
+        } else {
+            newMode = currentMetadata.getMaintenanceMode();


Can this be any cleaner? We seem to be checking things twice. Maybe we could check for a nell metadata first and then check if the change is valid?

colings86 · 2018-06-19T10:00:46Z

.../src/main/java/org/elasticsearch/xpack/indexlifecycle/action/RestGetOperationModeAction.java

+    public RestGetOperationModeAction(Settings settings, RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.GET,
+            IndexLifecycle.BASE_PATH + "maintenance", this);


I think the endpoint shouldn't contain maintenance otherwise its confusing what it means when this API returns mode: normal. Instead I think this should be _xapck/index_lifecycle/mode or _xapck/index_lifecycle/operation_mode

colings86 · 2018-06-19T10:01:39Z

...rc/main/java/org/elasticsearch/xpack/indexlifecycle/action/RestRequestMaintenanceAction.java

+    public RestRequestMaintenanceAction(Settings settings, RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.POST,
+            IndexLifecycle.BASE_PATH + "maintenance/_request", this);


I wonder if it would be better to have this endpoint as PUT _xpack/index_lifeycle/operation_mode/{mode}?

colings86 · 2018-06-19T10:03:27Z

...e/src/main/java/org/elasticsearch/xpack/indexlifecycle/action/RestStopMaintenanceAction.java

+    public RestStopMaintenanceAction(Settings settings, RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.POST,
+            IndexLifecycle.BASE_PATH + "maintenance/_stop", this);


I wonder if it would be better to have this endpoint as PUT _xpack/index_lifeycle/operation_mode/{mode}?

clintongormley · 2018-06-19T19:09:15Z

Could this just be:

POST _xpack/index_lifecycle/_start
POST _xpack/index_lifecycle/_stop
GET _xpack/index_lifecycle/_status

where _status could return running, stopping, stopped?

talevy · 2018-06-19T19:33:24Z

@clintongormley yes. I believe I prefer your suggestion. I believe there were two reason we
went down the route of naming it "maintenance"

it was meant to be a state which allows other cluster-level administrative operations to be done
we already had a concept of "stop" by way of "stopping lifecycles per index" with index.lifecycle.skip.

If one steps away from those two points, "maintenance" is a rather ambiguous term and running, stopping, stopped are self-explanatory. We can leave "skipping" to be an index-level "stop".

@colings86, what do you think?

talevy · 2018-06-19T23:02:21Z

@colings86 I've made the changes to the new format that Clint suggested! I think we are in a better place, thanks Clint!

colings86

I left some minor comments but I think its close

colings86 · 2018-06-20T07:54:06Z

...src/main/java/org/elasticsearch/xpack/core/indexlifecycle/action/SetOperationModeAction.java

+import java.io.IOException;
+import java.util.Objects;
+
+public class SetOperationModeAction extends Action<SetOperationModeAction.Response> {


Maybe to be consistent with the naming of the other APIs in Elasticsearch we should call this Put instead of Set?

I suppose this felt weird since I am not PUTing a resty resource, especially since it is being exposed by APIs that do not necessarily ever set a specific operation-mode directly, like STOPPED.

Given all of ^, I also view this as a pedantic argument that loses in the battle of consistency 😄. So I will rename

colings86 · 2018-06-20T07:56:59Z

...ain/java/org/elasticsearch/xpack/indexlifecycle/action/RestGetOperationModeStatusAction.java

+import org.elasticsearch.xpack.core.indexlifecycle.action.GetOperationModeStatusAction;
+import org.elasticsearch.xpack.indexlifecycle.IndexLifecycle;
+
+public class RestGetOperationModeStatusAction extends BaseRestHandler {


I think we should call this API *GetStatusAction since that is the endpoint that it maps to

agreed, another one of case of "crossed my mind, but decided against it for mediocre reasons". Will change

talevy · 2018-06-20T16:07:25Z

thanks @colings86, I've updated with the renaming and fixed the failing rest tests

colings86

LGTM assuming it passes CI

- POST _xpack/index_lifecycle/_stop - issues a request to be placed into STOPPED mode (maintenance mode). This is not immediate, since we must first verify that it is safe to go from STOPPING -> STOPPED. - POST _xpack/index_lifecycle/_start - issues a request to be placed back into RUNNING mode (immediately) - GET _xpack/index_lifecycle/_status - get back the current mode our lifecycle management is in - update task was hardened to support uninstalled metadata - if no metadata is installed, the start/stop actions will install metadata and proceed to try and change it (default start mode is RUNNING) - rename MAINTENANCE -> STOPPED, MAINTENANCE_REQUESTED -> STOPPING, NORMAL -> RUNNING follow-up to #31164.

talevy added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Jun 18, 2018

talevy requested a review from colings86 June 19, 2018 05:22

colings86 requested changes Jun 19, 2018

View reviewed changes

talevy added 2 commits June 19, 2018 15:22

Merge branch 'index-lifecycle' into ilm-maintenance-api

7100a55

refactor to leverage new APIs and remove notion of maintenace_mode

41a8ed3

colings86 reviewed Jun 20, 2018

View reviewed changes

talevy added 2 commits June 20, 2018 08:31

Merge branch 'index-lifecycle' into ilm-maintenance-api

b172873

rename and fix tests

fdd78e5

Merge branch 'index-lifecycle' into ilm-maintenance-api

703bab7

colings86 approved these changes Jun 21, 2018

View reviewed changes

Merge branch 'index-lifecycle' into ilm-maintenance-api

29e47a7

talevy merged commit 901aafb into elastic:index-lifecycle Jun 21, 2018

talevy deleted the ilm-maintenance-api branch June 21, 2018 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add APIs to Maintenance Mode in ILM #31410

add APIs to Maintenance Mode in ILM #31410

talevy commented Jun 18, 2018 •

edited

Loading

elasticmachine commented Jun 18, 2018

talevy commented Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

colings86 Jun 19, 2018

clintongormley commented Jun 19, 2018

talevy commented Jun 19, 2018

talevy commented Jun 19, 2018

colings86 left a comment

colings86 Jun 20, 2018

talevy Jun 20, 2018

colings86 Jun 20, 2018

talevy Jun 20, 2018

talevy commented Jun 20, 2018

colings86 left a comment

add APIs to Maintenance Mode in ILM #31410

add APIs to Maintenance Mode in ILM #31410

Conversation

talevy commented Jun 18, 2018 • edited Loading

elasticmachine commented Jun 18, 2018

talevy commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintongormley commented Jun 19, 2018

talevy commented Jun 19, 2018

talevy commented Jun 19, 2018

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talevy commented Jun 20, 2018

colings86 left a comment

Choose a reason for hiding this comment

talevy commented Jun 18, 2018 •

edited

Loading