Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Allocate jobs based on JobParams rather than cluster state config #33994

Merged
merged 5 commits into from
Sep 27, 2018

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Sep 24, 2018

OpenJobPersistentTasksExecutor allocates jobs to nodes based on the number of open jobs and their memory usage in the method getAssignment. getAssignment is called by PersistentTasksClusterService (master node) and must not do any heavy or blocking work such as a search, so getAssignment must be called with all the information required to make the allocation decision. Previously this worked because all the jobs and their estimated memory usage was stored in the cluster state and readily available but this is no longer the case with the jobs stored in the index.

I considered 2 solutions

  1. Add the estimated memory usage for all open jobs to OpenJobAction.JobParams and use those in OpenJobPersistentTasksExecutor.getAssignment
    The JobParams are created by the action and passed as the persistent task start params, some time later the persistent task will call getAssignment at which point the memory usage could be out of date.

  2. Add the estimated memory usage to the persistent task parameters or MlMetadata and update it as the jobs change. This way the most up to date information will be available for the allocation decision.

2 requires frequent cluster state updates as the job's established memory changes avoiding these updates is one of the goals of this project so option 1 was chosen. TransportOpenJobAction now collects open jobs memory usage by node before starting the persistent task and passes that information in the JobParams

Update

The job assignment and balancing problem is too large a change for this PR. I simplified the code to make the decision based on open job count. The job balancing/assignment problem will be solved elsewhere possibly outside of this feature branch.

Persisting JobParams

OpenJobAction.JobParams implements ToXContentObject but does not have a lenient parser yet (#33950). During a rolling upgrade from 6.last to 7 the extra fields in JobParams can be tolerated, in 6.x they cannot (failing to parse the clusterstate takes down the node) so those fields are streamed but not persisted in JobParams.toXContent.

Re-allocation Problem

If the job is re-allocated for some reason (e.g. node failure) the open job information stored in the persistent task parameters can be very out of date. This problem isn't addressed here that is for a follow up PR.

Progress!

With this change it is now possible to open a job and POST some data to it and get results. Closing the job and using the datafeed are other matters.

There is also a small refactoring moving the ml persistent task names from TransportOpenJobAction and TransportStartDatafeedAction to MlTasks.

@davidkyle davidkyle added >feature :ml Machine learning labels Sep 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@davidkyle davidkyle changed the title [ML] JIndex: Allocate jobs based on Jpoarams rather than cluster state config [ML] JIndex: Allocate jobs based on JobParams rather than cluster state config Sep 24, 2018
@davidkyle davidkyle added the WIP label Sep 24, 2018
@davidkyle davidkyle mentioned this pull request Sep 24, 2018
43 tasks
@droberts195
Copy link
Contributor

If the job is re-allocated for some reason (e.g. node failure) the open job information stored in the persistent task parameters can be very out of date. This problem isn't addressed here that is for a follow up PR.

Even though this is being left for another PR, is there a plan for how it will be done? I think this PR should not be merged until there is a plan for that case, because otherwise the way it's done may significantly change the way this change in this PR is done.

There is also another similar case to consider. What happens if a user has a system with no running jobs, then starts 5 jobs in quick succession, such that they are all in the master queue before any of the persistent tasks are allocated. I suspect that in that case the node assigned memory map will be empty for all 5 jobs, and they'll all get assigned to the same node. This scenario is not as unlikely as it might seem, as the UI can start many jobs at the same time if you use the pre-canned Nginx module jobs for example.

A possible option in both cases is that if the allocation detects that another persistent task has been allocated in between creation of the node assigned memory map and the current allocation decision then it fails to allocate the job and instead kicks off a new action that repopulates the node assigned memory map. There may be a better solution though.

@davidkyle
Copy link
Member Author

Even though this is being left for another PR, is there a plan for how it will be done?

At the moment no. It is an unforeseen problem I welcome any suggestions.

The current count of open jobs and allocating jobs is available from the persistent tasks so we could fall back to using the basic mechanism of the count of open jobs to determine assignment but at the moment there is no way of specifying the memory usage map is stale/invalid.

@davidkyle davidkyle changed the title [ML] JIndex: Allocate jobs based on JobParams rather than cluster state config [ML] Allocate jobs based on JobParams rather than cluster state config Sep 25, 2018
@davidkyle
Copy link
Member Author

I think the current approach is a bust due to the problems outlined above. It is possible to know if a task is re-assigning during a call to OpenJobPersistentTasksExecutor.getAssignment as the task will exist in the persistent tasks meta data based on that I suggest the following:

  1. Add a map of job id to established memory usage to the task params
  2. Add the jobs established model memory and max model memory (analysis limits) to each task's params
  3. Max model memory cannot be changed without closing the job so the value in task param is always current
  4. On initial assignment base the allocation decision on memory values stored in each task's parameters. This handles the case where a bunch of jobs open at the same time and the tasks weren't allocated when the job memory map was created. If the memory map contains the job then use that value as the established memory in preference to the one in the task's parameters as it is likely more recent.
  5. On re-location of the job assume the established model memory values and the memory map are out of date and use the worst case value of max model memory

@davidkyle
Copy link
Member Author

I pushed another commit removing the logic that makes the allocation decision based on job memory. The job balancing problem needs work which is beyond the scope of this PR

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool, happy to merge this to the feature branch with the plan of fixing #34084 before the feature branch is merged into 6.x.

@davidkyle davidkyle merged commit acd5ddd into elastic:feature/jindex-6.x Sep 27, 2018
@davidkyle davidkyle deleted the open-job-validator branch September 27, 2018 09:10
davidkyle added a commit to davidkyle/elasticsearch that referenced this pull request Oct 29, 2018
davidkyle added a commit that referenced this pull request Dec 18, 2018
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants