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.

Sign up for GitHub

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

Jump to bottom

Detect rollover transient failure and continues execution #866

Closed
wants to merge 8 commits into from
Closed

Detect rollover transient failure and continues execution #866

wants to merge 8 commits into from

Conversation

ronnaksaxena
Copy link
Contributor

Issue 33

Description of changes:
•Abstracted logic to handle nonidempotent failures that could cause inapropriate error log "Previous action was not able to update IndexMetaData."
•Detects transient failure for rollover step with testing

CheckList:

[ x] Commits are signed per the DCO using --signoff
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ronnaksaxena ronnaksaxena changed the title detect rollover transient failure and integ test Detect rollover transient failure and continues execution Jul 19, 2023
@codecov
Copy link

codecov bot commented Jul 19, 2023 •
edited
Loading

Codecov Report

Merging #866 (b4cb4d8) into 2.x (a7dc8c2) will decrease coverage by 0.11%.
The diff coverage is 48.48%.

@@             Coverage Diff              @@
##                2.x     #866      +/-   ##
============================================
- Coverage     76.06%   75.95%   -0.11%     
- Complexity     2908     2909       +1     
============================================
  Files           366      366              
  Lines         16558    16589      +31     
  Branches       2397     2404       +7     
============================================
+ Hits          12595    12601       +6     
- Misses         2582     2594      +12     
- Partials       1381     1394      +13
Files Changed Coverage Δ
...ement/step/forcemerge/AttemptCallForceMergeStep.kt 66.03% <0.00%> (-1.27%) ⬇️
...ement/step/notification/AttemptNotificationStep.kt 9.09% <0.00%> (-0.29%) ⬇️
...exstatemanagement/step/shrink/AttemptShrinkStep.kt 60.86% <0.00%> (-0.90%) ⬇️
...atemanagement/step/snapshot/AttemptSnapshotStep.kt 92.85% <0.00%> (-1.12%) ⬇️
...agement/indexstatemanagement/ManagedIndexRunner.kt 48.23% <50.00%> (+0.44%) ⬆️
...atemanagement/step/rollover/AttemptRolloverStep.kt 63.58% <55.55%> (-0.75%) ⬇️

... and 13 files with indirect coverage changes

@ronnaksaxena
Revised code based on comments
04b5210 
Signed-off-by: Ronnak Saxena <[email protected]>
AWSHurneyt
AWSHurneyt reviewed Jul 24, 2023
View reviewed changes
src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexRunner.kt Show resolved Hide resolved
...opensearch/indexmanagement/indexstatemanagement/step/forcemerge/AttemptCallForceMergeStep.kt Show resolved Hide resolved
...opensearch/indexmanagement/indexstatemanagement/step/forcemerge/AttemptCallForceMergeStep.kt Show resolved Hide resolved
...opensearch/indexmanagement/indexstatemanagement/step/notification/AttemptNotificationStep.kt Show resolved Hide resolved
...opensearch/indexmanagement/indexstatemanagement/step/notification/AttemptNotificationStep.kt Show resolved Hide resolved
.../kotlin/org/opensearch/indexmanagement/indexstatemanagement/step/shrink/AttemptShrinkStep.kt Show resolved Hide resolved
.../kotlin/org/opensearch/indexmanagement/indexstatemanagement/step/shrink/AttemptShrinkStep.kt Show resolved Hide resolved
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
src/test/kotlin/org/opensearch/indexmanagement/indexstatemanagement/action/RolloverActionIT.kt Outdated Show resolved Hide resolved
eirsep
eirsep reviewed Jul 25, 2023
View reviewed changes
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt
val response: ClusterStateResponse = client.suspendUntil {
client.admin().cluster().state(ClusterStateRequest(), it)
}
// If the index was rolled over, this is a transient failure
Copy link
Member

Choose a reason for hiding this comment

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

if index was rolled over that's the best case scenario.
what if the rollover failed beacuse of a lack of disk space.
even that is a transient failure because user might delete data to create space or add more nodes etc.
What if there was a circuit breaker exception which was later resolved. even in that case rollover would fail but in the upcoming executions of policy it might not encounter that error.

