-
Notifications
You must be signed in to change notification settings - Fork 45
Implement allocation action which can be used in index state management #106
Implement allocation action which can be used in index state management #106
Conversation
Hi @dabr-grapeup, Thanks for the PR! I'm currently looking into an easy way to include multi-node integration tests in index management since we'll need it for this PR. Will look over the rest of the PR over the next couple days too. |
@dbbaughe have you had a chance to review this PR? I look forward to this action. |
Hey @dabr-grapeup and @arnitolog, We have one main blocker for this PR which is the testing part. We need to setup our integration tests to run on a multi-node setup so we can accurately test the allocation part. If someone can pick that up it would be great, otherwise it will have to wait until we can allocate resources to that task. Thanks, |
Hi @dabr-grapeup, If you have the time, this PR has been unblocked by So we have a way to run multi node tests now. What we need to do is add an integration test for the Allocation action that successfully migrates the shards of the index to a different node and I believe you can use the node name or number (can't remember off top of my head.. I think it's one of those) to control the shard allocation. And we need the test to only conditionally run when it's being run in multi-node configuration which we can check with the number of nodes property. If you get the chance to look this over, great! If not I'll try to pick it up shortly and write the test. |
Hey @dbbaughe , I'm working on those tests, they should be ready tomorrow. |
…rch/index-management into opendistro-for-elasticsearch#103-allocation � Conflicts: � src/main/kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/model/action/ActionConfig.kt � src/test/kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/TestHelpers.kt � src/test/kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/model/ActionConfigTests.kt
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
============================================
+ Coverage 71.46% 71.60% +0.14%
- Complexity 635 652 +17
============================================
Files 86 89 +3
Lines 3690 3772 +82
Branches 610 619 +9
============================================
+ Hits 2637 2701 +64
- Misses 736 745 +9
- Partials 317 326 +9
Continue to review full report at Codecov.
|
...kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/action/AllocationAction.kt
Show resolved
Hide resolved
...mazon/opendistroforelasticsearch/indexstatemanagement/model/action/AllocationActionConfig.kt
Show resolved
Hide resolved
...zon/opendistroforelasticsearch/indexstatemanagement/step/allocation/AttemptAllocationStep.kt
Show resolved
Hide resolved
|
||
private fun buildSettings(): Settings { | ||
val builder = Settings.builder() | ||
config.require.forEach { (key, value) -> builder.put(SETTINGS_PREFIX + AllocationActionConfig.REQUIRE + "." + key, value) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the key values be absolutely anything? Or should we have some validation on them in the parser? i.e. can you have something like "index.routing.allocation._?__": " "
or will it throw an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it is not possible to configure key like "index.routing.allocation._..__": " "
it is possible to configure "index.routing.allocation._._._._.": " "
which is equally useless. As far as I know elastic doesn't provide one regex that we can match to so I've added a test that shows that any invalid key will result in a failed policy.
...tlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/action/AllocationActionIT.kt
Outdated
Show resolved
Hide resolved
updateManagedIndexConfigStartTime(managedIndexConfig) | ||
|
||
waitFor { | ||
assertEquals("integTest-1", getIndexShardNodes(indexName)[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible this started out on integTest-1 to begin with. I think we need to get the current value of the node its on and then migrate it to the other one and assert it or perhaps have two Allocation actions on the policy that swap it between both nodes and then add another updateManagedIndexConfigStartTime and assertion to show it was on both when required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I've added a check to see on which node index is allocated. I think it's a cleaner solution than allocating multiple times.
updateManagedIndexConfigStartTime(managedIndexConfig) | ||
|
||
waitFor { | ||
assertNotEquals("integTest-0", getIndexShardNodes(indexName)[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, it's possible this was not on integTest-0 to begin with
updateManagedIndexConfigStartTime(managedIndexConfig) | ||
|
||
waitFor { | ||
assertEquals("integTest-1", getIndexShardNodes(indexName)[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
import org.elasticsearch.cluster.service.ClusterService | ||
import org.elasticsearch.common.settings.Settings | ||
|
||
class AttemptAllocationStep( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also saw the mappings had a wait_for which I'm assuming is a flag to optionally wait for the shards to finish migrating to their respective filtered nodes, but I don't see it used. Are we skipping it for the first implementation? That's fine if so, otherwise we would need another step (WaitForAllocation or something) that would periodically check the shard locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, it's a placeholder for future step.
@dbbaughe Thanks for your comments, let me know if there's anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks! |
Issue #, if available: #103
Description of changes: Implemented Allocation action which can be used in ILM policy, action sets allocation properties to given index settings.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.