-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[7.x] [ML] Make ML indices hidden when the node becomes master (#77416) #79123
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
Conversation
# Conflicts: # x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java
|
run elasticsearch-ci/bwc |
1 similar comment
|
run elasticsearch-ci/bwc |
c28ac59 to
a9750ec
Compare
…ackport/7.x/pr-77416
…backport/7.x/pr-77416
|
It's too late to debug this for 7.16 now - closing. |
|
I had a look at why the BWC tests failed for this PR. It was because the job results aliases against the This is a bug that will affect the same functionality in version 8.0 if a user upgrades to 8.0 in multiple steps, for example 6.8 -> 7.16 -> 8.0. The only reason the 8.0 PR passed CI is that we don't test that multiple upgrade scenario in CI. I have opened #80148 to fix this for 8.0. |
|
I've realised that the best way to test #80148 on real indices is on the 7.16 branch, where the BWC tests will upgrade indices that really were created by 6.8 instead of just fake attempts at simulating these. Therefore I am temporarily opening this PR, not to merge it, but to exercise the code from #80148 that will eventually be committed to 8.0. |
Great job on getting to the root cause. Unfortunately I was not aware of these filters and could not find the issue :/
Thanks. I assigned this to myself. |
But that code (committed to |
They won't on 8.0. But they will on this PR, hence why I cherry-picked the change to it. I'll run BWC tests 4 times on this PR, and if they pass all 4 times then that gives confidence that the fix works. There are two jobs in the BWC tests, so getting the model size stats for a random job will get the correct ones 50% of the time, hence the need to run several iterations. If this PR proves that the fix works with a real pre-7.7 index then the next step will be to add a fake old alias with a filter to #80148 so that the code at least gets covered a little in CI even though it won't be completely real coverage. It would be great if you could think about where and how to write that test. The place where the filter gets added to the job's read alias in the production code is: Line 331 in 12ad399
Finally, this PR can be closed without merging it. |
…backport/7.x/pr-77416
7407de4 to
197c934
Compare
|
@przemekwitek I think you just force pushed this PR branch to remove the change I cherry-picked from #80148. To make the BWC tests pass for this PR you'll need to add back the changes from ca8297a. Otherwise the tests are just going to fail in the same way they did a week or two ago. |
If aliases on ML hidden indices are made hidden then all attributes of the alias need to be preserved. The particularly critical case for ML is where a job's results alias has a filter that restricts documents returned when searching the shared results index to those that relate to the job.
|
run elasticsearch-ci/bwc |
3 similar comments
|
run elasticsearch-ci/bwc |
|
run elasticsearch-ci/bwc |
|
run elasticsearch-ci/bwc |
|
It looks like we had 5 consecutive successful runs of the BWC tests, so this PR has done its job of confirming that the fix works with real 6.8 indices and aliases. The next step is to add a test to #80148 that tries to recreate the filtered alias scenario as best it can. |
|
Re-opening in order to perform a few more tests. |
x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlInitializationService.java
Outdated
Show resolved
Hide resolved
…itializationService.java Co-authored-by: David Roberts <[email protected]>
|
run elasticsearch-ci/bwc |
2 similar comments
|
run elasticsearch-ci/bwc |
|
run elasticsearch-ci/bwc |
|
This functionality won't go into 7.16. |
Backports the following commits to 7.x: