feat: Add read-ahead configuration and verification support for integration tests#4777
feat: Add read-ahead configuration and verification support for integration tests#4777kislaykishore wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
remind-reviewers
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the GCSFuse integration testing framework by adding support for configuring and verifying the read-ahead size of mount points. These changes enable more granular control over file system performance testing by allowing developers to specify and validate read-ahead settings directly through the test suite's CLI arguments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to configure and verify the read-ahead size (in KB) on FUSE mount points during integration testing. It adds a new --read-ahead-kb flag to the E2E test runner, implements the configuration and verification logic by writing to and reading from /sys/class/bdi, and includes corresponding unit and integration tests. The review feedback suggests improving the robustness of the configuration command by using non-interactive sudo -n to prevent potential hangs, and recommends replacing deprecated syscall usages with the golang.org/x/sys/unix package for better consistency.
|
|
||
| // If direct write fails (e.g., due to permission error on /sys/class/bdi/ when run as non-root), | ||
| // fallback to sudo tee. | ||
| cmd := exec.Command("sudo", "tee", bdiPath) |
There was a problem hiding this comment.
Use sudo -n (non-interactive) to prevent the command from prompting for a password and hanging indefinitely if passwordless sudo is not configured or misconfigured in the test environment.
| cmd := exec.Command("sudo", "tee", bdiPath) | |
| cmd := exec.Command("sudo", "-n", "tee", bdiPath) |
| var stat syscall.Stat_t | ||
| if err := syscall.Stat(mountDir, &stat); err != nil { |
There was a problem hiding this comment.
Use unix.Stat_t and unix.Stat from golang.org/x/sys/unix instead of the deprecated syscall package. This maintains consistency with the rest of the file which already imports and uses unix.
| var stat syscall.Stat_t | |
| if err := syscall.Stat(mountDir, &stat); err != nil { | |
| var stat unix.Stat_t | |
| if err := unix.Stat(mountDir, &stat); err != nil { |
| var stat syscall.Stat_t | ||
| if err := syscall.Stat(mountDir, &stat); err != nil { |
There was a problem hiding this comment.
Use unix.Stat_t and unix.Stat from golang.org/x/sys/unix instead of the deprecated syscall package to maintain consistency.
| var stat syscall.Stat_t | |
| if err := syscall.Stat(mountDir, &stat); err != nil { | |
| var stat unix.Stat_t | |
| if err := unix.Stat(mountDir, &stat); err != nil { |
| var stat syscall.Stat_t | ||
| err = syscall.Stat(tempMountDir, &stat) |
There was a problem hiding this comment.
Use unix.Stat_t and unix.Stat from golang.org/x/sys/unix instead of the deprecated syscall package to maintain consistency.
| var stat syscall.Stat_t | |
| err = syscall.Stat(tempMountDir, &stat) | |
| var stat unix.Stat_t | |
| err = unix.Stat(tempMountDir, &stat) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new command-line option --read-ahead-kb to configure and verify the read-ahead size on FUSE mount points during integration tests. This includes updates to the test runner script, setup flags, mounting utilities (with support for writing to sysfs BDI settings directly or via sudo tee), and a new verification test. The review comments suggest improving the robustness of the mount directory resolution by prioritizing setup.MntDir() and explicitly skipping the verification test using t.Skip when the option is not configured.
| mountDir := getMountDir(flags) | ||
| if mountDir != "" { | ||
| if err := ConfigureReadAhead(mountDir, readAheadKB); err != nil { | ||
| return fmt.Errorf("failed to configure read-ahead: %w", err) | ||
| } | ||
| } else { | ||
| log.Printf("Warning: read-ahead-kb specified but mount directory could not be resolved from flags: %v", flags) | ||
| } |
There was a problem hiding this comment.
To make the mount directory resolution more robust and less dependent on parsing command-line flags (which can be fragile if directory-valued flags are introduced in the future), prefer using setup.MntDir() as the primary source, falling back to getMountDir(flags) only if it is not set.
mountDir := setup.MntDir()
if mountDir == "" {
mountDir = getMountDir(flags)
}
if mountDir != "" {
if err := ConfigureReadAhead(mountDir, readAheadKB); err != nil {
return fmt.Errorf("failed to configure read-ahead: %w", err)
}
} else {
log.Printf("Warning: read-ahead-kb specified but mount directory could not be resolved: %v", flags)
}References
- Instead of extracting configuration values from flags at runtime, use a global constant in a shared setup file for values like block size.
| expectedKB := setup.ReadAheadKB() | ||
| if expectedKB > 0 { | ||
| err := mounting.VerifyReadAhead(setup.MntDir(), expectedKB) | ||
| if err != nil { | ||
| t.Errorf("read-ahead verification failed: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of silently passing when read-ahead-kb is not configured, use t.Skip to explicitly skip the test. This improves test reporting and visibility in CI/CD pipelines.
expectedKB := setup.ReadAheadKB()
if expectedKB <= 0 {
t.Skip("Skipping read-ahead verification as read-ahead-kb is not configured.")
}
err := mounting.VerifyReadAhead(setup.MntDir(), expectedKB)
if err != nil {
t.Errorf("read-ahead verification failed: %v", err)
}
|
/gemini review |
…fication test when not configured
|
/gemini review |
kislaykishore
changed the title
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring and verifying the FUSE mount read-ahead size (--read-ahead-kb) during integration tests. It introduces command-line flag propagation in the test runner, setup configuration parsing, and utility functions to write to and read from the Linux sysfs BDI interface. Feedback on this PR highlights critical compilation issues on macOS (Darwin) due to the direct use of Linux-specific golang.org/x/sys/unix APIs without build tags. To resolve this, the Linux-specific mounting utilities and their mock tests should be moved to separate files (e.g., using //go:build linux) with appropriate stubs for non-Linux platforms.
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring and verifying the read-ahead size (in KB) on FUSE mount points during integration testing. It adds a --read-ahead-kb flag to the E2E test runner script, propagates it to the test execution command, and implements the configuration on Linux by writing to the corresponding sysfs BDI path. A verification test and unit tests are also added. The review feedback suggests prioritizing the resolution of the mount directory from the command flags first, falling back to the global default mount directory only if it cannot be resolved.
| mountDir := setup.MntDir() | ||
| if mountDir == "" { | ||
| mountDir = getMountDir(flags) | ||
| } |
There was a problem hiding this comment.
When mounting GCSFuse with custom flags or custom mount directories, setup.MntDir() (which returns the global default mount directory) might not match the actual mount directory being used for this specific mount command. We should prioritize resolving the mount directory from the command flags first, and only fall back to setup.MntDir() if it cannot be resolved.
| mountDir := setup.MntDir() | |
| if mountDir == "" { | |
| mountDir = getMountDir(flags) | |
| } | |
| mountDir := getMountDir(flags) | |
| if mountDir == "" { | |
| mountDir = setup.MntDir() | |
| } |
1 similar comment
1 participant
This pull request adds support for configuring and verifying read-ahead size in GCSFuse integration tests.
Changes
--read-ahead-kbflag in setup.go to specify read-ahead sizes.ConfigureReadAheadandVerifyReadAheadin mounting.go to resolve BDI devices programmatically and apply configurations (with passwordlesssudo teefallback).TestVerifyReadAheadKB) to verify the mount point's read-ahead setting matches the flag value during E2E.--read-ahead-kbparameter.