[ML] Migrate unallocated jobs and datafeeds #37430

davidkyle · 2019-01-14T16:47:32Z

The final step of the process of migrating ml configs in #32905. This change migrates jobs & datafeed configs of open jobs once the persistent task becomes unallocated, the persistent task parameters must be updated for open jobs.

Prevent allocation of job tasks where the task parameters have not been updated.
OpenJobPersistentTasksExecutor.getAssignment will not assign a task that has the missing parameters
Change MlConfigMigrator to also migrate the configs of unallocated jobs & datafeeds
In the clusterstate update where configs are removed from MlMetadata also update the persistent task parameters of the unallocated tasks

I've made this a non issue for the docs as the backport in #37536 will document the change

elasticmachine · 2019-01-14T16:47:34Z

Pinging @elastic/ml-core

benwtrent · 2019-01-14T17:45:27Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlTasks.java

+    public static Set<String> unallocatedJobIds(@Nullable PersistentTasksCustomMetaData tasks,
+                                                DiscoveryNodes nodes) {
+        return unallocatedJobTasks(tasks, nodes).stream()
+                .map(task ->task.getId().substring(JOB_TASK_ID_PREFIX.length()))


#nit

Suggested change

.map(task ->task.getId().substring(JOB_TASK_ID_PREFIX.length()))

.map(task -> task.getId().substring(JOB_TASK_ID_PREFIX.length()))

That would be the macbook butterfly keyboard. Thanks apple I don't need the space bar much

benwtrent · 2019-01-14T17:48:09Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlTasks.java

+            @Nullable PersistentTasksCustomMetaData tasks,
+            DiscoveryNodes nodes) {
+        if (tasks == null) {
+            return Collections.emptySet();


#nit so that the collection type is the same as the return below.

Suggested change

return Collections.emptySet();

return Collections.emptyList();

benwtrent · 2019-01-14T17:48:30Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlTasks.java

+            @Nullable PersistentTasksCustomMetaData tasks,
+            DiscoveryNodes nodes) {
+        if (tasks == null) {
+            return Collections.emptySet();


#nit so that the collection type is the same as the return below.

Suggested change

return Collections.emptySet();

return Collections.emptyList();

benwtrent · 2019-01-14T17:50:56Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlConfigMigrator.java

@@ -215,13 +221,17 @@ public void writeConfigToIndex(Collection<DatafeedConfig> datafeedsToMigrate,
        );
    }

-    private void removeFromClusterState(List<String> jobsToRemoveIds, List<String> datafeedsToRemoveIds,
+    private void removeFromClusterState(List<Job> jobsToRemoveIds, List<DatafeedConfig> datafeedsToRemoveIds,


#nit naming change

Suggested change

private void removeFromClusterState(List<Job> jobsToRemoveIds, List<DatafeedConfig> datafeedsToRemoveIds,

private void removeFromClusterState(List<Job> jobsToRemove, List<DatafeedConfig> datafeedsToRemove,

benwtrent · 2019-01-14T17:51:36Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlConfigMigrator.java

+     * @param nodes         The nodes in the cluster
+     * @return  The argument {@code currentTasks}
+     */
+    public static PersistentTasksCustomMetaData rewritePersistentTaskParams(Map<String, Job> jobs, Map<String, DatafeedConfig>datafeeds,


Suggested change

public static PersistentTasksCustomMetaData rewritePersistentTaskParams(Map<String, Job> jobs, Map<String, DatafeedConfig>datafeeds,

public static PersistentTasksCustomMetaData rewritePersistentTaskParams(Map<String, Job> jobs, Map<String, DatafeedConfig> datafeeds,

clusterstate configs may be null

droberts195 · 2019-01-15T13:06:52Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlConfigMigrator.java

+                Job job = jobs.get(params.getJobId());
+                if (job != null) {
+                    logger.debug("updating persistent task params for job [{}]", params.getJobId());
+                    params.setJob(job);


Are you sure it's acceptable to update the parameters on currentTasks? I'm not convinced that it is, because it's changing the current local cluster state. It means this node's opinion of the cluster state version N will be different from all the other nodes. And version N+1 may not even get broadcast to other nodes if the local node thinks it's identical to version N.

Unless I'm mistaken, I think you need to build a new PersistentTasksCustomMetaData object rather than updating the one from current state.

(Same problem for datafeeds immediately below. And we should look at making JobParams and DatafeedParams immutable like our other cluster state classes in a followup.)

I had the same reservations hence all the javadoc comments. There is an assertion in the full cluster restart test that the task params are actually updated so it does appear to work.

elasticsearch/x-pack/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/MlMigrationFullClusterRestartIT.java

Line 236 in a3d0c2c

Object jobParam = XContentMapValues.extractValue("task.xpack/ml/job.params.job", task);

However reviewing PersistentTasksCustomMetaData uses the default implementation of AbstractNamedDiffable.readDiffFrom which returns the entire object and the TODO here https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/persistent/PersistentTasksCustomMetaData.java#L66 suggests that will not always be the case. Good catch I can't remember why I didn't use the builder in the first place

droberts195

LGTM

davidkyle · 2019-01-15T15:45:09Z

I pushed a change to use the task builder and modify a copy of the task params.

Making JobParams and DatafeedParams immutable is more involved as they have setters that are used in many places. The problem is that those objects are use in the requests not just as persistent task parameters.

Migrate ml job and datafeed config of open jobs and update the parameters of the persistent tasks as they become unallocated during a rolling upgrade. Block allocation of ml persistent tasks until the configs are migrated.

davidkyle added 7 commits January 14, 2019 16:42

Block assignment of open jobs until they have been migrated

ac947db

Migrate unallocated jobs

6fa88e0

Update task params

0d82d6e

Update datafeed task

152f299

Tests for updating persistent task parameters

465255a

Migration eligibility checks

b13fc1c

Remove unused code

13bbcca

davidkyle added v7.0.0 :ml Machine learning v6.7.0 labels Jan 14, 2019

davidkyle mentioned this pull request Jan 14, 2019

[ML] Metadata Migration Meta issue #32905

Closed

43 tasks

Hide empty lists of jobs and data feeds in mlmetadata

5c0739b

davidkyle mentioned this pull request Jan 14, 2019

[ML] Update full cluster restart migration tests #37082

Closed

benwtrent reviewed Jan 14, 2019

View reviewed changes

davidkyle added 2 commits January 15, 2019 10:15

Address review comments

c0d7614

Fix test assertion

a3d0c2c

clusterstate configs may be null

benwtrent approved these changes Jan 15, 2019

View reviewed changes

droberts195 reviewed Jan 15, 2019

View reviewed changes

droberts195 added the >enhancement label Jan 15, 2019

Use task builder

b63c9c7

droberts195 approved these changes Jan 15, 2019

View reviewed changes

davidkyle merged commit bea46f7 into elastic:master Jan 15, 2019

davidkyle deleted the migrate-unallocated branch January 15, 2019 18:21

davidkyle removed the v6.7.0 label Jan 16, 2019

davidkyle mentioned this pull request Jan 16, 2019

[ML] Migrate unallocated jobs and datafeeds #37536

Merged

davidkyle added the >non-issue label Jan 16, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Migrate unallocated jobs and datafeeds #37430

[ML] Migrate unallocated jobs and datafeeds #37430

davidkyle commented Jan 14, 2019 •

edited

elasticmachine commented Jan 14, 2019

benwtrent Jan 14, 2019

davidkyle Jan 15, 2019

benwtrent Jan 14, 2019

benwtrent Jan 14, 2019

benwtrent Jan 14, 2019

benwtrent Jan 14, 2019

droberts195 Jan 15, 2019 •

edited

davidkyle Jan 15, 2019

droberts195 left a comment

davidkyle commented Jan 15, 2019

	.map(task ->task.getId().substring(JOB_TASK_ID_PREFIX.length()))
	.map(task -> task.getId().substring(JOB_TASK_ID_PREFIX.length()))

	return Collections.emptySet();
	return Collections.emptyList();

	private void removeFromClusterState(List<Job> jobsToRemoveIds, List<DatafeedConfig> datafeedsToRemoveIds,
	private void removeFromClusterState(List<Job> jobsToRemove, List<DatafeedConfig> datafeedsToRemove,

	public static PersistentTasksCustomMetaData rewritePersistentTaskParams(Map<String, Job> jobs, Map<String, DatafeedConfig>datafeeds,
	public static PersistentTasksCustomMetaData rewritePersistentTaskParams(Map<String, Job> jobs, Map<String, DatafeedConfig> datafeeds,

[ML] Migrate unallocated jobs and datafeeds #37430

[ML] Migrate unallocated jobs and datafeeds #37430

Conversation

davidkyle commented Jan 14, 2019 • edited

elasticmachine commented Jan 14, 2019

benwtrent Jan 14, 2019

Choose a reason for hiding this comment

davidkyle Jan 15, 2019

Choose a reason for hiding this comment

benwtrent Jan 14, 2019

Choose a reason for hiding this comment

benwtrent Jan 14, 2019

Choose a reason for hiding this comment

benwtrent Jan 14, 2019

Choose a reason for hiding this comment

benwtrent Jan 14, 2019

Choose a reason for hiding this comment

droberts195 Jan 15, 2019 • edited

Choose a reason for hiding this comment

davidkyle Jan 15, 2019

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

davidkyle commented Jan 15, 2019

davidkyle commented Jan 14, 2019 •

edited

droberts195 Jan 15, 2019 •

edited