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.

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

Refactors the managed index runner to work with extensions #262

Merged

Conversation

downsrob
Copy link
Contributor

@downsrob downsrob commented Feb 7, 2022

Issue #, if available:
NA

Description of changes:

  • Adds NewClusterEventListeners so that extensions may react to cluster events with the NewCluster type
  • Supports transition condition evaluation for non-default index types by using the IndexMetadataProvider to get the index creation date. Adds caching of the index creation date for all index types to the ManagedIndexRunner
  • Adds a setting to deleteIndexMetadataAfterFinish to actions. When set to true, the managed index config and metadata for the index that the policy/action is attached to will be deleted following action completion. For in-cluster indices this should be handled automatically, so this setting is just to support this behavior for custom actions on non-default index types.
  • Adds IndexMetadataProvider implementation from Makes rest APIs use new metadata service #245 Changes to the IndexMetadataProvider may need to be made to either PR based on the other's comments. This is awkward, but it is a blocker otherwise.
  • Code cleanup

CheckList:

  • 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.

@downsrob downsrob marked this pull request as ready for review February 7, 2022 21:28
@downsrob downsrob requested a review from a team February 7, 2022 21:28
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

Merging #262 (ade5565) into development-extension (40544b8) will decrease coverage by 0.12%.
The diff coverage is 41.08%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##             development-extension     #262      +/-   ##
===========================================================
- Coverage                    73.20%   73.07%   -0.13%     
- Complexity                    1836     1856      +20     
===========================================================
  Files                          252      252              
  Lines                        10969    11047      +78     
  Branches                      1697     1726      +29     
===========================================================
+ Hits                          8030     8073      +43     
- Misses                        1950     1979      +29     
- Partials                       989      995       +6     
Impacted Files Coverage Δ
...ment/indexstatemanagement/IndexMetadataProvider.kt 65.62% <ø> (ø)
...ent/indexstatemanagement/util/ManagedIndexUtils.kt 63.39% <25.00%> (-6.10%) ⬇️
...agement/indexstatemanagement/ManagedIndexRunner.kt 36.34% <30.50%> (+3.00%) ⬆️
...anagement/step/transition/AttemptTransitionStep.kt 76.00% <50.00%> (-14.13%) ⬇️
...ndexmanagement/indexstatemanagement/model/State.kt 88.88% <80.00%> (-1.59%) ⬇️
...temanagement/opensearchapi/OpenSearchExtensions.kt 68.96% <85.71%> (+0.36%) ⬆️
...pensearch/indexmanagement/IndexManagementPlugin.kt 88.57% <100.00%> (+0.04%) ⬆️
...t/indexstatemanagement/action/TransitionsAction.kt 100.00% <100.00%> (ø)
...nt/indexstatemanagement/model/destination/Chime.kt 40.90% <0.00%> (-13.64%) ⬇️
...ndexstatemanagement/IndexStateManagementHistory.kt 77.30% <0.00%> (-1.42%) ⬇️
... and 18 more

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 40544b8...ade5565. Read the comment docs.

@downsrob downsrob requested review from thalurur, bowenlan-amzn, dbbaughe and a team February 25, 2022 18:43
downsrob added 2 commits March 3, 2022 21:21
Signed-off-by: Robert Downs <[email protected]>

Adds NewClusterEventListeners and refactors Transition to work with custom actions

Signed-off-by: Robert Downs <[email protected]>

Marks blocked actions list as deprecated

Signed-off-by: Clay Downs <[email protected]>

Asserts the deprecation warning after adding to allow list in test

Signed-off-by: Robert Downs <[email protected]>

Removes allow_list test

