Add support in SM Plugin to delete snapshots created manually #1452
Conversation
Tarun-kishore
requested review from
bowenlan-amzn,
Hailong-am,
vikasvb90,
tandonks,
shiv0408 and
soosinha
as code owners
Tarun-kishore
force-pushed
the
feature/snapshot-deletion-support
branch
from
86b255b to
933c487
Compare
|
Hey @bowenlan-amzn — since you were involved in the original issue, would you be open to reviewing the PR when you get a chance? Appreciate your insights! |
|
Will find time to do a review |
There was a problem hiding this comment.
Overrall looks pretty good!
I think as long as user only use the deletion-only policy after fully upgraded. There shouldn't be any issue. We can call this out in the documentation.
It's unfortunate we don't have code coverage, for some reason it hasn't been working for a long time. But I can see tests being added.
| if (job.creation == null) { | ||
| log.warn("Policy creation config becomes null before trying to create snapshot. Reset.") | ||
| return SMResult.Fail( | ||
| metadataBuilder, WorkflowType.CREATION, |
There was a problem hiding this comment.
Maybe we can have a resetCreation just like resetDeletion
There was a problem hiding this comment.
Yes, added resetCreation.
| // Get pattern-based snapshots if pattern is specified | ||
| val patternSnapshots = if (job.deletion.snapshotPattern != null) { | ||
| try { | ||
| client.getSnapshots(job.deletion.snapshotPattern, repository) | ||
| } catch (ex: Exception) { | ||
| log.error("Caught exception while getting snapshots for pattern ${job.deletion.snapshotPattern}.", ex) | ||
| metadataBuilder.setLatestExecution( | ||
| status = SMMetadata.LatestExecution.Status.RETRYING, | ||
| message = "Caught exception while getting snapshots for pattern ${job.deletion.snapshotPattern}.", | ||
| cause = ex, | ||
| ) | ||
| return SMResult.Fail(metadataBuilder, WorkflowType.DELETION) | ||
| } | ||
| } else { | ||
| emptyList() | ||
| } |
There was a problem hiding this comment.
Duplicate code, can extract to one method
There was a problem hiding this comment.
Thanks for catching that. Moved duplicate code to util class
… manually Signed-off-by: Tarun-kishore <tarun2kishore@gmail.com>
Signed-off-by: Tarun-kishore <tarun2kishore@gmail.com>
Yes, I'll update opensearch documentation after this PR is merged to use deletion-only policy after fully upgrading cluster.
Adding a ci check for code coverage (or preferrably new line changes) would be nice, it would ensure that tests are covering all cases. |
Tarun-kishore
force-pushed
the
feature/snapshot-deletion-support
branch
2 times, most recently
from
9ba8dd0 to
461d4d8
Compare
Signed-off-by: Tarun-kishore <tarun2kishore@gmail.com>
Tarun-kishore
force-pushed
the
feature/snapshot-deletion-support
branch
from
461d4d8 to
9f464fb
Compare
|
These CI failures look unrelated to this change, Other integration tests are failing which are not being reproduced in local environment. |
Description
This PR introduces optional creation workflows and snapshot pattern support for Snapshot Management policies, enabling users to create deletion-only policies and manage external snapshots alongside policy-created ones.
Key Changes
1. Optional Creation Field
creationfield optional inSMPolicy, allowing deletion-only policiesVersion.V_3_2_0) for backward compatibility2. Snapshot Pattern Support
snapshotPatternfield toSMPolicy.Deletionto include external snapshots in deletion workflows3. Enhanced State Machine Logic
DeletingStateto retrieve and combine snapshots from both policy and pattern sourcesDeletionFinishedStateto handle completion logic for pattern-based deletions4. Index Mapping Updates
snapshot_patternfield to deletion properties in index mappingsUse Cases Enabled
Backward Compatibility
All changes are fully backward compatible:
Testing
Comprehensive test coverage including:
Related Issues
Resolves #867
Check List
--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.