HDDS-7253. Fix exception when '/' in key name #3774

xichen01 · 2022-09-22T12:06:07Z

What changes were proposed in this pull request?

Fix exception when '/' in key name

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7253

How was this patch tested?

How to reproduce the bug

First put a key that containing '/'

[root@Linux /root/ozone]% bin/ozone sh key put s3v/testbucket/dir1/dir2/key1 ~/testfile
[root@Linux /root/ozone]% bin/ozone sh key ls s3v/testbucket/ | grep name
  "name" : "dir1/dir2/key1"

Execute command

ls command for keys containing '/'
before this commit

[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/
Found 1 items
drwxrwxrwx   - root root          0 2022-09-19 14:11 ofs://localhost/s3v/testbucket/dir1
[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/dir1
ls: `ofs://localhost/s3v/testbucket/dir1': No such file or directory

after this commit

[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/
Found 1 items
drwxrwxrwx   - root root          0 2022-09-19 14:37 ofs://localhost/s3v/testbucket/dir1
[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/dir1
Found 1 items
drwxrwxrwx   - root root          0 2022-09-19 14:37 ofs://localhost/s3v/testbucket/dir1/dir2

for mv, mkdir, count command for keys containing '/'

before this commit

[root@Linux /root/ozone]% bin/ozone fs -count ofs://localhost/s3v/testbucket/
count: dir1: No such file or directory!
[root@Linux /root/ozone]% bin/ozone fs -mv ofs://localhost/s3v/testbucket/dir1 ofs://localhost/s3v/testbucket/dir2
mv: `ofs://localhost/s3v/testbucket/dir1': No such file or directory
[root@Linux /root/ozone]% bin/ozone fs -mkdir ofs://localhost/s3v/testbucket/dir1/dir2/key2
mkdir: `ofs://localhost/s3v/testbucket/dir1/dir2': No such file or directory

after this commit

[root@Linux /root/ozone]% bin/ozone fs -count ofs://localhost/s3v/testbucket/
           3            1            1048576 ofs://localhost/s3v/testbucket
[root@Linux /root/ozone]% bin/ozone fs -mv ofs://localhost/s3v/testbucket/dir1 ofs://localhost/s3v/testbucket/newdir1
[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/
Found 1 items
drwxrwxrwx   - root root          0 2022-09-19 14:39 ofs://localhost/s3v/testbucket/newdir1
[root@Linux /root/ozone]% bin/ozone fs -mkdir ofs://localhost/s3v/testbucket/newdir1/dir2/dir3
[root@Linux /root/ozone]% bin/ozone fs -ls ofs://localhost/s3v/testbucket/newdir1/dir2/
Found 2 items
drwxrwxrwx   - root root          0 2022-09-19 14:43 ofs://localhost/s3v/testbucket/newdir1/dir2/dir3
-rw-rw-rw-   3 root root    1048576 2022-09-19 14:39 ofs://localhost/s3v/testbucket/newdir1/dir2/key1

sadanand48

@xichen01 This is not a bug but expected behaviour. By default , if you don't specify any bucket layout while creating, the bucket layout is LEGACY and the property ozone.om.enable.filesystem.paths is false by default. If the property is set to true , this issue wont occur i.e intermediate directories will be created. You could also create FSO buckets which would by default create intermediate dirs.

xichen01 · 2022-09-23T03:28:40Z

when '/' in key name the listSatus will create a fake directory return to the client. The client will get the fake directory info through getfileStatus, but currently getfileStatus has no logic for fake directory, this is inconsistent with listStatus behavior, So some exceptions occur.
This PR is to make listStatus and getFileStatus have the same behavior.

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Lines 1700 to 1711 in 6c1a5ee

    
           } else { 
        
             // if entry is a directory 
        
             if (!isKeyDeleted(entryInDb, keyTable)) { 
        
               if (!entryKeyName.equals(immediateChild)) { 
        
                 OmKeyInfo fakeDirEntry = createDirectoryKey( 
        
                     omKeyInfo.getVolumeName(), omKeyInfo.getBucketName(), 
        
                     immediateChild, omKeyInfo.getAcls()); 
        
                 cacheKeyMap.put(entryInDb, 
        
                     new OzoneFileStatus(fakeDirEntry, 
        
                         scmBlockSize, true)); 
        
               } else { 
        
                 // If entryKeyName matches dir name, we have the info

The problem I'm currently having is that I can't use the -du/-count command. Since some keys have '/' in their names.
For some already created and used buckets whose layout is LEGACY, some file system commands cannot be used due to the fake directory returned by listStatus. This should not be expected behavior, we should be able to handle this situation correctly

sadanand48

when '/' in key name the listSatus will create a fake directory return to the client. The client will get the fake directory info through getfileStatus, but currently getfileStatus has no logic for fake directory, this is inconsistent with listStatus behavior, So some exceptions occur.
This PR is to make listStatus and getFileStatus have the same behavior.

Thanks for explaining @xichen01. This would make the user see a filesystem consistent view of the keys created in legacy buckets (using fake dirs) . Overall the change looks good, just dropped some minor comments.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

kerneltime · 2022-09-26T15:54:55Z

cc @duongkame

xichen01 · 2022-10-10T17:01:26Z

@sadanand48 PTAL Thanks.

sadanand48

Thanks @xichen01 The patch looks good overall , just left few minor comments. PTAL

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java

duongkame

+1, LGTM.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

sadanand48

Thanks @xichen01 for updating the PR. +1 LGTM.

kaijchen · 2022-10-25T12:10:25Z

Thanks @xichen01 for the work, @duongkame and @sadanand48 for the review.

This reverts commit b9a47f6.

kaijchen · 2022-10-26T03:40:26Z

Sorry I have to revert this commit because it's making ITestOzoneContractMkdir flaky.
Please take a look and submit another PR for this fix. @xichen01

This reverts commit b9a47f6.

xichen01 · 2022-10-26T08:18:01Z

@kaijchen Are there some failed operations I can refer to them? So I can find out which tests are flaky.

kaijchen · 2022-10-26T08:20:36Z

@kaijchen Are there some failed operations I can refer to them? So I can find out which tests are flaky.

Here is one of the failing case, but not the only one. Please run ITestOzoneContractMkdir multiple times to find out more.
https://github.com/apache/ozone/actions/runs/3322837635/jobs/5492873757

* master: (718 commits) HDDS-7342. Move encryption-related code from MultipartCryptoKeyInputStream to OzoneCryptoInputStream (apache#3852) HDDS-7413. Fix logging while marking container state unhealthy (apache#3887) Revert "HDDS-7253. Fix exception when '/' in key name (apache#3774)" HDDS-7396. Force close non-RATIS containers in ReplicationManager (apache#3877) HDDS-7121. Support namespace summaries (du, dist & counts) for legacy FS buckets (apache#3746) HDDS-7258. Cleanup the allocated but uncommitted blocks (apache#3778) HDDS-7381. Cleanup of VolumeManagerImpl (apache#3873) HDDS-7253. Fix exception when '/' in key name (apache#3774) HDDS-7182. Add property to control RocksDB max open files (apache#3843) HDDS-7284. JVM crash for rocksdb for read/write after close (apache#3801) HDDS-7368. [Multi-Tenant] Add Volume Existence check in preExecute for OMTenantCreateRequest (apache#3869) HDDS-7403. README Security Improvement (apache#3879) HDDS-7199. Implement new mix workload Read/Write Freon command (apache#3872) HDDS-7248. Recon: Expand the container status page to show all unhealthy container states (apache#3837) HDDS-7141. Recon: Improve Disk Usage Page (apache#3789) HDDS-7369. Fix wrong order of command arguments in Nonrolling-Upgrade.md (apache#3866) HDDS-6210. EC: Add EC metrics (apache#3851) HDDS-7355. non-primordial scm fail to get signed cert from primordial SCM when converting an unsecure cluster to secure (apache#3859) HDDS-7356. Update SCM-HA.zh.md to match the English version (apache#3861) HDDS-6930. SCM,OM,RECON should not print ERROR and exit with code 1 on successful shutdown (apache#3848) ... Conflicts: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestLegacyReplicationManager.java

sadanand48 · 2022-11-28T09:03:56Z

@xichen01 I think it would be better to have this fix, could you please take a look at the failures.

kaijchen · 2022-11-28T11:55:46Z

@xichen01 I can help take a look if you need.

xichen01 · 2022-11-28T16:30:06Z

@sadanand48 @kaijchen Okay, I'll check this PR, I've looked at it briefly before, but I haven't identified possible reasons causing ITestOzoneContractMkdir flaky.

xichen01 · 2022-11-28T16:33:15Z

@xichen01 I can help take a look if you need.

yeah, thank you, I think it would be helpful, and if you have time, I will prioritize this issue as well

kaijchen · 2022-11-29T02:56:36Z

@xichen01 FYI, here is ITestOzoneContractMkdir repeated 100x with this PR merged.
https://github.com/kaijchen/ozone/actions/runs/3570532658

xichen01 · 2022-12-03T16:25:04Z

this is latest MR #4038
@kaijchen I think the issue of ITestOzoneContractMkdir should have been fixed, can you run it multiple times to confirm the fix, I have run it multiple times on my Linux and have not reproduced the issue again, thanks a lot.

sadanand48 reviewed Sep 22, 2022

View reviewed changes

sadanand48 reviewed Sep 23, 2022

View reviewed changes

xichen01 requested a review from sadanand48 September 25, 2022 07:52

xichen01 added 4 commits October 10, 2022 10:40

HDDS-7253. Fix exception when '/' in key name

d348fa9

checkstyle

ed80f87

Fix unit test; Improve code

075f79b

checkstyle

3f68af1

xichen01 force-pushed the HDDS-7253 branch from 1b3d8cc to 3f68af1 Compare October 10, 2022 02:48

Adaptation createDirectoryKey modified

17de24c

sadanand48 reviewed Oct 11, 2022

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java Outdated Show resolved Hide resolved

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java Outdated Show resolved Hide resolved

xichen01 added 3 commits October 12, 2022 23:03

Remove catch Exection; Change createFile to openKey

cafa3a9

fix compile issue

8c38928

fix integration test testLookupKeyWithLocation

3cf696e

duongkame approved these changes Oct 13, 2022

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java Show resolved Hide resolved

Extract "create fake dir" logic as a function

dc8fdd0

xichen01 requested review from duongkame and sadanand48 and removed request for duongkame and sadanand48 October 18, 2022 15:15

sadanand48 approved these changes Oct 18, 2022

View reviewed changes

kaijchen merged commit b9a47f6 into apache:master Oct 25, 2022

kaijchen pushed a commit to kaijchen/ozone that referenced this pull request Oct 26, 2022

HDDS-7253. Fix exception when '/' in key name (apache#3774)

1add651

kaijchen added a commit that referenced this pull request Oct 26, 2022

Revert "HDDS-7253. Fix exception when '/' in key name (#3774)"

561788e

This reverts commit b9a47f6.

kaijchen added a commit that referenced this pull request Oct 26, 2022

Revert "HDDS-7253. Fix exception when '/' in key name (#3774)"

758dbfa

This reverts commit b9a47f6.

xichen01 mentioned this pull request Dec 3, 2022

HDDS-7253. Fix exception when '/' in key name #4038

Merged

hemantk-12 mentioned this pull request Feb 1, 2023

HDDS-7861. [Snapshot] Delete keys from the source bucket to fix the flakiness of snapshot restore tests #4230

Merged

HDDS-7253. Fix exception when '/' in key name #3774

HDDS-7253. Fix exception when '/' in key name #3774

Uh oh!

Conversation

xichen01 commented Sep 22, 2022

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

sadanand48 left a comment

Choose a reason for hiding this comment

Uh oh!

xichen01 commented Sep 23, 2022

Uh oh!

sadanand48 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kerneltime commented Sep 26, 2022

Uh oh!

xichen01 commented Oct 10, 2022

Uh oh!

sadanand48 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

duongkame left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sadanand48 left a comment

Choose a reason for hiding this comment

Uh oh!

kaijchen commented Oct 25, 2022

Uh oh!

kaijchen commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xichen01 commented Oct 26, 2022

Uh oh!

kaijchen commented Oct 26, 2022

Uh oh!

sadanand48 commented Nov 28, 2022

Uh oh!

kaijchen commented Nov 28, 2022

Uh oh!

xichen01 commented Nov 28, 2022

Uh oh!

xichen01 commented Nov 28, 2022

Uh oh!

kaijchen commented Nov 29, 2022

Uh oh!

xichen01 commented Dec 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sadanand48 left a comment •

edited

Loading

sadanand48 left a comment •

edited

Loading

kaijchen commented Oct 26, 2022 •

edited

Loading

xichen01 commented Dec 3, 2022 •

edited

Loading