Skip to content

HDDS-5279. OFS mkdir -p does not work when Volume is not pre-created #2412

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

Merged
smengcl merged 4 commits into from
Jul 29, 2021
Merged

HDDS-5279. OFS mkdir -p does not work when Volume is not pre-created #2412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d131c50
HDDS-5279. OFS mkdir -p does not work when Volume is not pre-created
smengcl Jul 13, 2021
50d211b
Address UT failure.
smengcl Jul 14, 2021
1c17d7c
Update UT, adding new enableAcl param.
smengcl Jul 14, 2021
db7c054
Retrigger CI
smengcl Jul 21, 2021
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
16 changes: 11 additions & 5 deletions .../integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.hadoop.ozone.MiniOzoneCluster;
import org.apache.hadoop.ozone.OFSPath;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.TestDataUtil;
import org.apache.hadoop.ozone.client.ObjectStore;
Expand Down Expand Up @@ -108,16 +109,19 @@ public class TestRootedOzoneFileSystem {
@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
new Object[]{true, true},
new Object[]{true, false},
new Object[]{false, true},
new Object[]{false, false});
new Object[]{true, true, true},
new Object[]{true, true, false},
new Object[]{true, false, false},
new Object[]{false, true, false},
new Object[]{false, false, false}
);
}

public TestRootedOzoneFileSystem(boolean setDefaultFs,
boolean enableOMRatis) {
boolean enableOMRatis, boolean isAclEnabled) {
enabledFileSystemPaths = setDefaultFs;
omRatisEnabled = enableOMRatis;
enableAcl = isAclEnabled;
}

public static FileSystem getFs() {
Expand All @@ -134,6 +138,7 @@ public static Path getBucketPath() {
private static boolean enabledFileSystemPaths;
private static boolean omRatisEnabled;
private static boolean isBucketFSOptimized = false;
private static boolean enableAcl;

private static OzoneConfiguration conf;
private static MiniOzoneCluster cluster = null;
Expand Down Expand Up @@ -165,6 +170,7 @@ public static void init() throws Exception {
conf.setBoolean(OMConfigKeys.OZONE_OM_ENABLE_FILESYSTEM_PATHS,
enabledFileSystemPaths);
}
conf.setBoolean(OzoneConfigKeys.OZONE_ACL_ENABLED, enableAcl);
cluster = MiniOzoneCluster.newBuilder(conf)
.setNumDatanodes(3)
.build();
Expand Down
9 changes: 5 additions & 4 deletions ...ation-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,14 @@ public class TestRootedOzoneFileSystemWithFSO
@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
new Object[]{true, true},
new Object[]{true, false});
new Object[]{true, true, false},
new Object[]{true, false, false}
);
}

public TestRootedOzoneFileSystemWithFSO(boolean setDefaultFs,
boolean enableOMRatis) throws Exception {
super(setDefaultFs, enableOMRatis);
boolean enableOMRatis, boolean enableAcl) {
super(setDefaultFs, enableOMRatis, enableAcl);
}

@BeforeClass
Expand Down
38 changes: 17 additions & 21 deletions ...fs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,31 +223,23 @@ private OzoneBucket getBucket(String volumeStr, String bucketStr,
try {
bucket = proxy.getBucketDetails(volumeStr, bucketStr);
} catch (OMException ex) {
// Note: always create bucket if volumeStr matches "tmp" so -put works
if (createIfNotExist) {
// Note: getBucketDetails always throws BUCKET_NOT_FOUND, even if
// the volume doesn't exist.
if (ex.getResult().equals(BUCKET_NOT_FOUND)) {
OzoneVolume volume;
// getBucketDetails can throw VOLUME_NOT_FOUND when the parent volume
// doesn't exist and ACL is enabled; it can only throw BUCKET_NOT_FOUND
// when ACL is disabled. Both exceptions need to be handled.
switch (ex.getResult()) {
case VOLUME_NOT_FOUND:
case BUCKET_NOT_FOUND:
Copy link
Contributor

@kerneltime kerneltime Jul 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The case where volume is not found should not be as common as bucket not found. I think it would be better to avoid the redundant call to create volume.

Copy link
Contributor Author

@smengcl smengcl Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the comment @kerneltime .

VOLUME_NOT_FOUND is actually frequently encountered during testing where we create volume+bucket+directories on one shot with mkdir -p when ACL is enabled.

If you think about it, in both cases the action(s) we need to take is the same. i.e. volume needs to be created.
It's just that the exception thrown from the call proxy.getBucketDetails is different -- which is bizzare, and we maybe want to fix getBucketDetails's behavior at some point (for example, to always throw VOLUME_NOT_FOUND regardless of whether ACL is enabled or not), after which we can throw away case BUCKET_NOT_FOUND.

Copy link
Contributor

@kerneltime kerneltime Jul 19, 2021

Choose a reason for hiding this comment

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

Do you want to create a newbie jira to clean up the behavior to send the appropriate error which is the right fix.

Copy link
Contributor Author

@smengcl smengcl Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I will file one. but probably not a newbie one since changing that could involve a series of changes -- throwing a different exception can break other existing behaviors.

try {
volume = proxy.getVolumeDetails(volumeStr);
} catch (OMException getVolEx) {
if (getVolEx.getResult().equals(VOLUME_NOT_FOUND)) {
// Volume doesn't exist. Create it
try {
objectStore.createVolume(volumeStr);
} catch (OMException newVolEx) {
// Ignore the case where another client created the volume
if (!newVolEx.getResult().equals(VOLUME_ALREADY_EXISTS)) {
throw newVolEx;
}
}
} else {
throw getVolEx;
objectStore.createVolume(volumeStr);
} catch (OMException newVolEx) {
// Ignore the case where another client created the volume
if (!newVolEx.getResult().equals(VOLUME_ALREADY_EXISTS)) {
throw newVolEx;
}
// Try get volume again
volume = proxy.getVolumeDetails(volumeStr);
}

OzoneVolume volume = proxy.getVolumeDetails(volumeStr);
// Create the bucket
try {
volume.createBucket(bucketStr);
Expand All @@ -257,6 +249,10 @@ private OzoneBucket getBucket(String volumeStr, String bucketStr,
throw newBucEx;
}
}
break;
default:
// Throw unhandled exception
throw ex;
}
// Try get bucket again
bucket = proxy.getBucketDetails(volumeStr, bucketStr);
Expand Down