[ML] Job in Index: Enable integ tests #34851

davidkyle · 2018-10-25T11:18:33Z

Re-enables most of the integration tests excluding the rolling upgrade tests and a lot of fixes to make the tests pass again.

Bug Fixes

Refresh the .ml-config index after create, update and deletes
Mute tests depending on knowing the estimated memory usage [ML] Investigate alternative methods for sharing job memory usage information #34084
The job field added to the persistent task params JobParams needs to be serialise in toXContent for node restart tests. This field is not backwards compatible but the change is ok as it could only possibly cause an error after down-grading a cluster which is not supported.
Job and datafeed Ids must be ordered
Checks when creating a job that the job Id is not an existing group Id
Mute tests depending on missing job errors in TransportJobTaskAction. See [ML] Job in Index Feature: TransportJobTaskAction no longer detects unknown jobs #34747
Disable index mappings for CategorizationAnalyzerConfig. This field can either be the name of a pre-canned analyzer or a complex object. We cannot have the mappings for both in a single index.
Fix AutoDetectResultProcessor making a blocking call on the network thread
Wait for job update on AutoDetectResultProcessor close actions in awaitCompletion()
DeleteExpiredDataIT must set the retention period to a high number rather than null. See [ML] Prevent default job values overwriting nulled fields #34804

elasticmachine · 2018-10-25T11:18:37Z

Pinging @elastic/ml-core

davidkyle · 2018-10-25T11:21:54Z

...ulti-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/DatafeedJobsRestIT.java

@@ -705,7 +705,7 @@ public void testRealtime() throws Exception {
        response = e.getResponse();
        assertThat(response.getStatusLine().getStatusCode(), equalTo(409));
        assertThat(EntityUtils.toString(response.getEntity()),
-                containsString("Cannot delete job [" + jobId + "] because datafeed [" + datafeedId + "] refers to it"));
+                containsString("Cannot delete job [" + jobId + "] because the job is opened"));


The order of checks has changed. The response code is the same (409) only the error message has change which does not constitute a breaking change.

davidkyle · 2018-10-25T11:23:38Z

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetDatafeedsAction.java

-            configById.put(expandedDatafeedId, mlMetadata.getDatafeed(expandedDatafeedId));
+        try {
+            MlMetadata mlMetadata = MlMetadata.getMlMetadata(clusterState);
+            Set<String> expandedDatafeedIds = mlMetadata.expandDatafeedIds(datafeedExpression, allowNoDatafeeds);


This throws if the required job Id(s) is not found. Eventually this code will be removed but I think it's a useful early check that the future migration process is successful.

davidkyle · 2018-10-25T11:24:06Z

...ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/persistence/DatafeedConfigProvider.java

@@ -181,7 +186,7 @@ public void onFailure(Exception e) {
    public void findDatafeedsForJobIds(Collection<String> jobIds, ActionListener<Set<String>> listener) {
        SearchSourceBuilder sourceBuilder = new SearchSourceBuilder().query(buildDatafeedJobIdsQuery(jobIds));
        sourceBuilder.fetchSource(false);
-        sourceBuilder.docValueField(DatafeedConfig.ID.getPreferredName());
+        sourceBuilder.docValueField(DatafeedConfig.ID.getPreferredName(), DocValueFieldsContext.USE_DEFAULT_FORMAT);


USE_DEFAULT_FORMAT prevents a deprecation warning

davidkyle · 2018-10-25T11:26:22Z

...java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutoDetectResultProcessor.java

@@ -99,6 +100,7 @@
    private final boolean restoredSnapshot;

    final CountDownLatch completionLatch = new CountDownLatch(1);
+    volatile CountDownLatch onCloseActionsLatch;


This class now has 3 synchronisation objects - 2 latches and a semaphore and is a candidate for refactoring

davidkyle · 2018-10-25T11:27:10Z

...k/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/BasicDistributedJobsIT.java

+    // or .ml-anomalies-shared indices are not available. It fails because
+    // the data node stops and the ml node is not a data node so the job
+    // config cannot be read from .ml-config
+    @AwaitsFix(bugUrl = "Job in index")


I think this tests can be removed

Is there a way to use allocation rules to force .ml-config onto one data node, and .ml-state and .ml-anomalies-shared onto another, so that .ml-config remains available when the node hosting .ml-state and .ml-anomalies-shared goes down?

benwtrent

Only one small comment from me. :)

benwtrent · 2018-10-25T13:22:38Z

...ck/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobConfigProvider.java

+
+        SearchRequest searchRequest = client.prepareSearch(AnomalyDetectorsIndex.configIndexName())
+                .setIndicesOptions(IndicesOptions.lenientExpandOpen())
+                .setSource(sourceBuilder).request();


What if ids.size() > 10?

Good point! I should set .setSize(ids.size())
I'll push that change once the PR build has finished as I'm hoping it will go green

davidkyle · 2018-10-25T16:13:52Z

retest this please

Refresh config index after create, update and delete ops. Refresh config index after setting the job’s finished time

Fixes job_groups.yml tests

CategorizationAnalyzerConfig can be either string or a complex object we cannot define mappings for either and mix them in an index so the mappings must be disabled.

Prevents the network thread being used for a blocking call (JobResultsPersister.commitResultWrites)

retention days to a high number rather than null

davidkyle · 2018-10-26T08:39:30Z

Unrelated BWC tests are failing.
I merged 6.x into the base feature branch and rebased this PR.

droberts195

I left one suggestion for adding an assert.

I think the case of the .ml-config index not being available could be quite a big problem that needs to be considered in many parts of the code. But I'm happy to merge this to the feature branch as-is and deal with that in a future PR.

droberts195 · 2018-10-26T12:26:48Z

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetDatafeedsAction.java

+                configById.put(expandedDatafeedId, mlMetadata.getDatafeed(expandedDatafeedId));
+            }
+        } catch (Exception e){
+            // ignore


Based on your comment above it sounds like exceptions are unexpected here? If so, we could assert and display the exception and stack trace. Then we'd get CI failures that might flag situations we hadn't thought of. (But obviously not if I've misunderstood and exceptions here are expected.)

The purpose here is to check that the job is not defined in both the cluster state and the index and will error in that case. Unfortunately expandDatafeedIds throws if the argument is a wild card (foo*) and there is no match

droberts195 · 2018-10-26T12:51:03Z

...k/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/BasicDistributedJobsIT.java

+    // or .ml-anomalies-shared indices are not available. It fails because
+    // the data node stops and the ml node is not a data node so the job
+    // config cannot be read from .ml-config
+    @AwaitsFix(bugUrl = "Job in index")


Is there a way to use allocation rules to force .ml-config onto one data node, and .ml-state and .ml-anomalies-shared onto another, so that .ml-config remains available when the node hosting .ml-state and .ml-anomalies-shared goes down?

droberts195 · 2018-10-26T12:53:15Z

...k/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/integration/BasicDistributedJobsIT.java

+    // This tests is designed to check a job wont' open when the .ml-state
+    // or .ml-anomalies-shared indices are not available. It fails because
+    // the data node stops and the ml node is not a data node so the job
+    // config cannot be read from .ml-config


This is actually an important case. We probably need a whole suite of tests for how things fail when .ml-config becomes unavailable. It could happen for example when all the data nodes are shut down before all the ML nodes during a full cluster restart.

Also, it would be undesirable for jobs to go into the failed state if .ml-config has query failures for a fraction of a second when the node holding the primary shard fails and the replica needs to become primary.

But such tests can wait for a followup PR.

There is a way to make the test work by routing the config index to a different node. It makes for quite a convoluted test but I've pushed it anyway. Tests for missing the .ml-config index will follow as part of the migration work

Enables the ml integration tests excluding the rolling upgrade tests and a lot of fixes to make the tests pass again.

* [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 >feature :ml Machine learning labels Oct 25, 2018

davidkyle commented Oct 25, 2018

View reviewed changes

benwtrent reviewed Oct 25, 2018

View reviewed changes

benwtrent approved these changes Oct 25, 2018

View reviewed changes

davidkyle added 23 commits October 26, 2018 09:17

Fix deprecation warnings for doc value fields format

1f0fb22

Re-enable MachineLearningLicensingTests

9e0fc5c

Refresh, refresh, refresh

e873419

Refresh config index after create, update and delete ops. Refresh config index after setting the job’s finished time

Mute TooManyJobsIT tests they rely on the job memory usage being known

0cdff43

ToXContent job field in OpenJobParams

088617a

Mute BasicDistributedJobsIT.testMlIndicesNotAvailable

c81e54d

Enable muted job manager tests

d9dde59

Catch MlMetadata throwing from expandX

cecaa89

Return sorted Ids for job/datafeed expand Ids

4b40e57

Various checks at job creation that the job and group names are unique.

7c3a7ad

Fixes job_groups.yml tests

Mute tests where a missing job exception is no longer thrown

97bbfd3

Disable index mappings for CategorizationAnalyzerConfig

11054d7

CategorizationAnalyzerConfig can be either string or a complex object we cannot define mappings for either and mix them in an index so the mappings must be disabled.

Delete job was accidentally deleting .ml-anomalies-shared

9b3774c

Do update job work on autodetect close in a threaded listener

f24bff1

Prevents the network thread being used for a blocking call (JobResultsPersister.commitResultWrites)

Delete expired data tests must set model snapshot

c6b6882

retention days to a high number rather than null

fix test after the order of checks deleting a datafeed has changed

f5499a1

Wait for the update job request on autodetect result processor close

d330b51

Revert change to JobTests

501438c

Make onCloseActionsLatch visible to thread calling awaitCompletion

f2e7012

Mute rolling upgrade tests

9aec620

Actually really mute rolling upgrade tests

553928d

Set size for requests where the max number of hits is known

1274e16

Remove CategorizationAnalyzerConfig fields from mapping tests

cc223ed

davidkyle added 2 commits October 26, 2018 09:18

Fix unknown fields in job params toXContent tests

daffc3e

checkstyle

f0f976e

davidkyle force-pushed the make-testing-again branch from 70c5d1b to f0f976e Compare October 26, 2018 08:35

davidkyle mentioned this pull request Oct 26, 2018

[ML] Metadata Migration Meta issue #32905

Closed

43 tasks

droberts195 approved these changes Oct 26, 2018

View reviewed changes

Re-enable BasicDistributedJobsIT test for active indices

2e0b983

davidkyle merged commit 625e675 into elastic:feature-jindex-6x Oct 29, 2018

davidkyle deleted the make-testing-again branch October 29, 2018 16:46

davidkyle added a commit to davidkyle/elasticsearch that referenced this pull request Oct 30, 2018

[ML] Job in Index: Enable integ tests (elastic#34851)

2194128

Enables the ml integration tests excluding the rolling upgrade tests and a lot of fixes to make the tests pass again.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Job in Index: Enable integ tests #34851

[ML] Job in Index: Enable integ tests #34851

davidkyle commented Oct 25, 2018 •

edited by droberts195

Loading

elasticmachine commented Oct 25, 2018

davidkyle Oct 25, 2018

davidkyle Oct 25, 2018

davidkyle Oct 25, 2018

davidkyle Oct 25, 2018

davidkyle Oct 25, 2018

droberts195 Oct 26, 2018

benwtrent left a comment

benwtrent Oct 25, 2018

davidkyle Oct 25, 2018

davidkyle commented Oct 25, 2018

davidkyle commented Oct 26, 2018

droberts195 left a comment

droberts195 Oct 26, 2018

davidkyle Oct 26, 2018

droberts195 Oct 26, 2018

droberts195 Oct 26, 2018

davidkyle Oct 29, 2018

[ML] Job in Index: Enable integ tests #34851

[ML] Job in Index: Enable integ tests #34851

Conversation

davidkyle commented Oct 25, 2018 • edited by droberts195 Loading

elasticmachine commented Oct 25, 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

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle commented Oct 25, 2018

davidkyle commented Oct 26, 2018

droberts195 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

Choose a reason for hiding this comment

davidkyle commented Oct 25, 2018 •

edited by droberts195

Loading