From cc432aee34cd09ec56a1db69af7e4dfc7b8ec34d Mon Sep 17 00:00:00 2001 From: Dimitris Athanasiou Date: Mon, 22 Jan 2018 13:49:38 +0000 Subject: [PATCH] [ML] Check calendar exists before removing job (elastic/x-pack-elasticsearch#3661) Also, removes check for whether a job-to-remove exists and replaces it with a check of whether a job-to-remove is already present in the calendar. This allows to remove a job that may no longer exists and it improves feedback for the case that an existing job is removed from a calendar that doesn't contain it. relates elastic/x-pack-elasticsearch#3620 Original commit: elastic/x-pack-elasticsearch@3ea39be1b65cbfa1dfbb8f7bd5d42e4084de32c4 --- .../xpack/ml/job/persistence/JobProvider.java | 21 +++++++++++++++++- .../TransportUpdateCalendarJobAction.java | 22 +------------------ .../xpack/ml/integration/JobProviderIT.java | 22 ++++++++++++++----- .../rest-api-spec/test/ml/calendar_crud.yml | 13 +++++++++-- qa/smoke-test-ml-with-security/build.gradle | 1 + 5 files changed, 50 insertions(+), 29 deletions(-) diff --git a/plugin/core/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java b/plugin/core/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java index 95e7e66b224..7614184a00d 100644 --- a/plugin/core/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java +++ b/plugin/core/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobProvider.java @@ -67,7 +67,9 @@ import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.sort.FieldSortBuilder; import org.elasticsearch.search.sort.SortBuilders; import org.elasticsearch.search.sort.SortOrder; +import org.elasticsearch.xpack.ml.MLMetadataField; import org.elasticsearch.xpack.ml.MlMetaIndex; +import org.elasticsearch.xpack.ml.MlMetadata; import org.elasticsearch.xpack.ml.action.GetBucketsAction; import org.elasticsearch.xpack.ml.action.GetCategoriesAction; import org.elasticsearch.xpack.ml.action.GetInfluencersAction; @@ -1081,12 +1083,29 @@ public class JobProvider { result -> handler.accept(result.result), errorHandler, () -> null); } - public void updateCalendar(String calendarId, Set jobIdsToAdd, Set jobIdsToRemove, + public void updateCalendar(String calendarId, Set jobIdsToAdd, Set jobIdsToRemove, ClusterState clusterState, Consumer handler, Consumer errorHandler) { ActionListener getCalendarListener = ActionListener.wrap( calendar -> { Set currentJobs = new HashSet<>(calendar.getJobIds()); + + for (String jobToAdd : jobIdsToAdd) { + MlMetadata mlMetadata = clusterState.getMetaData().custom(MLMetadataField.TYPE); + if (mlMetadata.isGroupOrJob(jobToAdd) == false) { + errorHandler.accept(ExceptionsHelper.missingJobException(jobToAdd)); + return; + } + } + + for (String jobToRemove : jobIdsToRemove) { + if (currentJobs.contains(jobToRemove) == false) { + errorHandler.accept(ExceptionsHelper.badRequestException("Cannot remove [" + jobToRemove + + "] as it is not present in calendar [" + calendarId + "]")); + return; + } + } + currentJobs.addAll(jobIdsToAdd); currentJobs.removeAll(jobIdsToRemove); Calendar updatedCalendar = new Calendar(calendar.getId(), new ArrayList<>(currentJobs), calendar.getDescription()); diff --git a/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java b/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java index 58369e5594e..75a80d9caad 100644 --- a/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java +++ b/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportUpdateCalendarJobAction.java @@ -8,18 +8,14 @@ package org.elasticsearch.xpack.ml.action; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; -import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; -import org.elasticsearch.xpack.ml.MLMetadataField; -import org.elasticsearch.xpack.ml.MlMetadata; import org.elasticsearch.xpack.ml.job.JobManager; import org.elasticsearch.xpack.ml.job.persistence.JobProvider; -import org.elasticsearch.xpack.ml.utils.ExceptionsHelper; public class TransportUpdateCalendarJobAction extends HandledTransportAction { @@ -41,23 +37,7 @@ public class TransportUpdateCalendarJobAction extends HandledTransportAction listener) { - ClusterState state = clusterService.state(); - MlMetadata mlMetadata = state.getMetaData().custom(MLMetadataField.TYPE); - for (String jobToAdd: request.getJobIdsToAdd()) { - if (mlMetadata.isGroupOrJob(jobToAdd) == false) { - listener.onFailure(ExceptionsHelper.missingJobException(jobToAdd)); - return; - } - } - - for (String jobToRemove: request.getJobIdsToRemove()) { - if (mlMetadata.isGroupOrJob(jobToRemove) == false) { - listener.onFailure(ExceptionsHelper.missingJobException(jobToRemove)); - return; - } - } - - jobProvider.updateCalendar(request.getCalendarId(), request.getJobIdsToAdd(), request.getJobIdsToRemove(), + jobProvider.updateCalendar(request.getCalendarId(), request.getJobIdsToAdd(), request.getJobIdsToRemove(), clusterService.state(), c -> { jobManager.updateProcessOnCalendarChanged(c.getJobIds()); listener.onResponse(new PutCalendarAction.Response(c)); diff --git a/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java b/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java index c588e2a1614..188f97de42e 100644 --- a/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java +++ b/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/JobProviderIT.java @@ -10,7 +10,9 @@ import org.elasticsearch.action.bulk.BulkRequestBuilder; import org.elasticsearch.action.bulk.BulkResponse; import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.support.WriteRequest; +import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.routing.UnassignedInfo; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; @@ -20,10 +22,12 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.xpack.XPackSettings; -import org.elasticsearch.xpack.ml.MlSingleNodeTestCase; import org.elasticsearch.xpack.ml.LocalStateMachineLearning; +import org.elasticsearch.xpack.ml.MLMetadataField; import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.ml.MlMetaIndex; +import org.elasticsearch.xpack.ml.MlMetadata; +import org.elasticsearch.xpack.ml.MlSingleNodeTestCase; import org.elasticsearch.xpack.ml.action.PutJobAction; import org.elasticsearch.xpack.ml.action.util.QueryPage; import org.elasticsearch.xpack.ml.calendars.Calendar; @@ -136,6 +140,14 @@ public class JobProviderIT extends MlSingleNodeTestCase { } public void testUpdateCalendar() throws Exception { + MlMetadata.Builder mlBuilder = new MlMetadata.Builder(); + mlBuilder.putJob(createJob("foo").build(), false); + mlBuilder.putJob(createJob("bar").build(), false); + + ClusterState clusterState = ClusterState.builder(new ClusterName("_name")) + .metaData(new MetaData.Builder().putCustom(MLMetadataField.TYPE, mlBuilder.build())) + .build(); + String calendarId = "empty calendar"; Calendar emptyCal = new Calendar(calendarId, Collections.emptyList(), null); indexCalendars(Collections.singletonList(emptyCal)); @@ -143,7 +155,7 @@ public class JobProviderIT extends MlSingleNodeTestCase { Set addedIds = new HashSet<>(); addedIds.add("foo"); addedIds.add("bar"); - updateCalendar(calendarId, addedIds, Collections.emptySet()); + updateCalendar(calendarId, addedIds, Collections.emptySet(), clusterState); Calendar updated = getCalendar(calendarId); assertEquals(calendarId, updated.getId()); @@ -151,7 +163,7 @@ public class JobProviderIT extends MlSingleNodeTestCase { Set removedIds = new HashSet<>(); removedIds.add("foo"); - updateCalendar(calendarId, Collections.emptySet(), removedIds); + updateCalendar(calendarId, Collections.emptySet(), removedIds, clusterState); updated = getCalendar(calendarId); assertEquals(calendarId, updated.getId()); @@ -240,10 +252,10 @@ public class JobProviderIT extends MlSingleNodeTestCase { return result.get().results(); } - private void updateCalendar(String calendarId, Set idsToAdd, Set idsToRemove) throws Exception { + private void updateCalendar(String calendarId, Set idsToAdd, Set idsToRemove, ClusterState clusterState) throws Exception { CountDownLatch latch = new CountDownLatch(1); AtomicReference exceptionHolder = new AtomicReference<>(); - jobProvider.updateCalendar(calendarId, idsToAdd, idsToRemove, + jobProvider.updateCalendar(calendarId, idsToAdd, idsToRemove, clusterState, r -> latch.countDown(), e -> { exceptionHolder.set(e); diff --git a/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml b/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml index 05c0969ed1e..75d39443339 100644 --- a/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml +++ b/plugin/src/test/resources/rest-api-spec/test/ml/calendar_crud.yml @@ -252,10 +252,10 @@ job_id: "missing job" - do: - catch: missing + catch: /Cannot remove \[missing_job\] as it is not present in calendar \[wildlife\]/ xpack.ml.delete_calendar_job: calendar_id: "wildlife" - job_id: "missing job" + job_id: "missing_job" --- "Test calendar get events": @@ -622,3 +622,12 @@ xpack.ml.delete_calendar_event: calendar_id: "unknown" event_id: "event_1" + +--- +"Test delete job from non existing calendar": + + - do: + catch: /No calendar with id \[unknown\]/ + xpack.ml.delete_calendar_job: + calendar_id: "unknown" + job_id: "missing_job" diff --git a/qa/smoke-test-ml-with-security/build.gradle b/qa/smoke-test-ml-with-security/build.gradle index 64ab3b9cc8e..20c2cd44482 100644 --- a/qa/smoke-test-ml-with-security/build.gradle +++ b/qa/smoke-test-ml-with-security/build.gradle @@ -27,6 +27,7 @@ integTestRunner { 'ml/calendar_crud/Test post calendar events given empty events', 'ml/calendar_crud/Test put calendar given id contains invalid chars', 'ml/calendar_crud/Test delete event from non existing calendar', + 'ml/calendar_crud/Test delete job from non existing calendar', 'ml/custom_all_field/Test querying custom all field', 'ml/datafeeds_crud/Test delete datafeed with missing id', 'ml/datafeeds_crud/Test put datafeed referring to missing job_id',