Signed-off-by: Robert Downs <[email protected]>
Signed-off-by: Robert Downs <[email protected]>
Signed-off-by: Robert Downs <[email protected]>
Comment on lines 96 to 104
final fun isFinishedSuccessfully(managedIndexMetaData: ManagedIndexMetaData): Boolean {
val policyRetryInfo = managedIndexMetaData.policyRetryInfo
if (policyRetryInfo == null || policyRetryInfo.failed) return false
val actionMetaData = managedIndexMetaData.actionMetaData
if (actionMetaData == null || actionMetaData.failed || actionMetaData.name != this.type) return false
val stepMetaData = managedIndexMetaData.stepMetaData
if (stepMetaData == null || !isLastStep(stepMetaData.name) || stepMetaData.stepStatus != Step.StepStatus.COMPLETED) return false
return true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this new code or just existing code refactored to this place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a generalization of the isSuccessfulDelete function:
https://github.com/opensearch-project/index-management/blob/main/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/util/ManagedIndexUtils.kt#L430-L433
The purpose is that if an extension creates an action to delete their off cluster index, then they can set deleteIndexMetadataAfterFinish to true in the custom action, and then after it isFinishedSuccessfully, the metadata will be deleted for the action.
Thinking about this a bit more now, I can add an additional validation here to make it so these extension actions which set deleteIndexMetadataAfterFinish = true can't transition or have actions after the delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do see a logical error in the code though, if (policyRetryInfo == null || policyRetryInfo.failed) return false would mean that the action would never be noted as isFinishedSuccessfully if the policyRetryInfo was null. It is initialized with the metadata so it shouldn't be a problem but I will change that as well

Signed-off-by: Robert Downs <[email protected]>
@downsrob downsrob requested a review from dbbaughe March 4, 2022 22:34
dbbaughe
dbbaughe previously approved these changes Mar 4, 2022
Signed-off-by: Clay Downs <[email protected]>
Copy link
Member

@bowenlan-amzn bowenlan-amzn left a comment

Choose a reason for hiding this comment

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

Good good

@downsrob downsrob merged commit dc05f52 into opensearch-project:development-extension Mar 6, 2022
@downsrob downsrob deleted the runner-refactor branch March 6, 2022 19:12
downsrob pushed a commit to downsrob/index-management that referenced this pull request Mar 8, 2022
Signed-off-by: Ravi Thaluru <[email protected]>

Base commit to clean up old action interfaces and disabling all ISM related tests (opensearch-project#218)

Signed-off-by: Ravi Thaluru <[email protected]>

Implement DeleteAcion using new interface (opensearch-project#221)

Signed-off-by: Ravi Thaluru <[email protected]>

Adding base logic to transition step to enable policy execution (opensearch-project#223)

Signed-off-by: Ravi Thaluru <[email protected]>

Support close action using new interface (opensearch-project#224)

* Implement close action

Signed-off-by: Annie Lee <[email protected]>

* Update functions

Signed-off-by: Annie Lee <[email protected]>

* Update AttemptCloseStepTests.kt

Signed-off-by: Annie Lee <[email protected]>

* Mark a test as private for now

Since TransitionAction is not yet implemented. Marking a test as private to avoid integ test failure

Signed-off-by: Annie Lee <[email protected]>

* Update CloseActionIT.kt

Signed-off-by: Annie Lee <[email protected]>

Implement ReadOnlyAction using new interface (opensearch-project#227)

* Refactors ReadOnlyAction

Signed-off-by: Robert Downs <[email protected]>

Implement ReadWriteAction using new interface (opensearch-project#228)

Signed-off-by: Clay Downs <[email protected]>

Implement OpenAction using new interface (opensearch-project#230)

* Support open action

Signed-off-by: Annie Lee <[email protected]>

* Update AttemptOpenStep.kt

Signed-off-by: Annie Lee <[email protected]>

* Add close action test

Signed-off-by: Annie Lee <[email protected]>

* Add open action related tests

Signed-off-by: Annie Lee <[email protected]>

* Add open action test round trip

Signed-off-by: Annie Lee <[email protected]>

* Fix open action xcontent test

Signed-off-by: Annie Lee <[email protected]>

* Modify XContentTests for better comparison

Signed-off-by: Annie Lee <[email protected]>

* Update XContentTests.kt

Signed-off-by: Annie Lee <[email protected]>

Implements RolloverAction with new interface, fixes default action retry commit (opensearch-project#231)

* Implements rollover action with new interface, fixes default action retry

Signed-off-by: Clay Downs <[email protected]>

Support ReplicaCountAction using new interface (opensearch-project#233)

Signed-off-by: Annie Lee <[email protected]>

Refactors rollup action and enables multi step actions (opensearch-project#235)

Signed-off-by: Robert Downs <[email protected]>

Refactors Notification action with new interface (opensearch-project#238)

* Refactors notification action with new interface

Signed-off-by: Robert Downs <[email protected]>

Upgrades detekt version, fixes flaky tests (opensearch-project#254)

* Upgrades detekt version to 1.17.1 (opensearch-project#252)

* Adds detekt ignores to not-yet-refactored files

* Fixes flaky rollup/transform explain IT (opensearch-project#247)

Signed-off-by: Robert Downs <[email protected]>

Support force merge action using new interface (opensearch-project#256)

* Support force merge action

Signed-off-by: Annie Lee <[email protected]>

Refactors Snapshot action to use new interface (opensearch-project#253)

* Refactors snapshot action to use new interface

Signed-off-by: Clay Downs <[email protected]>

Support index priority action using new interface (opensearch-project#257)

Signed-off-by: Annie Lee <[email protected]>

Support Allocation action using new interface (opensearch-project#246)

* Support Allocation action using new interface

Signed-off-by: Annie Lee <[email protected]>

* Pass in required parameter

Signed-off-by: Annie Lee <[email protected]>

* Update AttemptAllocationStep.kt

Adding correct const

Signed-off-by: Annie Lee <[email protected]>

* Update AttemptAllocationStep.kt

Typo in message

Signed-off-by: Annie Lee <[email protected]>

* Debug tests

Signed-off-by: Annie Lee <[email protected]>

* Update XContentTests.kt

Signed-off-by: Annie Lee <[email protected]>

* Revert "Debug tests"

This reverts commit d7123bd.

Signed-off-by: Annie Lee <[email protected]>

* Update IndexPolicyRequestTests.kt

Signed-off-by: Annie Lee <[email protected]>

* Support force merge action using new interface (opensearch-project#256)

* Support force merge action

Signed-off-by: Annie Lee <[email protected]>

* Update AllocationActionIT.kt

Signed-off-by: Annie Lee <[email protected]>

* Revert "Update IndexPolicyRequestTests.kt"

This reverts commit bd34e2e.

Signed-off-by: Annie Lee <[email protected]>

* Add debug log

Signed-off-by: Annie Lee <[email protected]>

* Remove debug log and change order of parameters

Signed-off-by: Annie Lee <[email protected]>

* Update after comments

Fix order of include exclude parameter in parser
Add check to tests
Remove "Config" in message

Signed-off-by: Annie Lee <[email protected]>

Merging changes from main branch (opensearch-project#259)

Signed-off-by: Ravi Thaluru <[email protected]>

merge with main (opensearch-project#270)

Signed-off-by: Ravi Thaluru <[email protected]>

Adds custom action parsing (opensearch-project#273)

* Adds custom action parsing logic

Signed-off-by: Robert Downs <[email protected]>

Refactor coordinator to support create and delete index events when different index types exist on cluster (opensearch-project#272)

* Refactor coordinator to support different index type create and delete events and sweep

Signed-off-by: Ravi Thaluru <[email protected]>

introducing custom uuid setting in SPI (opensearch-project#278)

* introducing custom uuid setting in SPI

Signed-off-by: Ravi Thaluru <[email protected]>

Refactors the managed index runner to work with extensions (opensearch-project#262)

* Adds index creation date

Signed-off-by: Robert Downs <[email protected]>

Adds NewClusterEventListeners and refactors Transition to work with custom actions

Signed-off-by: Robert Downs <[email protected]>

Marks blocked actions list as deprecated

Signed-off-by: Clay Downs <[email protected]>

Asserts the deprecation warning after adding to allow list in test

Signed-off-by: Robert Downs <[email protected]>

Removes allow_list test

Signed-off-by: Robert Downs <[email protected]>

* Fix additional rebase issues

Signed-off-by: Robert Downs <[email protected]>

* Fixes failing tests

Signed-off-by: Robert Downs <[email protected]>

* changes based on comments

Signed-off-by: Robert Downs <[email protected]>

* Adds additional comments

Signed-off-by: Clay Downs <[email protected]>

Makes rest APIs use new metadata service (opensearch-project#245)

* Makes rest API use new metadata service

Signed-off-by: Robert Downs <[email protected]>

Adds missed flags (opensearch-project#286)

Signed-off-by: Clay Downs <[email protected]>

Ports over show-applied-policies logic (opensearch-project#287)

Signed-off-by: Clay Downs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants