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

Fixed false error for implicit dir in rename and remove directory flow #1186

Merged
merged 4 commits into from Jun 19, 2023

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Jun 15, 2023

Description

While running rm -r command on an implicit directory, we were getting the following error:
rm: cannot remove 'implicitSubDirectory/': Input/output error

While running mv command on an implicit directory, we were getting the following error:
mv: cannot stat 'implicitDirectory/': No such file or directory

Cause: for implicit directory, the 0 byte directory backing object did not exist on gcsfuse but we were still making a deleteObject request in DeleteChildDir() method.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Manually tested recursive delete and rename flows.
    Details:

    • Recursive delete:
      Mount Command: go run ./ --implicit-dirs --debug_fuse --debug_gcs --foreground ashmeenbkt /tmp/testnew/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── explicitDir
      ├── implicitDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── implicitDirectoryNested
      │       └── fileInNestedImplDir
      └── implicitDirectory2
          └── file1InImplicitDir2
      
      5 directories, 4 files
      ashmeen@ashmeen:/tmp/testnew$ rm -r explicitDir/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── implicitDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── implicitDirectoryNested
      │       └── fileInNestedImplDir
      └── implicitDirectory2
          └── file1InImplicitDir2
      
      4 directories, 4 files
      ashmeen@ashmeen:/tmp/testnew$ rm -r implicitDirectory1/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      └── implicitDirectory2
          └── file1InImplicitDir2
      
      2 directories, 1 file
      ashmeen@ashmeen:/tmp/testnew$ rm -r implicitDirectory2/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      
      0 directories, 0 files
      ashmeen@ashmeen:/tmp/test$ tree
      .
      └── explicitDir
          └── implicitDirectory
              └── fileInImplicitDir1
      
      3 directories, 1 file
      ashmeen@ashmeen:/tmp/test$ rm -r explicitDir/
      ashmeen@ashmeen:/tmp/test$ tree
      .
      
      0 directories, 0 files
    
    
    • rename:
      Mount Command: go run ./ --implicit-dirs --debug_fuse --debug_gcs --foreground --rename-dir-limit=3 ashmeenbkt /tmp/testnew/
     ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── explicitDir
      │   └── f1.txt
      ├── implicitDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── implicitDirectoryNested
      │       └── fileInNestedImplDir
      └── implicitDirectory2
          └── file1InImplicitDir2
      
      5 directories, 5 files
      ashmeen@ashmeen:/tmp/testnew$ mv explicitDir/ newExplitDir/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── implicitDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── implicitDirectoryNested
      │       └── fileInNestedImplDir
      ├── implicitDirectory2
      │   └── file1InImplicitDir2
      └── newExplitDir
          └── f1.txt
      
      5 directories, 5 files
      ashmeen@ashmeen:/tmp/testnew$ mv implicitDirectory1/ newDirectory1/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── implicitDirectory2
      │   └── file1InImplicitDir2
      ├── newDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── implicitDirectoryNested
      │       └── fileInNestedImplDir
      └── newExplitDir
          └── f1.txt
      
      5 directories, 5 files
      ashmeen@ashmeen:/tmp/testnew$ mv implicitDirectory2/ newDirectory1/newSubDirectory2/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── newDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   ├── implicitDirectoryNested
      │   │   └── fileInNestedImplDir
      │   └── newSubDirectory2
      │       └── file1InImplicitDir2
      └── newExplitDir
          └── f1.txt
      
      5 directories, 5 files
      ashmeen@ashmeen:/tmp/testnew$ mv newDirectory1/implicitDirectoryNested/ renamedNestedDirectory/
      ashmeen@ashmeen:/tmp/testnew$ tree
      .
      ├── newDirectory1
      │   ├── file1InImplicitDir1
      │   ├── file2InImplicitDir1
      │   └── newSubDirectory2
      │       └── file1InImplicitDir2
      ├── newExplitDir
      │   └── f1.txt
      └── renamedNestedDirectory
          └── fileInNestedImplDir
      
      5 directories, 5 files
    
      ashmeen@ashmeen:/tmp/test$ tree
      .
      └── explicitDir
          └── implicitDirectory
              └── fileInImplicitDir1
      
      3 directories, 1 file
      ashmeen@ashmeen:/tmp/test$ mv explicitDir/ newDir/
      ashmeen@ashmeen:/tmp/test$ tree
      .
      └── newDir
          └── implicitDirectory
              └── fileInImplicitDir1
    
    
    
  2. Unit tests - Added unit tests.

  3. Integration tests - Ran all existing integration tests.

Copy link
Collaborator

@sethiay sethiay left a comment

Choose a reason for hiding this comment

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

Minor comment.
Please add details of manual testing in the description - mount command, test directory structure, command run and output.

internal/fs/fs.go Show resolved Hide resolved
internal/fs/inode/dir.go Show resolved Hide resolved
internal/fs/fs.go Show resolved Hide resolved
internal/fs/fs.go Show resolved Hide resolved
Copy link
Collaborator

@Tulsishah Tulsishah left a comment

Choose a reason for hiding this comment

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

Please check all the flows. (List, Create, Delete, Compose, Stat, Update, Copy) to confirm nothing breaks.

Copy link
Collaborator

@vadlakondaswetha vadlakondaswetha left a comment

Choose a reason for hiding this comment

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

also add manual testing details around implicitDir inside explicitDir and what happens when you try to delete/rename explicitDir.

Copy link
Collaborator

@sethiay sethiay left a comment

Choose a reason for hiding this comment

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

Couple of things:

  1. Manually test and add in description about delete and rename flows where you create explicitDir inside implicitDir. Also test rmdir
  2. Add execute-perf-tests to this PR which will run e2e tests and perf tests(which will indirectly cover read and write flows)

@ashmeenkaur ashmeenkaur added the execute-perf-test Execute performance test in PR label Jun 19, 2023
@ashmeenkaur ashmeenkaur removed the execute-perf-test Execute performance test in PR label Jun 19, 2023
Copy link
Collaborator

@sethiay sethiay left a comment

Choose a reason for hiding this comment

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

Please add unit tests for rename also.

internal/fs/implicit_dirs_with_cache_test.go Outdated Show resolved Hide resolved
@ashmeenkaur ashmeenkaur dismissed vadlakondaswetha’s stale review June 19, 2023 12:23

Added the requested changes in a separate file.

Copy link
Collaborator

@vadlakondaswetha vadlakondaswetha left a comment

Choose a reason for hiding this comment

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

minor formatting comments. LGTM

@ashmeenkaur ashmeenkaur merged commit f2017f2 into master Jun 19, 2023
4 checks passed
@ashmeenkaur ashmeenkaur deleted the impl-dir-err branch August 30, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants