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

Metadata-prefetch (aka Recursive listing or ls -R during mount) #1930

Merged
merged 36 commits into from
May 24, 2024

Conversation

gargnitingoogle
Copy link
Collaborator

@gargnitingoogle gargnitingoogle commented May 20, 2024

Description

Metadata-prefetch (aka Recursive listing or ls -R during mount)

Metadata-prefetch is equivalent to running ls -R
on the mount-directory to prime/prefill/prefetch the
metadata caches (both type and stat).

This change adds support for mountconfig-flag metadata-prefetch-on-mount
in config-file. The type of value passed is a string.
The supported values are: (a) disabled, (b) sync, (c)
async.
If not set, the value disabled is taken by default.

If value disabled is set, then no difference from the existing behaviour.

If value sync is set, then mount is blocked
until the metadata prefetch is complete. In this mode, if
the metadata-prefetch fails, the mount itself fails.

If value is set to async, then mount is not
blocked, but metadata prefetch is
initiated in a go-routine and will complete sometime
later. In this case, if the metadata-prefetch fails,
then an error is logged in the GCSFuse log-file.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Done
    • Logs with metadata-prefetch-on-mount: disabled or with metadata-prefetch-on-mount not set:
       message="Mounting file system \"gargnitin-fuse-test-bucket1\"..."
       message="File system has been successfully mounted."
    • Logs with metadata-prefetch-on-mount: sync:
       message="Mounting file system \"gargnitin-fuse-test-bucket1\"..."
       message="Started recursive metadata-prefetch of directory:  \"blah/blah/blah\" ..."
       message="... Completed recursive metadata-prefetch of directory: \"blah/blah/blah\". Number of items discovered: 61"
       message="File system has been successfully mounted."
    • Logs with metadata-prefetch-on-mount: async:
       message="Mounting file system \"gargnitin-fuse-test-bucket1\"..."
       message="File system has been successfully mounted."
       message="Started recursive metadata-prefetch of directory:  \"blah/blah/blah\" ..."
      Here, control comes back to the user. Later log, after some time,
        message="... Completed recursive metadata-prefetch of directory: \"blah/blah/blah\". Number of items discovered: 61"
  2. Unit tests - Ran locally and all passed.
  3. Integration tests - Ran through presubmit.

Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 60.76%. Comparing base (1d0f0c0) to head (e554469).

Files Patch % Lines
main.go 37.50% 25 Missing ⚠️
flags.go 95.23% 1 Missing ⚠️
mount.go 0.00% 1 Missing ⚠️
tools/mount_gcsfuse/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1930      +/-   ##
==========================================
- Coverage   61.25%   60.76%   -0.50%     
==========================================
  Files         130      130              
  Lines       12350    12397      +47     
==========================================
- Hits         7565     7533      -32     
- Misses       4445     4526      +81     
+ Partials      340      338       -2     
Flag Coverage Δ
unittests 60.76% <55.55%> (-0.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch 3 times, most recently from e4ab612 to ceddacc Compare May 20, 2024 11:46
@gargnitingoogle gargnitingoogle added the execute-integration-tests Run only integration tests label May 20, 2024
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch 2 times, most recently from b65d7b7 to 9d528d5 Compare May 21, 2024 07:11
@gargnitingoogle gargnitingoogle changed the title DontReview listing during mount Metadata-prefetch (aka Recursive listing or ls -R during mount) May 21, 2024
@gargnitingoogle gargnitingoogle marked this pull request as ready for review May 21, 2024 10:48
@gargnitingoogle gargnitingoogle requested review from ashmeenkaur and a team as code owners May 21, 2024 10:48
@gargnitingoogle
Copy link
Collaborator Author

Integration tests are currently running. I'll add result for that once they finish.

@gargnitingoogle
Copy link
Collaborator Author

Integration tests are currently running. I'll add result for that once they finish.

Tests have passed. logs

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch 2 times, most recently from 0567424 to 363c7c6 Compare May 22, 2024 09:45
@gargnitingoogle
Copy link
Collaborator Author

@kislaykishore As discussed offline, I have replaced config-file metadata-prefetch-mode with cli flag of same name and type. PTAL.
I have removed the code for the config-file flag in commit, which can be reverted in future, if we ever want to have both the ways of setting the configuration.

@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch from 363c7c6 to 55cb967 Compare May 22, 2024 10:06
flags.go Outdated Show resolved Hide resolved
flags.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch from 8426c15 to 8b4fca4 Compare May 22, 2024 11:42
.. to metadata-prefetch-on-mount
Change "synchronous" to "sync", and
 "asynchronous" to "async" for values for
 metadata-prefetch-on-mount everywhere.
Changes in all internal state variables/ constants etc. needed to fix the mount-flags log.
This creates a mini-race condition between the
debug log for successful and that of starting
the recursive listing of mountPoint when
metadata-prefetch-on-mount has been set to "async",
but it is immaterial to the end user.
@gargnitingoogle gargnitingoogle force-pushed the gargnitin/listing-during-mount/production/v1 branch from 0913570 to a54ef3e Compare May 23, 2024 10:05
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
Copy link
Collaborator

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

Added a couple of comments.

Removed redundant call to callRecursiveList
from the parent process of the GCSFuse daemon.
@gargnitingoogle
Copy link
Collaborator Author

Added a couple of comments.

@raj-prince I've addressed your comments. PTAL.

main.go Show resolved Hide resolved
Reuse isDynamicMount utility in mount.go
@gargnitingoogle gargnitingoogle merged commit d086815 into master May 24, 2024
14 of 16 checks passed
@gargnitingoogle gargnitingoogle deleted the gargnitin/listing-during-mount/production/v1 branch May 24, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-integration-tests Run only integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants