Skip to content

object/cifs: support cifs/smb object #6052

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

object/cifs: support cifs/smb object #6052

wants to merge 8 commits into from

Conversation

zxh326
Copy link
Member

@zxh326 zxh326 commented Apr 23, 2025 •
edited
Loading

This PR introduces support for testing CIFS(AKA smb/samba)

fix: #5526

Test:

objbench

➜  ./juicefs objbench --storage cifs cifs://127.0.0.1:445/Data --access-key samba --secret-key secret
Start Functional Testing ...
+----------+---------------------+--------------------------------------------------+
| CATEGORY |         TEST        |                      RESULT                      |
+----------+---------------------+--------------------------------------------------+
|    basic |     create a bucket |                                             pass |
|    basic |       put an object |                                             pass |
|    basic |       get an object |                                             pass |
|    basic |       get non-exist |                                             pass |
|    basic |  get partial object |                                             pass |
|    basic |      head an object |                                             pass |
|    basic |    delete an object |                                             pass |
|    basic |    delete non-exist |                                             pass |
|    basic |        list objects |                                             pass |
|     sync |         special key | put encode file failed: open __juicefs_benchm... |
|     sync |    put a big object |                                             pass |
|     sync | put an empty object |                                             pass |
|     sync |    multipart upload |                                      not support |
|     sync |  change owner/group |                                      not support |
|     sync |   change permission |                                      not support |
|     sync |        change mtime |                                      not support |
+----------+---------------------+--------------------------------------------------+

Note:

  • smb does not fully support chmod, chown, change atime, so the file system interface has not been implemented
  • special key not support

Dependency Updates:

  • Added github.com/hirochachacha/go-smb2 for CIFS support

@zxh326
pkg/object: support cifs/smb
0de69d1 
Signed-off-by: Xuhui zhang <[email protected]>
@zxh326 zxh326 requested a review from Copilot April 23, 2025 08:42
Copilot
Copilot AI reviewed Apr 23, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for CIFS (SMB/Samba) storage to the object module, along with tests for its functionality.

  • Added a new test function (TestCifs) in object_storage_test.go to verify CIFS integration.
  • Updated the unit test workflow to include CIFS environment variables.

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

File Description
pkg/object/object_storage_test.go Added TestCifs to run CIFS storage tests.
.github/workflows/unittests.yml Added CIFS-related environment variables for tests.
Files not reviewed (2)
  • .github/scripts/prepare_db.sh: Language not supported
  • go.mod: Language not supported
Comments suppressed due to low confidence (2)

pkg/object/object_storage_test.go:1082

  • Consider using t.Skip with a descriptive message (e.g., t.Skip("Skipping CIFS test due to missing CIFS_ADDR")) instead of printing a message followed by t.SkipNow for improved clarity and standard testing practice.
fmt.Println("skip CIFS test")

.github/workflows/unittests.yml:52

  • The CIFS_ADDR value in the workflow does not match the address format specified in the PR description (e.g., cifs://127.0.0.1:445/Data). Verify that the configuration is consistent with the intended CIFS testing environment.
CIFS_ADDR: localhost:4445/Data

@zhijian-pro
Copy link
Contributor

add testFileSystem test

juicefs/pkg/object/filesystem_test.go

Line 85 in 1e66b4a

func testFileSystem(t *testing.T, s ObjectStorage) {

@zhijian-pro
Copy link
Contributor

There are still two interfaces to be implemented.

juicefs/pkg/object/object_storage.go

Line 36 in 0de69d1

type MtimeChanger interface {

juicefs/pkg/object/object_storage.go

Line 40 in 0de69d1

type SupportSymlink interface {

@zxh326
fix ci
b1aeb2f 
Signed-off-by: Xuhui zhang <[email protected]>
davies
davies reviewed Apr 23, 2025
View reviewed changes
davies
davies reviewed Apr 23, 2025
View reviewed changes
@zxh326
review
533d935 
Signed-off-by: Xuhui zhang <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Apr 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 1.26183% with 313 lines in your changes missing coverage. Please review.

Project coverage is 45.34%. Comparing base (f0aa148) to head (9ad8de2).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
pkg/object/cifs.go 1.26% 313 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6052      +/-   ##
==========================================
- Coverage   50.70%   45.34%   -5.36%     
==========================================
  Files         165      165              
  Lines       49119    49193      +74     
==========================================
- Hits        24908    22309    -2599     
- Misses      21390    24184    +2794     
+ Partials     2821     2700     -121

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zxh326
Copy link
Member Author

zxh326 commented Apr 25, 2025

add testFileSystem test

juicefs/pkg/object/filesystem_test.go

Line 85 in 1e66b4a

func testFileSystem(t *testing.T, s ObjectStorage) {

smb does not fully support chmod, chown, change atime, so the file system interface has not been implemented

@zxh326 zxh326 marked this pull request as ready for review April 25, 2025 03:13
@zxh326 zxh326 requested a review from zhijian-pro April 25, 2025 03:14
davies
davies reviewed Apr 26, 2025
View reviewed changes
@zxh326
fix: follow symlink
7798017 
Signed-off-by: Xuhui zhang <[email protected]>
@zhijian-pro zhijian-pro changed the title feat: support cifs/smb object object/cifs: support cifs/smb object Apr 28, 2025
zhijian-pro
zhijian-pro reviewed Apr 28, 2025
View reviewed changes
zhijian-pro
zhijian-pro reviewed Apr 28, 2025
View reviewed changes
@zxh326
review
5aa132e 
Signed-off-by: Xuhui zhang <[email protected]>
zhijian-pro
zhijian-pro reviewed Apr 28, 2025
View reviewed changes
zhijian-pro
zhijian-pro reviewed Apr 28, 2025
View reviewed changes
zhijian-pro
zhijian-pro reviewed Apr 28, 2025
View reviewed changes
zxh326 added 2 commits April 28, 2025 14:30
@zxh326
performance
9ad8de2 
Signed-off-by: Xuhui zhang <[email protected]>
@zxh326
fix
0dcc875 
Signed-off-by: Xuhui zhang <[email protected]>
@zhijian-pro zhijian-pro requested a review from davies May 8, 2025 06:41
@davies davies added this to the Release 1.4 milestone May 8, 2025
@davies davies removed their request for review May 8, 2025 06:55
@davies
Copy link
Contributor

davies commented May 8, 2025

smb will be mostly used by sync, but permission and mtime are not supported by the sdk, we should fork it and fix that.

defer this for 1.4

@zhijian-pro zhijian-pro marked this pull request as draft May 27, 2025 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davies davies davies left review comments

Copilot code review Copilot Copilot left review comments

@zhijian-pro zhijian-pro zhijian-pro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 1.4
Development

Successfully merging this pull request may close these issues.

support pkg/object/cifs.go
3 participants
@zxh326 @zhijian-pro @davies