ilm: Handle DeleteAllVersions action differently for DEL markers

[krisis]

Community Contribution License

All community contributions in this pull request are licensed to the project maintainers
under the terms of the Apache 2 license.
By creating this pull request I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 license.

Description

This PR brings about two changes, one adds a new lifecycle action, DelMarkerExpiration and another modifies the behavior of an existing lifecycle configuration element, ExpiredObjectDeleteAllVersions.

Companion PRs

• Add support for DelMarkerExpiration element minio-go#1959
• event-add: Add DelMarkerExpiration event type mc#4913

Addition of DelMarkerExpiration

This ILM action removes all versions of an object if its latest version is a DEL marker.

    <DelMarkerObjectExpiration>
        <Days> 10 </Days>
    </DelMarkerObjectExpiration>

1. Applies only to objects whose,

   • latest version is a DEL marker.
   • satisfies the number of days criteria

2. Deletes all versions of this object

3. Associated rule can't have tag-based filtering

Modification to ExpiredObjectDeleteAllVersions

Will apply only to objects whose latest version is not a DEL marker.

Motivation and Context

This is a breaking change to how ExpiredObejctDeleteAllVersions functions today. This is necessary to avoid the following highly probable footgun scenario in the future.

Scenario:
User uses tags based filtering to select the time to live(TTL) for an object. Application sometimes deletes objects too, making their latest version a DEL marker. Previous implementation skipped tag based filters if latest version was DEL marker, voiding the tag based TTL. User is surprised to find objects expired sooner than expected.

How to test this PR?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Optimization (provides speedup with no functional changes)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• Fixes a regression (If yes, please add commit-id or PR # here)
• Unit tests added/updated
• Internal documentation updated
• Create a documentation update request here

[harshavardhana]

Also don't forget to update docs/bucket/lifecycle

[vadmeste]

Few comments

[poornas]

This feature may interact poorly with replication if replication is set up as active -> passive (read only DR). Depending on the duration of ilm expiry rule above - there is a chance that replication of non current versions is not complete. Either the docs needs to warn about this possibility or it needs to be addressed by verifying all underlying versions are in COMPLETED status.

[poornas]

Another q - Since this op is doing a prefix delete, there may be a window between queuing and acting upon this expiry task where a new version can be uploaded for the same object. Seems like expiry task is not actually verifying that current Objectinfo is indeed the latest before proceeding

Also, seems like if one or more versions in the stack is in tiered status, clean up will not be done

[klauspost]

Associated rule can't have tag-based filtering

Why is this limitation there?

[krisis]

Associated rule can't have tag-based filtering

Why is this limitation there?

DelMarkerExpiration applies exclusively to DEL markers. Tag-based filtering doesn't apply to DEL markers, since they can't have tags. Similar restrictions are present in S3's lifecycle elements too. e.g,

When you use the ExpiredObjectDeleteMarker S3 Lifecycle action, the rule cannot specify a tag-based filter.

(ref: https://docs.aws.amazon.com/AmazonS3/latest/userguide/lifecycle-configuration-examples.html; just above Example 8)

[klauspost]

DelMarkerExpiration applies exclusively to DEL markers.

👍🏼 It was also my impression that it doesn't make sense. So it isn't a limitation as such - just a natural consequence of delete markers not having tags.

[harshavardhana]

Another q - Since this op is doing a prefix delete, there may be a window between queuing and acting upon this expiry task where a new version can be uploaded for the same object. Seems like expiry task is not actually verifying that current Objectinfo is indeed the latest before proceeding

latest objectInfo() must be verified first and it is already. opts.DeletePrefix when set and expiration is "expire=true"

        if opts.DeletePrefix {
                if opts.Expiration.Expire {
                        // Expire all versions expiration must still verify the state() on disk
                        // via a getObjectInfo() call as follows, any read quorum issues we
                        // must not proceed further for safety reasons. attempt a MRF heal
                        // while we see such quorum errors.
                        goi, _, gerr := er.getObjectInfoAndQuorum(ctx, bucket, object, opts)
                        if gerr != nil && goi.Name == "" {
                                if _, ok := gerr.(InsufficientReadQuorum); ok {
                                        // Add an MRF heal for next time.
                                        er.addPartial(bucket, object, opts.VersionID)

                                        return objInfo, InsufficientWriteQuorum{}
                                }
                                return objInfo, gerr
                        }

[poornas]

Another q - Since this op is doing a prefix delete, there may be a window between queuing and acting upon this expiry task where a new version can be uploaded for the same object. Seems like expiry task is not actually verifying that current Objectinfo is indeed the latest before proceeding

latest objectInfo() must be verified first and it is already. opts.DeletePrefix when set and expiration is "expire=true"

        if opts.DeletePrefix {
                if opts.Expiration.Expire {
                        // Expire all versions expiration must still verify the state() on disk
                        // via a getObjectInfo() call as follows, any read quorum issues we
                        // must not proceed further for safety reasons. attempt a MRF heal
                        // while we see such quorum errors.
                        goi, _, gerr := er.getObjectInfoAndQuorum(ctx, bucket, object, opts)
                        if gerr != nil && goi.Name == "" {
                                if _, ok := gerr.(InsufficientReadQuorum); ok {
                                        // Add an MRF heal for next time.
                                        er.addPartial(bucket, object, opts.VersionID)

                                        return objInfo, InsufficientWriteQuorum{}
                                }
                                return objInfo, gerr
                        }

while this checks the version id that qualified for ilm exists, it doesn't verify it is the latest version

[harshavardhana]

while this checks the version id that qualified for ilm exists, it doesn't verify it is the latest version

that we should add @krisis

[krisis]

while this checks the version id that qualified for ilm exists, it doesn't verify it is the latest version

Good catch! I have addressed this.