@ronnaksaxena ronnaksaxena mentioned this pull request Jul 26, 2023
Open
ronnaksaxena
ronnaksaxena commented Jul 27, 2023
View reviewed changes
...opensearch/indexmanagement/indexstatemanagement/step/forcemerge/AttemptCallForceMergeStep.kt
@@ -104,6 +106,10 @@ class AttemptCallForceMergeStep(private val action: ForceMergeAction) : Step(nam
}

override fun isIdempotent() = false
override suspend fun isTransientFailure(client: Client, stepContext: StepContext, managedIndexMetaData: ManagedIndexMetaData): Boolean {
// TODO implement logic for detecting transient failure in attempt call force merge step
return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test for this on https://github.com/opensearch-project/index-management/pull/866/files#diff-b19d511f03f9bd3a41f9bfcd7efa52e7521e485bbb706c99f96aec2e64fee230R47

ronnaksaxena
ronnaksaxena commented Jul 27, 2023
View reviewed changes
...opensearch/indexmanagement/indexstatemanagement/step/notification/AttemptNotificationStep.kt
@@ -66,6 +68,10 @@ class AttemptNotificationStep(private val action: NotificationAction) : Step(nam
}

override fun isIdempotent(): Boolean = false
override suspend fun isTransientFailure(client: Client, stepContext: StepContext, managedIndexMetaData: ManagedIndexMetaData): Boolean {
// TODO implement logic for detecting transient failure in attempt notification step
return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test for this: https://github.com/opensearch-project/index-management/pull/866/files#diff-941c5b84c6110ba6de9b893313de9234ae33a4a265f28fdc0c083bacdc6c33abR47

ronnaksaxena
ronnaksaxena commented Jul 27, 2023
View reviewed changes
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt
}
if (aliasName == null) {
logger.error("Index $indexName has no alias attached to it. Not a Transient Failure in step attemptRolloverStep")
isTransientFailure = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test for this: https://github.com/opensearch-project/index-management/pull/866/files#diff-65640a89df7a4b5fd19bd152ba3930d3745f8ada06a064195b6fb7ec75da89a7R48

ronnaksaxena
ronnaksaxena commented Jul 27, 2023
View reviewed changes
.../kotlin/org/opensearch/indexmanagement/indexstatemanagement/step/shrink/AttemptShrinkStep.kt
@@ -135,6 +136,10 @@ class AttemptShrinkStep(private val action: ShrinkAction) : ShrinkStep(name, tru
}

override fun isIdempotent() = false
override suspend fun isTransientFailure(client: Client, stepContext: StepContext, managedIndexMetaData: ManagedIndexMetaData): Boolean {
// TODO implement logic for detecting transient failure in attempt shrink step
return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test for this: https://github.com/opensearch-project/index-management/pull/866/files#diff-448d720352c87f3274c41dda9478e8c67102371b956c8a02ad8ff025e9678e58R48

ronnaksaxena
ronnaksaxena commented Jul 27, 2023
View reviewed changes
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt
@@ -155,6 +157,10 @@ class AttemptSnapshotStep(private val action: SnapshotAction) : Step(name) {
}

override fun isIdempotent(): Boolean = false
override suspend fun isTransientFailure(client: Client, stepContext: StepContext, managedIndexMetaData: ManagedIndexMetaData): Boolean {
// TODO implement logic for detecting transient failure in attempt snapshot step
return false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test for this: https://github.com/opensearch-project/index-management/pull/866/files#diff-984f43aac192fa8a3932172668d7a7f0359955c0a60079c01927f775ee32b900R146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AWSHurneyt AWSHurneyt AWSHurneyt left review comments

@bowenlan-amzn bowenlan-amzn bowenlan-amzn left review comments

@eirsep eirsep eirsep left review comments

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02

@lezzago lezzago Awaiting requested review from lezzago

@praveensameneni praveensameneni Awaiting requested review from praveensameneni

@xluo-aws xluo-aws Awaiting requested review from xluo-aws

@gaobinlong gaobinlong Awaiting requested review from gaobinlong

@Hailong-am Hailong-am Awaiting requested review from Hailong-am Hailong-am is a code owner

@amsiglan amsiglan Awaiting requested review from amsiglan

@sbcd90 sbcd90 Awaiting requested review from sbcd90

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ronnaksaxena @eirsep @bowenlan-amzn @AWSHurneyt