[ML] Job In Index: Migrate config from the clusterstate #35834

davidkyle · 2018-11-22T17:01:14Z

The new class MlConfigMigrator reads ml configurations from the cluster state and for every config that does not have an associated persistent task writes the configurations to .ml-config in a bulk update then removes them from the clusterstate MlMetaData. Bulk update failures are tracked so that only the successfully indexed configs are removed from the clusterstate.

Migrating open jobs and datafeeds which have a persisted task will be addressed in a follow up PR.

Migration is triggered by clusterstate changes and only after all nodes in the cluster are on version 6.6.0 or higher, the migrator class has a check to prevent concurrent invocations.

To enable old jobs to run on v6.6.0 nodes after migration the job version is updated to v6.6.0. It's useful to somehow mark configs that have been migrated as having done so. Jobs have a custom settings map for arbitrary values, upon migration a new entry is added migrated from version : vX.X.X. Datafeeds do not have a version or custom settings.

The migrator is triggered on cluster state changes, since ml already had a class listening for these events in MlAssignmentNotifier I added the migrator there.

One of the problems I hit was that when a job is closed completing the persistent task causes an clusterstate update event which in turn can trigger the migrator to move the config. After a job is closed its finished_time is set, there is a window between the migrator copying the config to the index and removing the config from the clusterstate where the finished_time can be set in the clusterstate config but not in the index version. I reordered completing the persistent task and setting the finished time to prevent this happening

elasticmachine · 2018-11-22T17:01:15Z

Pinging @elastic/ml-core

davidkyle · 2018-11-29T14:35:15Z

run gradle build tests 2

davidkyle · 2018-11-29T17:50:29Z

I rebased this on the change to prefer clusterstate config to index #36014 and changed the index op of the config documents to allow those to be overwritten. This is for the case where the config is indexed but removing it from the clusterstate fails so the config is in 2 places. We always prefer the clusterstate version in this case so overwriting the index copy is ok.

benwtrent · 2018-11-29T17:52:32Z

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

+
+    /**
+     * Remove the datafeeds and jobs listed in the parameters from
+     * mlMetadata if they exist. An account of emoved jobs and datafeeds


Suggested change

* mlMetadata if they exist. An account of emoved jobs and datafeeds

* mlMetadata if they exist. An account of removed jobs and datafeeds

benwtrent · 2018-11-29T17:53:24Z

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

+            if (currentDatafeeds.remove(datafeedId) != null) {
+                removedDatafeedIds.add(datafeedId);
+            }
+        }


I guess this is domain knowledge I don't have yet, but why are we concerned about the order of the datafeeds, but not the Jobs?

No this was because MlMetaData.getDatafeeds returns a sorted map whereas MlMetaData.getJobs returns a plain map and I brought that inconsistency into this code. I change it to a plain map as it does not need to be anything else.

benwtrent · 2018-11-29T17:56:33Z

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

+
+        try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
+            indexRequest.source(source.toXContent(builder, params));
+        } catch (IOException e) {


There are also unchecked exceptions like XContentParseException orIllegalArgumentException. If the context of the documentId is needed, you MAY want to catch those.

If something really bad like that happens the exception will abort the migration process and eventually the error will be logged in MlAssignmentNotifier and the migration will retry some time later on the next clusterstate change. This happens before anything is written so we are not left in an inconsistent state

benwtrent · 2018-11-29T17:57:16Z

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

+            return mlMetadata.getJobs().values().stream()
+                    .filter(job -> openJobIds.contains(job.getId()) == false)
+                    .collect(Collectors.toList());
+        }


weird indentation

benwtrent · 2018-11-29T18:03:24Z

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

+                            ActionListener.wrap(
+                                    response -> task.markAsCompleted(),
+                                    e -> logger.error("error finalizing job [" + jobId + "]", e)
+                            ));


Why did we move this from the close action to the open one?

Because.

autodetectProcessManager.openJob takes a lambda which is called when the job is closed, that lambda has a generically useless name handler and as you can see calls a method on jobTask which we pass to the method. The same JobTask is also passed to the close and kill methods which eventually call the lambda parameter we pass here which calls a method on the jobTask we pass in close & kill. Basically we don't need the lambda parameter and it makes it very difficult to find the bit of code called on job close as it is set here in the open method.

task.markAsCompleted() causes a clusterstate update at which point the migrator sees that the job is closed and is eligible for migration. The FinalizeJobExecutionAction call needs to happen before to prevent a race condition between FinalizeJobExecutionAction updating the job config and the migrator reading the config from the clusterstate. This happens to be the simplest place to make that change, it's not a natural home but it simplifies the logic doing it here.

droberts195 · 2018-11-30T10:44:57Z

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

+            // ok to migrate
+            mlConfigMigrator.migrateConfigsWithoutTasks(event.state(), ActionListener.wrap(
+                    response -> threadPool.executor(executorName()).execute(() -> auditChangesToMlTasks(current, previous, event.state())),
+                    e -> logger.error("error migrating ml configurations", e)


Maybe a migration error should still proceed to audit changes to ML tasks?

droberts195 · 2018-11-30T10:48:39Z

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

+                .map(MlConfigMigrator::updateJobForMigration)
+                .collect(Collectors.toList());
+
+        ActionListener<Boolean> onFailureUnMarkMigrationInProgress = ActionListener.wrap(


Could the variable name be just unMarkMigrationInProgress? It's not just for failures.

droberts195 · 2018-11-30T10:49:45Z

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

+        writeConfigToIndex(datafeedsToMigrate, jobsToMigrate, ActionListener.wrap(
+                failedDocumentIds -> {
+                    List<String> successfulJobWrites = filterFailedJobConfigWrites(failedDocumentIds, jobsToMigrate);
+                    List<String> successfullDatafeedWrites =


Typo: sucessfull -> successful

davidkyle · 2018-11-30T13:46:24Z

The BWC test failed deleting a job as it was migrated between these 2 lines

       if (ClusterStateJobUpdate.jobIsInClusterState(clusterService.state(), jobId)) {
            ClusterStateJobUpdate.markJobAsDeleting(jobId, force, clusterService, listener);  <-- cluster state update fails 
        } else {

One answer is reject deletion requests if the job is eligible for migration (i.e. closed) but hasn't been migrated yet

droberts195

LGTM

benwtrent

davidkyle · 2018-12-04T10:29:39Z

run gradle build tests 1

Migrate ML configuration from clusterstate to index for closed jobs only once all nodes are v6.6.0 or higher

* [ML] Job and datafeed mappings with index template (#32719) Index mappings for the configuration documents * [ML] Job config document CRUD operations (#32738) * [ML] Datafeed config CRUD operations (#32854) * [ML] Change JobManager to work with Job config in index (#33064) * [ML] Change Datafeed actions to read config from the config index (#33273) * [ML] Allocate jobs based on JobParams rather than cluster state config (#33994) * [ML] Return missing job error when .ml-config is does not exist (#34177) * [ML] Close job in index (#34217) * [ML] Adjust finalize job action to work with documents (#34226) * [ML] Job in index: Datafeed node selector (#34218) * [ML] Job in Index: Stop and preview datafeed (#34605) * [ML] Delete job document (#34595) * [ML] Convert job data remover to work with index configs (#34532) * [ML] Job in index: Get datafeed and job stats from index (#34645) * [ML] Job in Index: Convert get calendar events to index docs (#34710) * [ML] Job in index: delete filter action (#34642) This changes the delete filter action to search for jobs using the filter to be deleted in the index rather than the cluster state. * [ML] Job in Index: Enable integ tests (#34851) Enables the ml integration tests excluding the rolling upgrade tests and a lot of fixes to make the tests pass again. * [ML] Reimplement established model memory (#35500) This is the 7.0 implementation of a master node service to keep track of the native process memory requirement of each ML job with an associated native process. The new ML memory tracker service works when the whole cluster is upgraded to at least version 6.6. For mixed version clusters the old mechanism of established model memory stored on the job in cluster state was used. This means that the old (and complex) code to keep established model memory up to date on the job object has been removed in 7.0. Forward port of #35263 * [ML] Need to wait for shards to replicate in distributed test (#35541) Because the cluster was expanded from 1 node to 3 indices would initially start off with 0 replicas. If the original node was killed before auto-expansion to 1 replica was complete then the test would fail because the indices would be unavailable. * [ML] DelayedDataCheckConfig index mappings (#35646) * [ML] JIndex: Restore finalize job action (#35939) * [ML] Replace Version.CURRENT in streaming functions (#36118) * [ML] Use 'anomaly-detector' in job config doc name (#36254) * [ML] Job In Index: Migrate config from the clusterstate (#35834) Migrate ML configuration from clusterstate to index for closed jobs only once all nodes are v6.6.0 or higher * [ML] Check groups against job Ids on update (#36317) * [ML] Adapt to periodic persistent task refresh (#36633) * [ML] Adapt to periodic persistent task refresh If https://github.com/elastic/elasticsearch/pull/36069/files is merged then the approach for reallocating ML persistent tasks after refreshing job memory requirements can be simplified. This change begins the simplification process. * Remove AwaitsFix and implement TODO * [ML] Default search size for configs * Fix TooManyJobsIT.testMultipleNodes Two problems: 1. Stack overflow during async iteration when lots of jobs on same machine 2. Not effectively setting search size in all cases * Use execute() instead of submit() in MlMemoryTracker We don't need a Future to wait for completion * [ML][TEST] Fix NPE in JobManagerTests * [ML] JIindex: Limit the size of bulk migrations (#36481) * [ML] Prevent updates and upgrade tests (#36649) * [FEATURE][ML] Add cluster setting that enables/disables config migration (#36700) This commit adds a cluster settings called `xpack.ml.enable_config_migration`. The setting is `true` by default. When set to `false`, no config migration will be attempted and non-migrated resources (e.g. jobs, datafeeds) will be able to be updated normally. Relates #32905 * [ML] Snapshot ml configs before migrating (#36645) * [FEATURE][ML] Split in batches and migrate all jobs and datafeeds (#36716) Relates #32905 * SQL: Fix translation of LIKE/RLIKE keywords (#36672) * SQL: Fix translation of LIKE/RLIKE keywords Refactor Like/RLike functions to simplify internals and improve query translation when chained or within a script context. Fix #36039 Fix #36584 * Fixing line length for EnvironmentTests and RecoveryTests (#36657) Relates #34884 * Add back one line removed by mistake regarding java version check and COMPAT jvm parameter existence * Do not resolve addresses in remote connection info (#36671) The remote connection info API leads to resolving addresses of seed nodes when invoked. This is problematic because if a hostname fails to resolve, we would not display any remote connection info. Yet, a hostname not resolving can happen across remote clusters, especially in the modern world of cloud services with dynamically chaning IPs. Instead, the remote connection info API should be providing the configured seed nodes. This commit changes the remote connection info to display the configured seed nodes, avoiding a hostname resolution. Note that care was taken to preserve backwards compatibility with previous versions that expect the remote connection info to serialize a transport address instead of a string representing the hostname. * [Painless] Add boxed type to boxed type casts for method/return (#36571) This adds implicit boxed type to boxed types casts for non-def types to create asymmetric casting relative to the def type when calling methods or returning values. This means that a user calling a method taking an Integer can call it with a Byte, Short, etc. legally which matches the way def works. This creates consistency in the casting model that did not previously exist. * SNAPSHOTS: Adjust BwC Versions in Restore Logic (#36718) * Re-enables bwc tests with adjusted version conditions now that #36397 enables concurrent snapshots in 6.6+ * ingest: fix on_failure with Drop processor (#36686) This commit allows a document to be dropped when a Drop processor is used in the on_failure fork of the processor chain. Fixes #36151 * Initialize startup `CcrRepositories` (#36730) Currently, the CcrRepositoryManger only listens for settings updates and installs new repositories. It does not install the repositories that are in the initial settings. This commit, modifies the manager to install the initial repositories. Additionally, it modifies the ccr integration test to configure the remote leader node at startup, instead of using a settings update. * [TEST] fix float comparison in RandomObjects#getExpectedParsedValue This commit fixes a test bug introduced with #36597. This caused some test failure as stored field values comparisons would not work when CBOR xcontent type was used. Closes #29080 * [Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (#35320) This commit exposes lucene's LatLonShape field as the default type in GeoShapeFieldMapper. To use the new indexing approach, simply set "type" : "geo_shape" in the mappings without setting any of the strategy, precision, tree_levels, or distance_error_pct parameters. Note the following when using the new indexing approach: * geo_shape query does not support querying by MULTIPOINT. * LINESTRING and MULTILINESTRING queries do not yet support WITHIN relation. * CONTAINS relation is not yet supported. The tree, precision, tree_levels, distance_error_pct, and points_only parameters are deprecated. * TESTS:Debug Log. IndexStatsIT#testFilterCacheStats * ingest: support default pipelines + bulk upserts (#36618) This commit adds support to enable bulk upserts to use an index's default pipeline. Bulk upsert, doc_as_upsert, and script_as_upsert are all supported. However, bulk script_as_upsert has slightly surprising behavior since the pipeline is executed _before_ the script is evaluated. This means that the pipeline only has access the data found in the upsert field of the script_as_upsert. The non-bulk script_as_upsert (existing behavior) runs the pipeline _after_ the script is executed. This commit does _not_ attempt to consolidate the bulk and non-bulk behavior for script_as_upsert. This commit also adds additional testing for the non-bulk behavior, which remains unchanged with this commit. fixes #36219 * Fix duplicate phrase in shrink/split error message (#36734) This commit removes a duplicate "must be a" from the shrink/split error messages. * Deprecate types in get_source and exist_source (#36426) This change adds a new untyped endpoint `{index}/_source/{id}` for both the GET and the HEAD methods to get the source of a document or check for its existance. It also adds deprecation warnings to RestGetSourceAction that emit a warning when the old deprecated "type" parameter is still used. Also updating documentation and tests where appropriate. Relates to #35190 * Revert "[Geo] Integrate Lucene's LatLonShape (BKD Backed GeoShapes) as default `geo_shape` indexing approach (#35320)" This reverts commit 5bc7822. * Enhance Invalidate Token API (#35388) This change: - Adds functionality to invalidate all (refresh+access) tokens for all users of a realm - Adds functionality to invalidate all (refresh+access)tokens for a user in all realms - Adds functionality to invalidate all (refresh+access) tokens for a user in a specific realm - Changes the response format for the invalidate token API to contain information about the number of the invalidated tokens and possible errors that were encountered. - Updates the API Documentation After back-porting to 6.x, the `created` field will be removed from master as a field in the response Resolves: #35115 Relates: #34556 * Add raw sort values to SearchSortValues transport serialization (#36617) In order for CCS alternate execution mode (see #32125) to be able to do the final reduction step on the CCS coordinating node, we need to serialize additional info in the transport layer as part of each `SearchHit`. Sort values are already present but they are formatted according to the provided `DocValueFormat` provided. The CCS node needs to be able to reconstruct the lucene `FieldDoc` to include in the `TopFieldDocs` and `CollapseTopFieldDocs` which will feed the `mergeTopDocs` method used to reduce multiple search responses (one per cluster) into one. This commit adds such information to the `SearchSortValues` and exposes it through a new getter method added to `SearchHit` for retrieval. This info is only serialized at transport and never printed out at REST. * Watcher: Ensure all internal search requests count hits (#36697) In previous commits only the stored toXContent version of a search request was using the old format. However an executed search request was already disabling hit counts. In 7.0 hit counts will stay enabled by default to allow for proper migration. Closes #36177 * [TEST] Ensure shard follow tasks have really stopped. Relates to #36696 * Ensure MapperService#getAllMetaFields elements order is deterministic (#36739) MapperService#getAllMetaFields returns an array, which is created out of an `ObjectHashSet`. Such set does not guarantee deterministic hash ordering. The array returned by its toArray may be sorted differently at each run. This caused some repeatability issues in our tests (see #29080) as we pick random fields from the array of possible metadata fields, but that won't be repeatable if the input array is sorted differently at every run. Once setting the tests seed, hppc picks that up and the sorting is deterministic, but failures don't repeat with the seed that gets printed out originally (as a seed was not originally set). See also https://issues.carrot2.org/projects/HPPC/issues/HPPC-173. With this commit, we simply create a static sorted array that is used for `getAllMetaFields`. The change is in production code but really affects only testing as the only production usage of this method was to iterate through all values when parsing fields in the high-level REST client code. Anyways, this seems like a good change as returning an array would imply that it's deterministically sorted. * Expose Sequence Number based Optimistic Concurrency Control in the rest layer (#36721) Relates #36148 Relates #10708 * [ML] Mute MlDistributedFailureIT

davidkyle added WIP :ml Machine learning labels Nov 22, 2018

davidkyle mentioned this pull request Nov 22, 2018

[ML] Metadata Migration Meta issue #32905

Closed

43 tasks

benwtrent approved these changes Nov 26, 2018

View reviewed changes

davidkyle force-pushed the migrate branch from 1d2deb8 to 1b9a2f7 Compare November 28, 2018 14:31

davidkyle removed the WIP label Nov 29, 2018

davidkyle added 14 commits November 29, 2018 17:16

First go at a migrator

cfa3373

Add config migrator to cluster state listener

4f803cd

Don’t set job version for pre v5.5 jobs

ed0e433

Add integration tests

b829d68

migrate all configs not just those without a task

bb14eb2

Trigger migration only if all nodes are >= version 6.6

d070fa7

oops

e264d6b

Improve logging

2369c07

Revert back to migrating only closed jobs

4dc454c

Prevent concurrent migrations

72599d9

Improve logging of migrated configs

0204c6e

Finalize the job before marking the the persistent task as completed

eb4ef32

Checkstyle fix

c231480

Allow index configs to be overwritten

e4a814d

davidkyle force-pushed the migrate branch from 00ad7e3 to e4a814d Compare November 29, 2018 17:47

benwtrent reviewed Nov 29, 2018

View reviewed changes

davidkyle added 2 commits November 30, 2018 09:58

Review comments and test fix

b4d5d18

checkstyle

87a8a84

droberts195 reviewed Nov 30, 2018

View reviewed changes

Typos and review comments

2ce38e1

droberts195 approved these changes Nov 30, 2018

View reviewed changes

Remove non-deterministic tests

e2953da

benwtrent approved these changes Dec 3, 2018

View reviewed changes

Add ml template to xpack upgrade tests

59e02e9

davidkyle merged commit 93044e2 into elastic:feature-jindex-6x Dec 5, 2018

davidkyle deleted the migrate branch December 5, 2018 11:00

davidkyle added a commit that referenced this pull request Dec 5, 2018

[ML] Job In Index: Migrate config from the clusterstate (#35834)

81549e6

Migrate ML configuration from clusterstate to index for closed jobs only once all nodes are v6.6.0 or higher

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Job In Index: Migrate config from the clusterstate #35834

[ML] Job In Index: Migrate config from the clusterstate #35834

davidkyle commented Nov 22, 2018 •

edited

Loading

elasticmachine commented Nov 22, 2018

davidkyle commented Nov 29, 2018

davidkyle commented Nov 29, 2018

benwtrent Nov 29, 2018

benwtrent Nov 29, 2018

davidkyle Nov 30, 2018

benwtrent Nov 29, 2018

davidkyle Nov 30, 2018

benwtrent Nov 29, 2018

benwtrent Nov 29, 2018

davidkyle Nov 30, 2018

droberts195 Nov 30, 2018 •

edited

Loading

droberts195 Nov 30, 2018

droberts195 Nov 30, 2018

davidkyle Nov 30, 2018

davidkyle commented Nov 30, 2018

droberts195 left a comment

benwtrent left a comment

davidkyle commented Dec 4, 2018

	* mlMetadata if they exist. An account of emoved jobs and datafeeds
	* mlMetadata if they exist. An account of removed jobs and datafeeds

[ML] Job In Index: Migrate config from the clusterstate #35834

[ML] Job In Index: Migrate config from the clusterstate #35834

Conversation

davidkyle commented Nov 22, 2018 • edited Loading

elasticmachine commented Nov 22, 2018

davidkyle commented Nov 29, 2018

davidkyle commented Nov 29, 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

droberts195 Nov 30, 2018 • edited Loading

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

davidkyle commented Nov 30, 2018

droberts195 left a comment

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

davidkyle commented Dec 4, 2018

davidkyle commented Nov 22, 2018 •

edited

Loading

droberts195 Nov 30, 2018 •

edited

Loading