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

[#6131] feat (gvfs-fuse): Add integration test framework of gvfs-fuse #6160

Merged
merged 29 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions .github/workflows/gvfs-fuse-build-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,17 @@ jobs:
run: |
dev/ci/check_commands.sh

- name: Build and test Gravitino
- name: Build and test Gvfs-fuse
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
run: |
./gradlew :clients:filesystem-fuse:build -PenableFuse=true

- name: Integration test
run: |
./gradlew bundles:aws-bundle:build -x :clients:client-python:build compileDistribution -x test -x web -PjdkVersion=${{ matrix.java-version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

it's hard to use, use make prepare-it-env ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not good to let make call gradle again.
Currently, the integration test module does not automatically build the package, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the current IT doesn't automatically build the package, but here you depends on bundles:aws-bundle:build, I wonder whether you could make it auto dependents. and There are separate actions in Gravitino IT, seems better make rust client keep consistent with it.

      - name: Package Gravitino
        if: ${{ inputs.test-mode == 'deploy' }}
        run: |
          ./gradlew compileDistribution -x test -PjdkVersion=${{ inputs.java-version }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

cd clients/filesystem-fuse
make test-s3
make test-fuse

- name: Free up disk space
run: |
dev/ci/util_free_space.sh
Expand All @@ -85,5 +92,7 @@ jobs:
with:
name: Gvfs-fuse integrate-test-reports-${{ matrix.java-version }}
path: |
clients/filesystem-fuse/build/test/log/*.log
clients/filesystem-fuse/target/debug/fuse.log
distribution/package/logs/gravitino-server.out
distribution/package/logs/gravitino-server.log

6 changes: 6 additions & 0 deletions clients/filesystem-fuse/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ doc-test:
unit-test: doc-test
cargo test --no-fail-fast --lib --all-features --workspace

test-fuse:
@bash ./tests/bin/s3_fileset_it.sh test

test-s3:
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
@bash ./tests/bin/s3_test.sh test

test: doc-test
cargo test --no-fail-fast --all-targets --all-features --workspace

Expand Down
14 changes: 11 additions & 3 deletions clients/filesystem-fuse/src/default_raw_filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,13 +334,21 @@ impl<T: PathFileSystem> RawFileSystem for DefaultRawFileSystem<T> {
file.flush().await
}

async fn close_file(&self, _file_id: u64, fh: u64) -> Result<()> {
async fn close_file(&self, file_id: u64, fh: u64) -> Result<()> {
let file_entry = self.get_file_entry(file_id).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

why adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check if the file has been deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no file entry, is it expected not closing the open file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, closing deleted file may cause bugs.
This fix is not good. Add the todo comments to handle that case.


let opened_file = self
.opened_file_manager
.remove(fh)
.ok_or(Errno::from(libc::EBADF))?;
let mut file = opened_file.lock().await;
file.close().await

// todo: need to handle racing condition
if file_entry.is_ok() {
let mut file = opened_file.lock().await;
file.close().await
} else {
Ok(())
}
}

async fn read(&self, file_id: u64, fh: u64, offset: u64, size: u32) -> Result<Bytes> {
Expand Down
24 changes: 24 additions & 0 deletions clients/filesystem-fuse/src/gravitino_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ pub(crate) struct Fileset {
properties: HashMap<String, String>,
}

impl Fileset {
pub fn new(name: &str, storage_location: &str) -> Fileset {
Self {
name: name.to_string(),
fileset_type: "managed".to_string(),
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
comment: "".to_string(),
storage_location: storage_location.to_string(),
properties: HashMap::default(),
}
}
}

#[derive(Debug, Deserialize)]
struct FilesetResponse {
code: u32,
Expand All @@ -58,6 +70,18 @@ pub(crate) struct Catalog {
pub(crate) properties: HashMap<String, String>,
}

impl Catalog {
pub fn new(name: &str, properties: HashMap<String, String>) -> Catalog {
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
Self {
name: name.to_string(),
catalog_type: "fileset".to_string(),
provider: "s3".to_string(),
comment: "".to_string(),
properties: properties,
}
}
}

#[derive(Debug, Deserialize)]
struct CatalogResponse {
code: u32,
Expand Down
79 changes: 76 additions & 3 deletions clients/filesystem-fuse/src/gravitino_fileset_filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,26 @@ impl PathFileSystem for GravitinoFilesetFileSystem {

#[cfg(test)]
mod tests {
use crate::config::GravitinoConfig;
use crate::config::{AppConfig, GravitinoConfig};
use crate::default_raw_filesystem::DefaultRawFileSystem;
use crate::filesystem::tests::{TestPathFileSystem, TestRawFileSystem};
use crate::filesystem::{FileSystemContext, PathFileSystem, RawFileSystem};
use crate::gravitino_client::{Catalog, Fileset, GravitinoClient};
use crate::gravitino_fileset_filesystem::GravitinoFilesetFileSystem;
use crate::gvfs_creator::create_fs_with_fileset;
use crate::memory_filesystem::MemoryFileSystem;
use crate::s3_filesystem::extract_s3_config;
use crate::s3_filesystem::tests::{cleanup_s3_fs, s3_test_config};
use crate::test_enable_with;
use crate::RUN_TEST_WITH_S3;
use std::collections::HashMap;
use std::path::Path;

#[tokio::test]
async fn test_map_fileset_path_to_raw_path() {
let fs = GravitinoFilesetFileSystem {
physical_fs: Box::new(MemoryFileSystem::new().await),
client: super::GravitinoClient::new(&GravitinoConfig::default()),
client: GravitinoClient::new(&GravitinoConfig::default()),
location: "/c1/fileset1".into(),
};
let path = fs.gvfs_path_to_raw_path(Path::new("/a"));
Expand All @@ -162,7 +172,7 @@ mod tests {
async fn test_map_raw_path_to_fileset_path() {
let fs = GravitinoFilesetFileSystem {
physical_fs: Box::new(MemoryFileSystem::new().await),
client: super::GravitinoClient::new(&GravitinoConfig::default()),
client: GravitinoClient::new(&GravitinoConfig::default()),
location: "/c1/fileset1".into(),
};
let path = fs
Expand All @@ -172,4 +182,67 @@ mod tests {
let path = fs.raw_path_to_gvfs_path(Path::new("/c1/fileset1")).unwrap();
assert_eq!(path, Path::new("/"));
}

async fn create_fileset_fs(path: &Path, config: &AppConfig) -> GravitinoFilesetFileSystem {
let opendal_config = extract_s3_config(config);

cleanup_s3_fs(path, &opendal_config).await;

let bucket = opendal_config.get("bucket").expect("Bucket must exist");
let endpoint = opendal_config.get("endpoint").expect("Endpoint must exist");

let catalog = Catalog::new(
"c1",
vec![
("location".to_string(), format!("s3a://{}", bucket)),
("s3-endpoint".to_string(), endpoint.to_string()),
]
.into_iter()
.collect::<HashMap<String, String>>(),
);
let file_set_location = format!("s3a://{}{}", bucket, path.to_string_lossy());
let file_set = Fileset::new("fileset1", &file_set_location);

let fs_context = FileSystemContext::default();
let inner_fs = create_fs_with_fileset(&catalog, &file_set, config, &fs_context)
.await
.unwrap();
GravitinoFilesetFileSystem::new(
inner_fs,
path,
GravitinoClient::new(&config.gravitino),
config,
&fs_context,
)
.await
}

#[tokio::test]
async fn s3_ut_test_fileset_file_system() {
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
test_enable_with!(RUN_TEST_WITH_S3);

let config = s3_test_config();
let cwd = Path::new("/gvfs_test3");
let fs = create_fileset_fs(cwd, &config).await;
let _ = fs.init().await;
let mut tester = TestPathFileSystem::new(Path::new("/"), fs);
tester.test_path_file_system().await;
}

#[tokio::test]
async fn s3_ut_test_fileset_with_raw_file_system() {
test_enable_with!(RUN_TEST_WITH_S3);

let config = s3_test_config();
let cwd = Path::new("/gvfs_test4");
let fileset_fs = create_fileset_fs(cwd, &config).await;
let raw_fs = DefaultRawFileSystem::new(
fileset_fs,
&AppConfig::default(),
&FileSystemContext::default(),
);
let _ = raw_fs.init().await;
let mut tester = TestRawFileSystem::new(Path::new("/"), raw_fs);
tester.test_raw_file_system().await;
}
}
10 changes: 5 additions & 5 deletions clients/filesystem-fuse/src/gvfs_creator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,15 @@ pub async fn create_gvfs_filesystem(
.get_fileset(&catalog_name, &schema_name, &fileset_name)
.await?;

let inner_fs = create_fs_with_fileset(&catalog, &fileset, config, fs_context)?;
let inner_fs = create_fs_with_fileset(&catalog, &fileset, config, fs_context).await?;

let target_path = extract_root_path(fileset.storage_location.as_str())?;
let fs =
GravitinoFilesetFileSystem::new(inner_fs, &target_path, client, config, fs_context).await;
Ok(CreateFileSystemResult::Gvfs(fs))
}

fn create_fs_with_fileset(
pub(crate) async fn create_fs_with_fileset(
catalog: &Catalog,
fileset: &Fileset,
config: &AppConfig,
Expand All @@ -104,9 +104,9 @@ fn create_fs_with_fileset(
let schema = extract_filesystem_scheme(&fileset.storage_location)?;

match schema {
FileSystemSchema::S3 => Ok(Box::new(S3FileSystem::new(
catalog, fileset, config, fs_context,
)?)),
FileSystemSchema::S3 => Ok(Box::new(
S3FileSystem::new(catalog, fileset, config, fs_context).await?,
)),
}
}

Expand Down
13 changes: 13 additions & 0 deletions clients/filesystem-fuse/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@ mod opened_file_manager;
mod s3_filesystem;
mod utils;

#[macro_export]
macro_rules! test_enable_with {
($env_var:expr) => {
if std::env::var($env_var).is_err() {
println!("Test skipped because {} is not set", $env_var);
return;
}
};
}

pub const RUN_TEST_WITH_S3: &str = "RUN_TEST_WITH_S3";
pub const RUN_TEST_WITH_FUSE: &str = "RUN_TEST_WITH_FUSE";

pub async fn gvfs_mount(mount_to: &str, mount_from: &str, config: &AppConfig) -> GvfsResult<()> {
gvfs_fuse::mount(mount_to, mount_from, config).await
}
Expand Down
47 changes: 40 additions & 7 deletions clients/filesystem-fuse/src/open_dal_filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,22 +261,29 @@ fn opendal_filemode_to_filetype(mode: EntryMode) -> FileType {
mod test {
use crate::config::AppConfig;
use crate::s3_filesystem::extract_s3_config;
use crate::s3_filesystem::tests::s3_test_config;
use crate::test_enable_with;
use crate::RUN_TEST_WITH_S3;
use opendal::layers::LoggingLayer;
use opendal::{services, Builder, Operator};

#[tokio::test]
async fn test_s3_stat() {
let config = AppConfig::from_file(Some("tests/conf/gvfs_fuse_s3.toml")).unwrap();
let opendal_config = extract_s3_config(&config);

fn create_opendal(config: &AppConfig) -> Operator {
let opendal_config = extract_s3_config(config);
let builder = services::S3::from_map(opendal_config);

// Init an operator
let op = Operator::new(builder)
Operator::new(builder)
.expect("opendal create failed")
.layer(LoggingLayer::default())
.finish();
.finish()
}

#[tokio::test]
async fn s3_ut_test_s3_stat() {
FANNG1 marked this conversation as resolved.
Show resolved Hide resolved
test_enable_with!(RUN_TEST_WITH_S3);
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems hacks, could you refer other rust projects for more general way to control the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can’t find a good way to mark the test tags and run them.

Copy link
Contributor

@FANNG1 FANNG1 Jan 13, 2025

Choose a reason for hiding this comment

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

try using this?
#[cfg(feature = "ENABLE_S3_TEST")], after using this macro, seems we could remove s3_ut prefix too.

Copy link
Contributor Author

@diqiu50 diqiu50 Jan 13, 2025

Choose a reason for hiding this comment

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

This is generally used to control whether a module participates in compilation, and I haven't seen any projects using it to control tests. People familiar with Rust might find this confusing, right?

It also cannot filter out tests that do not have feature = "ENABLE_S3_TEST" enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There have a lib can mark tags for the testers. but it can not work with #[tokio::test]
https://docs.rs/tagrs/latest/tagrs/

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@diqiu50 diqiu50 Jan 14, 2025

Choose a reason for hiding this comment

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

The storage-s3 feature is used to enable that functionality for all code, not just for testing.


let config = s3_test_config();
let op = create_opendal(&config);
let path = "/";
let list = op.list(path).await;
if let Ok(l) = list {
Expand All @@ -294,4 +301,30 @@ mod test {
println!("stat error: {:?}", meta.err());
}
}

#[tokio::test]
async fn s3_ut_test_s3_delete() {
test_enable_with!(RUN_TEST_WITH_S3);
let config = s3_test_config();

let op = create_opendal(&config);
let path = "/s1/fileset1/gvfs_test/test_dir/test_file";

let meta = op.stat(path).await;
if let Ok(m) = meta {
println!("stat result: {:?}", m);
} else {
println!("stat error: {:?}", meta.err());
}

let result = op.remove(vec![path.to_string()]).await;
match result {
Ok(_) => {
println!("Delete successful (or no-op).");
}
Err(e) => {
println!("Delete failed: {:?}", e);
}
}
}
}
Loading
Loading