ARSN-350 Missing Null Version in Lifecycle List of Non-Current Versions #2150

nicolas2bert · 2023-07-12T19:22:37Z

Note: We only support the v1 bucket format for "list lifecycle" in Artesca.

We made the assumption that the first version key stored the current/latest version, which is true in most cases except for "null" versions. In the case of a "null" version, the current version is stored in the master key alone, rather than being stored in both the master key and a new version key. Here's an example of the key structure:

Mkey0: Represents the null version ID.
VKey0<versionID>: Represents a non-current version.

Additionally, we assumed that the versions for a given key were ordered by creation date, from newest to oldest. However, in Ring S3C, for non-current null versions, the metadata version ID is not part of the metadata key id. Therefore, the non-current null version is listed before the current version that has a version ID. Here's an example of the key ordering:

Mkey0: Master version
Vkey0: "Null" Non-current version
VKey0<versionID>: Current version

The listing was using only versions, but because those assumptions are incorrect, we now use both the master and the versions for each given key to ensure that we return the correct non-current versions.

bert-e · 2023-07-12T19:22:40Z

Hello nicolas2bert,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2023-07-12T19:22:47Z

Incorrect fix version

The Fix Version/s in issue ARSN-350 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.1.105

Please check the Fix Version/s of ARSN-350, or the target
branch of this pull request.

bert-e · 2023-07-12T19:55:11Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

jonathan-gramain · 2023-07-13T00:06:12Z

lib/algos/list/delimiterNonCurrent.js

            this.staleDate = this.getLastModified(value);
            return FILTER_ACCEPT;
        }

+        const isCurrentVersion = this.masterKey === key && this.masterVersionId === versionId;


If delete markers are also candidates for expiration, they don't have an associated master key even though they can be current. The way to know whether they are current is if the previous key in the merged M+V listing is a different key, or if it's the very first key of the listing.

Delete markers have a version ID that is correctly ordered.

It doesn't matter if the current delete marker doesn't have a master when listing non-current versions.
The first version key in the listing should store the "delete marker" version and will be skipped.

Also, just to make sure, I added tests for delete marker here: scality/cloudserver#5231

jonathan-gramain

We should also keep in mind that at some point, Artesca may enable null keys on Cloudserver (the change to allow V1 metadata to be more performant), and this listing algo doesn't seem to support them properly. Not needed urgently as currently Cloudserver on Artesca is hard-coded not to use null keys although the code is there, but we should probably track this in some way to avoid forgetting about it the day we decide to switch to null keys in Artesca.

This is where the compat mode (not to use null keys) is forcefully enabled today:

https://github.com/scality/cloudserver/blob/652bf925365e8ec1bd94dec9b54ac61b675da7f2/lib/Config.js#L1611

nicolas2bert · 2023-07-13T15:13:31Z

@jonathan-gramain Is there a ticket for Artesca to support "null keys"?
Based on this comment: "null version compatibility mode is enforced because null keys are not supported by the MongoDB backend.", it seems like no listing algo supports "null keys".

jonathan-gramain · 2023-07-13T21:08:17Z

@nicolas2bert it would be part of the work to use 8.x branches for S3C, I don't think there is an EPIC for this but it should go in there. I'll check this.

The current Arsenal code for DelimiterVersions supports null keys, as it was forward-ported from 7.70 (for example: https://github.com/scality/Arsenal/blob/development/8.1/lib/algos/list/delimiterVersions.ts#L302)

bert-e · 2023-07-14T13:16:55Z

Conflict

There is a conflict between your branch bugfix/ARSN-350/lifecycle-listing and the
destination branch development/8.1.

Please resolve the conflict on the feature branch (bugfix/ARSN-350/lifecycle-listing).

git fetch && \
git checkout origin/bugfix/ARSN-350/lifecycle-listing && \
git merge origin/development/8.1

Resolve merge conflicts and commit

git push origin HEAD:bugfix/ARSN-350/lifecycle-listing

Note: We only support the v1 bucket format for "list lifecycle" in Artesca. We made the assumption that the first version key stored the current/latest version, which is true in most cases except for "null" versions. In the case of a "null" version, the current version is stored in the master key alone, rather than being stored in both the master key and a new version key. Here's an example of the key structure: Mkey0: Represents the null version ID. VKey0<versionID>: Represents a non-current version. Additionally, we assumed that the versions for a given key were ordered by creation date, from newest to oldest. However, in Ring S3C, for non-current null versions, the metadata version ID is not part of the metadata key id. Therefore, the non-current null version is listed before the current version that has a version ID. Here's an example of the key ordering: Mkey0: Master version Vkey0: "null" non-current version VKey0<versionID>: Current version The listing was using only versions, but because those assumptions are incorrect, we now use both the master and the versions for each given key to ensure that we return the correct non-current versions.

bert-e · 2023-07-14T13:21:42Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

codecov-commenter · 2023-07-14T13:29:18Z

Codecov Report

Merging #2150 (7c4f461) into development/8.1 (8716fee) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@                 Coverage Diff                 @@
##           development/8.1    #2150      +/-   ##
===================================================
+ Coverage            64.34%   64.41%   +0.06%     
===================================================
  Files                  210      210              
  Lines                16381    16412      +31     
  Branches              3316     3324       +8     
===================================================
+ Hits                 10541    10572      +31     
  Misses                5825     5825              
  Partials                15       15

Impacted Files	Coverage Δ
lib/algos/list/delimiterNonCurrent.js	`97.52% <100.00%> (+0.78%)`	⬆️
...orage/metadata/mongoclient/MongoClientInterface.js	`62.55% <100.00%> (+0.07%)`	⬆️

nicolas2bert · 2023-07-14T13:32:05Z

@bert-e approve

bert-e · 2023-07-14T13:32:16Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/8.1

The following branches will NOT be impacted:

development/6.4
development/7.10
development/7.4
development/7.70

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2023-07-14T13:32:27Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.1

The following branches have NOT changed:

development/6.4
development/7.10
development/7.4
development/7.70

Please check the status of the associated issue ARSN-350.

Goodbye nicolas2bert.

nicolas2bert force-pushed the bugfix/ARSN-350/lifecycle-listing branch from 19c9ac1 to 98567d0 Compare July 12, 2023 19:46

nicolas2bert force-pushed the bugfix/ARSN-350/lifecycle-listing branch from 98567d0 to ae1fd6e Compare July 12, 2023 20:14

nicolas2bert mentioned this pull request Jul 12, 2023

CLDSRV-412 Test null version in Lifecycle list of non-current versions scality/cloudserver#5231

Merged

nicolas2bert requested a review from jonathan-gramain July 12, 2023 20:19

nicolas2bert force-pushed the bugfix/ARSN-350/lifecycle-listing branch from ae1fd6e to b9e1a1e Compare July 12, 2023 20:38

jonathan-gramain reviewed Jul 13, 2023

View reviewed changes

jonathan-gramain approved these changes Jul 13, 2023

View reviewed changes

naren-rajendran approved these changes Jul 13, 2023

View reviewed changes

nicolas2bert added 2 commits July 14, 2023 09:20

bump version

7c4f461

nicolas2bert force-pushed the bugfix/ARSN-350/lifecycle-listing branch from 732f398 to 7c4f461 Compare July 14, 2023 13:21

bert-e merged commit 7c4f461 into development/8.1 Jul 14, 2023

bert-e deleted the bugfix/ARSN-350/lifecycle-listing branch July 14, 2023 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARSN-350 Missing Null Version in Lifecycle List of Non-Current Versions #2150

ARSN-350 Missing Null Version in Lifecycle List of Non-Current Versions #2150

nicolas2bert commented Jul 12, 2023 •

edited

Loading

bert-e commented Jul 12, 2023

bert-e commented Jul 12, 2023

bert-e commented Jul 12, 2023

jonathan-gramain Jul 13, 2023

nicolas2bert Jul 13, 2023

nicolas2bert Jul 13, 2023

jonathan-gramain left a comment

nicolas2bert commented Jul 13, 2023

jonathan-gramain commented Jul 13, 2023

bert-e commented Jul 14, 2023

bert-e commented Jul 14, 2023

codecov-commenter commented Jul 14, 2023

nicolas2bert commented Jul 14, 2023

bert-e commented Jul 14, 2023

bert-e commented Jul 14, 2023

ARSN-350 Missing Null Version in Lifecycle List of Non-Current Versions #2150

ARSN-350 Missing Null Version in Lifecycle List of Non-Current Versions #2150

Conversation

nicolas2bert commented Jul 12, 2023 • edited Loading

bert-e commented Jul 12, 2023

Hello nicolas2bert,

bert-e commented Jul 12, 2023

Incorrect fix version

bert-e commented Jul 12, 2023

Waiting for approval

jonathan-gramain Jul 13, 2023

Choose a reason for hiding this comment

nicolas2bert Jul 13, 2023

Choose a reason for hiding this comment

nicolas2bert Jul 13, 2023

Choose a reason for hiding this comment

jonathan-gramain left a comment

Choose a reason for hiding this comment

nicolas2bert commented Jul 13, 2023

jonathan-gramain commented Jul 13, 2023

bert-e commented Jul 14, 2023

Conflict

bert-e commented Jul 14, 2023

Waiting for approval

codecov-commenter commented Jul 14, 2023

Codecov Report

nicolas2bert commented Jul 14, 2023

bert-e commented Jul 14, 2023

In the queue

bert-e commented Jul 14, 2023

nicolas2bert commented Jul 12, 2023 •

edited

Loading