ML: update set_upgrade_mode, add logging

[benwtrent]

There have been some flaky tests in the set_upgrade_mode yml tests. This PR adds some logging (to aid future debugging) and hopefully addresses the flakiness of the tests.

I have not been able to reproduce this scenario: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+6.x+matrix-java-periodic/ES_BUILD_JAVA=java11,ES_RUNTIME_JAVA=java8,nodes=immutable&&linux&&docker/221/console

from what I can tell in the logs, the datafeed was stopped

./x-pack/plugin/build/cluster/integTestCluster node0/elasticsearch-6.7.0-SNAPSHOT/logs/x-pack_plugin_integTestCluster.log:[2019-02-03T23:49:03,257][INFO ][o.e.x.m.d.DatafeedManager] [node-0] [no_realtime] try lock [20s] to stop datafeed [set-upgrade-mode-job-datafeed] for job [set-upgrade-mode-job]...

The no_realtime indicates that for some reason the Datafeed was not being treated as a realtime job. To combat, I am adding a index health check + logs to aid in troubleshooting again in the future.

[elasticmachine]

Pinging @elastic/ml-core

[benwtrent]

run elasticsearch-ci/1

[droberts195]

LGTM

On further analysis I can see that the problem is not fixed

[droberts195]

I've had a closer look at what's happening here, and have spotted a bug that is the most likely cause. It's actually a bug that could affect us in other situations, so it's good that implementing this endpoint has exposed it.

Some history: when the concept of isolating datafeeds was first added, we always isolated all datafeeds running on a particular node. Later the ability to just isolate a single datafeed was added, and this is when the bug was introduced.

DatafeedManager has member variable isolated that is set to true when all datafeeds are isolated simultaneously. This dates back to the time when the only way to isolate datafeeds was to isolate all datafeeds on a node. Now that it is possible to isolate a single datafeed we should not have this variable at all. Please delete this variable from the code. All the tests of isolated in DatafeedManager should instead be testing the value of holder.isIsolated(), which is set on a per-datafeed basis when that datafeed is isolated.

The test failure is coming from this piece of code in DatafeedManager:

                if (isolated == false) {
                    if (next != null) {
                        doDatafeedRealtime(next, holder.datafeedJob.getJobId(), holder);
                    } else {
                        holder.stop("no_realtime", TimeValue.timeValueSeconds(20), null);
                        holder.problemTracker.finishReport();
                    }
                }

The intention is not to react to a lookback finishing if the datafeed is isolated, but we are reacting because we're checking the variable that's only set when all datafeeds are isolated by a single function call. Changing it to this should fix the problem:

                if (holder.isIsolated() == false) {
                    if (next != null) {
                        doDatafeedRealtime(next, holder.datafeedJob.getJobId(), holder);
                    } else {
                        holder.stop("no_realtime", TimeValue.timeValueSeconds(20), null);
                        holder.problemTracker.finishReport();
                    }
                }

[droberts195]

This highlights the massive complexity caused by separating jobs and datafeeds yet then trying to make them work together in a user friendly way.

There are 3 scenarios where we isolate datafeeds:

1. Node shutting down - this is handled by the if at the very beginning of the method.
2. Upgrade mode - in this case we most definitely do not want to close the associated job.
3. Force-delete datafeed - this usually happens as part of the UI deleting a job and datafeed, so closing the job is unnecessary as the force-delete of the job that comes soon after the force-delete of the datafeed will do that. But theoretically a user could force-delete just the datafeed and not the job, and in this case it would make sense to auto-close the job. However, leaving the job open is the path that is least likely to do damage, so we'll go with that.

All this highlights the total lack of encapsulation between jobs and datafeeds. The fact that you need an intimate knowledge of exactly how every usage pattern plays out to make a small change to datafeeds shows that it would make things much easier if they were one and the same thing, as suggested by #34231. But since implementing #34231 would be a multi-month project maybe the best thing is just to get things working with the current separation and then try to change as little as possible in this area.

[droberts195]

I renamed this variable to make clearer what it's used for. In the case of isolating a datafeed prior to enabling upgrade mode the datafeed may relocate to a different node when upgrade mode is eventually cancelled. But as long as the node where this code is running remains up it needs to run the finishHandler.accept(e); line below otherwise the local task associated with the persistent task will linger and cause problems.

[dimitris-athanasiou]

LGTM

[droberts195]

After merging to master this will now need backporting to 7.x, 7.0 and 6.7.