Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fixes snapshot issues, adds history mapping update workflow, adds tests #255

Merged
merged 4 commits into from
Jul 17, 2020

Conversation

dbbaughe
Copy link
Contributor

Issue #, if available:

Description of changes:
This PR addresses a few different issues/fixes.

For starters the snapshot_name in ActionProperties was missing from the history index mappings.
This was causing indexing of history documents for the snapshot action to fail to be indexed as the index has strict mappings.
By adding these new mappings it was apparent we did not have a way for the history index to update it's mappings if needed.
This was added following the similar workflow for the config index by adding a schema_version and checking the existing index schema_version with the one from the loaded plugin to see if it needs to update.
To ensure this doesn't happen again for the ActionProperties a test has been added that verifies all properties in ActionProperties exists in the action_properties mapping in the history mappings.

The initiation of the history index did not do anything with the boolean return result which means it would silently fail and then attempt to index the history documents anyways. This could lead to dynamic mappings for the index. As of now this has been changed to log an error and return early as our indexing of history documents is best effort. We should look into making this error available for a monitoring system to alert on.

Fixes issues in the WaitForSnapshotStep that that was causing writes to fail to the history index and catching RemoteTransportExceptions for the multi-node clusters.

Added some tests to catch when the mappings update for the indices. If the mappings update then it means the person who updated them must increase the schema_version otherwise at runtime the plugin will not know it needs to update the index mappings. Before this was just done through code reviews, but now it will fail a test if the mappings change. It's on the user to then update the cached mappings and make sure they correctly increased the schema_version. An added security to make sure it's not missed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #255 into master will increase coverage by 0.07%.
The diff coverage is 56.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #255      +/-   ##
============================================
+ Coverage     71.31%   71.38%   +0.07%     
- Complexity      633      635       +2     
============================================
  Files            86       86              
  Lines          3646     3690      +44     
  Branches        603      610       +7     
============================================
+ Hits           2600     2634      +34     
- Misses          732      737       +5     
- Partials        314      319       +5     
Impacted Files Coverage Δ Complexity Δ
...ndexstatemanagement/IndexStateManagementHistory.kt 77.66% <0.00%> (-2.53%) 28.00 <0.00> (ø)
...anagement/step/forcemerge/WaitForForceMergeStep.kt 33.33% <0.00%> (-0.47%) 5.00 <0.00> (ø)
...atemanagement/step/snapshot/WaitForSnapshotStep.kt 46.55% <13.33%> (-8.77%) 4.00 <0.00> (ø)
...sticsearch/indexstatemanagement/util/IndexUtils.kt 76.92% <76.00%> (-0.86%) 1.00 <0.00> (ø)
...ndexstatemanagement/IndexStateManagementIndices.kt 61.40% <76.92%> (+0.98%) 10.00 <3.00> (ø)
...ent/model/managedindexmetadata/ActionProperties.kt 95.45% <80.00%> (+9.74%) 7.00 <3.00> (ø)
...atemanagement/resthandler/RestIndexPolicyAction.kt 84.28% <100.00%> (ø) 8.00 <0.00> (ø)
...exstatemanagement/model/destination/Destination.kt 48.52% <0.00%> (+1.47%) 7.00% <0.00%> (+1.00%)
...ch/indexstatemanagement/model/destination/Slack.kt 57.14% <0.00%> (+42.85%) 4.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f05b774...c24edaa. Read the comment docs.

@dbbaughe
Copy link
Contributor Author

Has some snapshot tests that need to be tested with unit tests, still tracking in this.
#245

@dbbaughe dbbaughe merged commit 41925f3 into opendistro-for-elasticsearch:master Jul 17, 2020
@dbbaughe dbbaughe deleted the snapshot-fixes branch July 17, 2020 00:06
dbbaughe added a commit to dbbaughe/index-management that referenced this pull request Jul 29, 2020
…ts (opendistro-for-elasticsearch#255)

* Fixes snapshot issues, adds history mapping update workflow, adds tests

* Adds a couple more tests for the destination parsing tests

* Removes unneeded line
dbbaughe added a commit to dbbaughe/index-management that referenced this pull request Jul 29, 2020
…ts (opendistro-for-elasticsearch#255)

* Fixes snapshot issues, adds history mapping update workflow, adds tests

* Adds a couple more tests for the destination parsing tests

* Removes unneeded line
dbbaughe added a commit that referenced this pull request Jul 29, 2020
…ts (#255) (#262)

* Fixes snapshot issues, adds history mapping update workflow, adds tests

* Adds a couple more tests for the destination parsing tests

* Removes unneeded line
dbbaughe added a commit that referenced this pull request Jul 29, 2020
* Fixes snapshot bugs (#244)

* Simplifies snapshot message

* Handle ConcurrentSnapshotExecutionExceptions during remote transport calls

* Adds try/catch block around WaitForSnapshotStep execute

* Fixes snapshot completed check including failed/aborted and makes status check exhaustive

* Adds snapshot name to ActionProperties

* Uses snapshotName in ActionProperties in snapshot steps

* Adds a couple more integration tests for snapshot action

* Suppress complex method in execute step

* Fixes snapshot issues, adds history mapping update workflow, adds tests (#255)

* Fixes snapshot issues, adds history mapping update workflow, adds tests

* Adds a couple more tests for the destination parsing tests

* Removes unneeded line
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants