Copilot commented on code in PR #11847:
URL: https://github.com/apache/ozone/pull/11847#discussion_r4413507079
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantCreateRequest.java:
##########
@@ -172,6 +173,14 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
throw new OMException("Volume already exists", VOLUME_ALREADY_EXISTS);
}
+ // A volume can only be used by one tenant
+ if (forceCreationWhenVolumeExists) {
+ final OmVolumeArgs existingVolumeArgs =
ozoneManager.getMetadataManager().getVolumeTable().get(dbVolumeKey);
+ if (existingVolumeArgs != null && existingVolumeArgs.getRefCount() !=
0L) {
+ throw new OMException("Volume '" + volumeName + "' is already used by
a tenant", VOLUME_IS_REFERENCED);
Review Comment:
The error message string is duplicated in both `preExecute` and
`validateAndUpdateCache`. Consider extracting it to a private constant or a
small helper (e.g., `volumeReferencedMessage(volumeName)`) to prevent
accidental drift and keep tests from becoming brittle if the message changes in
one location.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantCreateRequest.java:
##########
@@ -172,6 +173,14 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
throw new OMException("Volume already exists", VOLUME_ALREADY_EXISTS);
}
+ // A volume can only be used by one tenant
+ if (forceCreationWhenVolumeExists) {
+ final OmVolumeArgs existingVolumeArgs =
ozoneManager.getMetadataManager().getVolumeTable().get(dbVolumeKey);
+ if (existingVolumeArgs != null && existingVolumeArgs.getRefCount() !=
0L) {
+ throw new OMException("Volume '" + volumeName + "' is already used by
a tenant", VOLUME_IS_REFERENCED);
Review Comment:
`preExecute` uses a direct table `get(...)` for the refCount check, while
`validateAndUpdateCache` uses `getVolumeInfo(...)`. To keep behavior consistent
(and avoid subtle differences in how cache/DB state is resolved), consider
using the same helper method in both places (or extracting a shared helper) so
`preExecute` and apply evaluate the refCount from the same source of truth.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/tenant/TestOMTenantCreateRequest.java:
##########
@@ -199,6 +207,41 @@ public void testValidateAndUpdateCacheWhenVolumeExists()
throws Exception {
assertEquals(Status.OK, omResponse.getStatus());
assertNotNull(omMetadataManager.getVolumeTable().get(
omMetadataManager.getVolumeKey(tenantId)));
+ assertEquals(1L,
omMetadataManager.getVolumeTable().get(omMetadataManager.getVolumeKey(tenantId)).getRefCount());
+ }
+
+ @Test
+ public void testRejectForceCreationOnVolumeUsedByAnotherTenant() throws
Exception {
+ acceptTenantIdCreationHelper("tenanta");
+
+ OMException omException = assertThrows(OMException.class, () ->
doPreExecute("tenantb", "tenanta", true));
+ assertEquals(VOLUME_IS_REFERENCED, omException.getResult());
+ verify(ozoneManager.getMultiTenantManager().getAuthorizerOp(),
never()).createTenant(eq("tenantb"), any(), any());
+
+ // Without force, the existing volume check still comes first
+ omException = assertThrows(OMException.class, () ->
doPreExecute("tenantb", "tenanta", false));
+ assertEquals(VOLUME_ALREADY_EXISTS, omException.getResult());
+ }
+
+ @Test
+ public void
testRejectForceCreationWhenVolumeBecomesReferencedAfterPreExecute() throws
Exception {
+ OMRequestTestUtils.addVolumeToDB("vol", "username", omMetadataManager);
+
+ // Both requests pass preExecute before either is applied
+ OMTenantCreateRequest request1 = doPreExecute("tenant1", "vol", true);
+ OMTenantCreateRequest request2 = doPreExecute("tenant2", "vol", true);
+
+ assertEquals(Status.OK, request1.validateAndUpdateCache(ozoneManager,
2L).getOMResponse().getStatus());
+
+ OMResponse omResponse = request2.validateAndUpdateCache(ozoneManager,
3L).getOMResponse();
+ assertEquals(Status.VOLUME_IS_REFERENCED, omResponse.getStatus());
+ assertEquals("Volume 'vol' is already used by a tenant",
omResponse.getMessage());
Review Comment:
This assertion hard-codes an exact error message, which makes the test
fragile to future wording changes (even if semantics remain correct). Prefer
asserting on the status/result code only, or use a less strict assertion (e.g.,
that the message contains the volume name and “already used”) or a shared
constant if one exists for this message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
