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 22 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.
|
/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.
|
/gemini review |
1 similar comment
|
/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.
3 similar comments
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4777 +/- ##
=======================================
Coverage 83.66% 83.66%
=======================================
Files 168 168
Lines 20850 20850
=======================================
Hits 17444 17444
Misses 2755 2755
Partials 651 651
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…fication test when not configured
…ad and non-read-ahead configurations
kislaykishore
force-pushed
the
read-ahead-support
branch
from
270f0fc to
e08125d
Compare
…flags over global setup.MntDir()
… support cross-platform compilation
|
/Gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite test execution via a new Makefile target and script, adding support for configuring and verifying the FUSE read-ahead size (--read-ahead-kb) on Linux mount points. The review feedback highlights two critical issues in tools/integration_tests/util/setup/setup.go where resetting the global mount directory (SetMntDir("")) outside of conditional blocks in UnmountGCSFuse and UnmountGCSFuseWithConfig will break subsequent test cases when running on pre-mounted directories (such as GKE). Moving these resets inside the conditional blocks is recommended.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite and adds support for configuring and verifying the FUSE read-ahead size (read_ahead_kb) on Linux mount points. It includes a new Makefile target, a CD script (npi_conformance.sh) to run the suite with and without read-ahead, updates to the E2E test runner to accept a --read-ahead-kb flag, and corresponding implementation, verification tests, and unit tests. There are no review comments, so I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite via a new script and Makefile target, adding support for configuring and verifying read-ahead sizes on FUSE mount points during integration tests. The feedback suggests prioritizing setup.MntDir() over the heuristic-based getMountDir(flags) to avoid incorrectly identifying other directory flags as the mount directory.
| mountDir := getMountDir(flags) | ||
| if mountDir == "" { | ||
| mountDir = setup.MntDir() | ||
| } |
There was a problem hiding this comment.
The getMountDir function is heuristic-based and can incorrectly identify directory arguments of other flags (such as --temp-dir) as the mount directory if they are placed after the mount directory in the flags slice. Since setup.MntDir() is the source of truth for the mount directory in integration tests, it is safer to prefer it when it is non-empty, falling back to getMountDir only if necessary.
| mountDir := getMountDir(flags) | |
| if mountDir == "" { | |
| mountDir = setup.MntDir() | |
| } | |
| mountDir := setup.MntDir() | |
| if mountDir == "" { | |
| mountDir = getMountDir(flags) | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite runner script and adds support for configuring and verifying the FUSE mount read-ahead size (--read-ahead-kb) in integration tests, including Linux-specific sysfs BDI configuration. Feedback suggests defining the default value of READ_AHEAD_KB at the top of the Makefile rather than using $(eval ...) inside the recipe for more idiomatic Make syntax.
| npi-conformance: fmt | ||
| $(eval READ_AHEAD_KB ?= 128) | ||
| @echo "Running NPI Conformance Suite via tools/cd_scripts/npi_conformance.sh..." |
There was a problem hiding this comment.
Instead of using $(eval ...) inside the recipe, define the default value for READ_AHEAD_KB at the top of the Makefile alongside other variable definitions (e.g., CSI_VERSION ?= main). This is more idiomatic and avoids mixing Make evaluation with shell execution.
npi-conformance: fmt
@echo "Running NPI Conformance Suite via tools/cd_scripts/npi_conformance.sh..."
…or idiomatic Make syntax
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite and adds support for configuring and verifying the read-ahead size (read_ahead_kb) on FUSE mount points. It includes a new conformance script, Makefile targets, updates to the integration test runner, and platform-specific implementations for Linux to write to and read from sysfs BDI. The review feedback suggests adding defensive checks to ensure the mount directory path is not empty before executing system calls in the Linux mounting utility.
| if readAheadKB <= 0 { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a defensive check to ensure mountDir is not empty before attempting to perform system operations like unix.Stat. If mountDir is empty, unix.Stat will fail with an unclear error, whereas an explicit check provides a clear and actionable error message.
| if mountDir == "" { return fmt.Errorf("mount directory cannot be empty") } |
| } | ||
|
|
||
| func VerifyReadAhead(mountDir string, expectedKB int) error { | ||
| var stat unix.Stat_t |
There was a problem hiding this comment.
Add a defensive check to ensure mountDir is not empty before attempting to perform system operations like unix.Stat.
| var stat unix.Stat_t | |
| if mountDir == "" { return fmt.Errorf("mount directory cannot be empty") }; var stat unix.Stat_t |
…x mounting utility
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring and verifying the FUSE read-ahead size (read_ahead_kb) during integration and conformance testing. It adds a new npi-conformance target to the Makefile, a conformance script (npi_conformance.sh), and updates the integration test runner to accept a --read-ahead-kb flag. Additionally, it implements platform-specific mounting utilities to configure and verify the read-ahead setting via sysfs on Linux, along with corresponding unit and integration tests. There are no review comments to address.
… mount point from active test directory
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite script and adds support for configuring and verifying the FUSE read-ahead size (--read-ahead-kb) during integration tests. Feedback on the changes highlights that the new npi_conformance.sh script lacks initialization, parsing, and forwarding for the --read-ahead-kb option, which would cause runtime failures. Additionally, it is recommended to dynamically resolve the mount directory from execution flags in mounting.go to ensure the read-ahead configuration is applied to the correct path.
| RELEASE_VERSION="" | ||
| RUN_TESTS_WITH_ZONAL_BUCKET=false | ||
| PROJECT_ID="" | ||
| BUCKET_LOCATION="" |
There was a problem hiding this comment.
The --read-ahead-kb parameter is documented in the usage description but is not initialized. Since set -u is enabled, referencing this variable later without initialization will cause the script to crash. Initialize READ_AHEAD_KB to -1 by default.
| BUCKET_LOCATION="" | |
| BUCKET_LOCATION="" | |
| READ_AHEAD_KB=-1 |
| } | ||
|
|
||
| # Define options for getopt | ||
| LONG=local-run,zonal,release-package-bucket:,release-version:,project-id:,bucket-location:,output-dir:,help |
There was a problem hiding this comment.
Add read-ahead-kb: to the LONG options of getopt so that the script can parse this option.
| LONG=local-run,zonal,release-package-bucket:,release-version:,project-id:,bucket-location:,output-dir:,help | |
| LONG=local-run,zonal,release-package-bucket:,release-version:,project-id:,bucket-location:,output-dir:,help,read-ahead-kb: |
| --help) | ||
| usage 0 | ||
| ;; | ||
| --) |
There was a problem hiding this comment.
Add a case in the getopt parsing loop to handle the --read-ahead-kb option.
| --) | |
| --read-ahead-kb) | |
| READ_AHEAD_KB="$2" | |
| shift 2 | |
| ;; | |
| --) |
| if [[ -n "$BUCKET_LOCATION" ]]; then | ||
| ARGS+=("--bucket-location=$BUCKET_LOCATION") | ||
| fi |
There was a problem hiding this comment.
Forward the --read-ahead-kb parameter to improved_run_e2e_tests.sh if it has been configured.
| if [[ -n "$BUCKET_LOCATION" ]]; then | |
| ARGS+=("--bucket-location=$BUCKET_LOCATION") | |
| fi | |
| if [[ -n "$BUCKET_LOCATION" ]]; then | |
| ARGS+=("--bucket-location=$BUCKET_LOCATION") | |
| fi | |
| if [[ "$READ_AHEAD_KB" -ne -1 ]]; then | |
| ARGS+=("--read-ahead-kb=$READ_AHEAD_KB") | |
| fi |
| readAheadKB := setup.ReadAheadKB() | ||
| if readAheadKB > 0 { | ||
| if err := ConfigureReadAhead(setup.RootMntDir(), readAheadKB); err != nil { | ||
| return fmt.Errorf("failed to configure read-ahead: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
In integration tests, GCSFuse might be mounted to a custom directory passed via flags rather than the global setup.RootMntDir(). To ensure the read-ahead configuration is applied to the correct mount point, dynamically resolve the mount directory from the last argument of flags if it exists and is a directory, falling back to setup.RootMntDir().
readAheadKB := setup.ReadAheadKB()
if readAheadKB > 0 {
mountDir := setup.RootMntDir()
if len(flags) > 0 {
lastArg := flags[len(flags)-1]
if fi, err := os.Stat(lastArg); err == nil && fi.IsDir() {
mountDir = lastArg
}
}
if err := ConfigureReadAhead(mountDir, readAheadKB); err != nil {
return fmt.Errorf("failed to configure read-ahead: %w", err)
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the NPI Conformance Suite script and Makefile target, along with support for configuring and verifying the FUSE mount's read-ahead size (--read-ahead-kb) on Linux. Feedback was provided regarding a mismatch in npi_conformance.sh where READ_AHEAD_KB is initialized as an empty string instead of its documented default of 128, which causes the verification test to be bypassed by default.
| RELEASE_PACKAGE_BUCKET="" | ||
| RELEASE_VERSION="" | ||
| RUN_TESTS_WITH_ZONAL_BUCKET=false | ||
| READ_AHEAD_KB="" |
There was a problem hiding this comment.
The help message for --read-ahead-kb states that the default value is 128, but the variable READ_AHEAD_KB is initialized to an empty string "". This results in the read-ahead configuration and verification being bypassed by default, skipping the TestVerifyReadAheadKB test.
To align the implementation with the documented default and ensure the verification test runs by default in the NPI conformance suite, initialize READ_AHEAD_KB to "128".
| READ_AHEAD_KB="" | |
| READ_AHEAD_KB="128" |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an NPI conformance test suite and adds support for configuring and verifying the read-ahead size (read-ahead-kb) on FUSE mount points. The feedback recommends adding a filesystem type check in ConfigureReadAhead and VerifyReadAhead to ensure the target directory is a FUSE mount before modifying sysfs BDI settings, preventing unintended side effects on the host's main disk. It also suggests updating the unit tests to mock this check.
| var sysfsBdiPrefix = "/sys/class/bdi" | ||
|
|
||
| func ConfigureReadAhead(mountDir string, readAheadKB int) error { | ||
| if readAheadKB <= 0 { | ||
| return nil | ||
| } | ||
| if mountDir == "" { | ||
| return fmt.Errorf("mount directory cannot be empty") | ||
| } | ||
|
|
||
| var stat unix.Stat_t | ||
| if err := unix.Stat(mountDir, &stat); err != nil { | ||
| return fmt.Errorf("failed to stat mount directory %s: %w", mountDir, err) | ||
| } | ||
|
|
||
| major := unix.Major(stat.Dev) | ||
| minor := unix.Minor(stat.Dev) | ||
| bdiPath := fmt.Sprintf("%s/%d:%d/read_ahead_kb", sysfsBdiPrefix, major, minor) | ||
|
|
||
| valStr := strconv.Itoa(readAheadKB) + "\n" | ||
|
|
||
| // Try direct write first. | ||
| if err := os.WriteFile(bdiPath, []byte(valStr), 0644); err == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // 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", "-n", "tee", bdiPath) | ||
| cmd.Stdin = strings.NewReader(valStr) | ||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("sudo tee failed to write to %s: %w (output: %s)", bdiPath, err, string(output)) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
ConfigureReadAhead does not verify that the target mountDir is actually a FUSE filesystem before attempting to read/write the BDI. If the mount failed or is not yet active, stat.Dev will refer to the host's underlying filesystem, and writing to /sys/class/bdi/<major>:<minor>/read_ahead_kb via sudo tee will modify the host's main disk read-ahead setting, which is a dangerous side effect. We should add a filesystem type check using unix.Statfs to ensure the filesystem magic number matches unix.FUSE_SUPER_MAGIC. Introducing a package-level lookupFSType function variable allows clean mocking in unit tests.
var sysfsBdiPrefix = "/sys/class/bdi"
var lookupFSType = func(dir string) (int64, error) {
var statfs unix.Statfs_t
if err := unix.Statfs(dir, &statfs); err != nil {
return 0, err
}
return int64(statfs.Type), nil
}
func ConfigureReadAhead(mountDir string, readAheadKB int) error {
if readAheadKB <= 0 {
return nil
}
if mountDir == "" {
return fmt.Errorf("mount directory cannot be empty")
}
fsType, err := lookupFSType(mountDir)
if err != nil {
return fmt.Errorf("failed to check filesystem type of %s: %w", mountDir, err)
}
if fsType != unix.FUSE_SUPER_MAGIC {
return fmt.Errorf("mount directory %s is not a FUSE mount (fs type: 0x%x)", mountDir, fsType)
}
var stat unix.Stat_t
if err := unix.Stat(mountDir, &stat); err != nil {
return fmt.Errorf("failed to stat mount directory %s: %w", mountDir, err)
}
major := unix.Major(stat.Dev)
minor := unix.Minor(stat.Dev)
bdiPath := fmt.Sprintf("%s/%d:%d/read_ahead_kb", sysfsBdiPrefix, major, minor)
valStr := strconv.Itoa(readAheadKB) + "\n"
// Try direct write first.
if err := os.WriteFile(bdiPath, []byte(valStr), 0644); err == nil {
return nil
}
// 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", "-n", "tee", bdiPath)
cmd.Stdin = strings.NewReader(valStr)
if output, err := cmd.CombinedOutput(); err != nil {
return fmt.Errorf("sudo tee failed to write to %s: %w (output: %s)", bdiPath, err, string(output))
}
return nil
}| func VerifyReadAhead(mountDir string, expectedKB int) error { | ||
| if mountDir == "" { | ||
| return fmt.Errorf("mount directory cannot be empty") | ||
| } | ||
|
|
||
| var stat unix.Stat_t | ||
| if err := unix.Stat(mountDir, &stat); err != nil { | ||
| return fmt.Errorf("failed to stat mount directory %s: %w", mountDir, err) | ||
| } | ||
|
|
||
| major := unix.Major(stat.Dev) | ||
| minor := unix.Minor(stat.Dev) | ||
| bdiPath := fmt.Sprintf("%s/%d:%d/read_ahead_kb", sysfsBdiPrefix, major, minor) | ||
|
|
||
| content, err := os.ReadFile(bdiPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read read_ahead_kb from %s: %w", bdiPath, err) | ||
| } | ||
|
|
||
| actualStr := strings.TrimSpace(string(content)) | ||
| actualKB, err := strconv.Atoi(actualStr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse read_ahead_kb value %q: %w", actualStr, err) | ||
| } | ||
|
|
||
| if actualKB != expectedKB { | ||
| return fmt.Errorf("read-ahead setting mismatch for %s: expected %d KB, got %d KB", mountDir, expectedKB, actualKB) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
For consistency and safety, verify that the target mountDir is a FUSE filesystem in VerifyReadAhead as well.
func VerifyReadAhead(mountDir string, expectedKB int) error {
if mountDir == "" {
return fmt.Errorf("mount directory cannot be empty")
}
fsType, err := lookupFSType(mountDir)
if err != nil {
return fmt.Errorf("failed to check filesystem type of %s: %w", mountDir, err)
}
if fsType != unix.FUSE_SUPER_MAGIC {
return fmt.Errorf("mount directory %s is not a FUSE mount (fs type: 0x%x)", mountDir, fsType)
}
var stat unix.Stat_t
if err := unix.Stat(mountDir, &stat); err != nil {
return fmt.Errorf("failed to stat mount directory %s: %w", mountDir, err)
}
major := unix.Major(stat.Dev)
minor := unix.Minor(stat.Dev)
bdiPath := fmt.Sprintf("%s/%d:%d/read_ahead_kb", sysfsBdiPrefix, major, minor)
content, err := os.ReadFile(bdiPath)
if err != nil {
return fmt.Errorf("failed to read read_ahead_kb from %s: %w", bdiPath, err)
}
actualStr := strings.TrimSpace(string(content))
actualKB, err := strconv.Atoi(actualStr)
if err != nil {
return fmt.Errorf("failed to parse read_ahead_kb value %q: %w", actualStr, err)
}
if actualKB != expectedKB {
return fmt.Errorf("read-ahead setting mismatch for %s: expected %d KB, got %d KB", mountDir, expectedKB, actualKB)
}
return nil
}| // Override package-level sysfsBdiPrefix | ||
| oldPrefix := sysfsBdiPrefix | ||
| sysfsBdiPrefix = tempBdiPrefix | ||
| defer func() { | ||
| sysfsBdiPrefix = oldPrefix | ||
| }() |
There was a problem hiding this comment.
Mock lookupFSType to return unix.FUSE_SUPER_MAGIC so the unit tests (which run on a standard host filesystem) do not fail due to the new safety check.
| // Override package-level sysfsBdiPrefix | |
| oldPrefix := sysfsBdiPrefix | |
| sysfsBdiPrefix = tempBdiPrefix | |
| defer func() { | |
| sysfsBdiPrefix = oldPrefix | |
| }() | |
| // Override package-level sysfsBdiPrefix and lookupFSType | |
| oldPrefix := sysfsBdiPrefix | |
| sysfsBdiPrefix = tempBdiPrefix | |
| oldLookup := lookupFSType | |
| lookupFSType = func(dir string) (int64, error) { | |
| return unix.FUSE_SUPER_MAGIC, nil | |
| } | |
| defer func() { | |
| sysfsBdiPrefix = oldPrefix | |
| lookupFSType = oldLookup | |
| }() |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring and verifying the read-ahead size (read-ahead-kb) on FUSE mount points during integration testing. It adds a new npi-conformance target to the Makefile, a helper script npi_conformance.sh to run the conformance suite, and implements the platform-specific read-ahead configuration and verification logic for Linux via /sys/class/bdi. Additionally, it includes unit and integration tests to verify this behavior. There are no review comments, so I have no feedback to provide.
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.