Skip to content

[NSFS] Fix version listing #8413

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

Merged

Conversation

tangledbytes
Copy link
Member

Explain the changes

This PR attempts to fix version listing. Previously, there was an assumption that when both KeyMarker and NextIdVersion is provided then only the items belonging to that KeyMarker starting from the given IdVersion should be returned. This behaviour sounds sane but upon checking with AWS S3 it turns out that AWS would keep on listing items beyond KeyMarker as well (and the way to control the key is by using Prefix and MaxKeys).

This PR simply removes that additional check.

Issues: Fixed #xxx / Gap #xxx

Fixes #8406

Testing Instructions:

  1. sudo jest --testRegex=jest_tests/test_versioning_conc -t 'concurrent puts & list versions' (After NSFS | Versioning | Concurrency tests #8405 gets merged)
  • Doc added/updated
  • Tests added

Signed-off-by: Utkarsh Srivastava <[email protected]>
@romayalon
Copy link
Contributor

romayalon commented Sep 26, 2024

@tangledbytes

  1. While taking this fix and putting it in my test branch, and passing in the listObjectVersions request only a NextVersionIdMarker (KeyMarker is not provided) I experience still the same issue (always getting the first 1000 objects) is this the expected behavior?
  2. I think we need functionality tests here and we shouldn't rely on the concurrency test

@tangledbytes
Copy link
Member Author

@tangledbytes

  1. While taking this fix and putting it in my test branch, and passing in the listObjectVersions request only a NextVersionIdMarker (KeyMarker is not provided) I experience still the same issue (always getting the first 1000 objects) is this the expected behavior?
  2. I think we need functionality tests here and we shouldn't rely on the concurrency test

@romayalon,

  1. I think the implementation expects it, we have the check for this at the s3 layer and AWS S3 throws error as well if we don't KeyMarker with version id. And that makes sense right? The fact that keymarker and versionid share the same prefix is a consequence of our implementation and not inherent to S3 so it does makes sense that both are required at the same time as version id without the key name might not make much sense.
  2. Yes, I can add the test for the same here.

Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

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

Let's merge it and add functionality tests on a follow-up PR, i'll remove the skip on my test after you merge it.

@tangledbytes tangledbytes merged commit e24e59a into noobaa:master Sep 26, 2024
10 checks passed
@shirady shirady mentioned this pull request Sep 29, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NSFS | Versioning | ListObjectVersions | Version ID marker is not working
2 participants