Skip to content

HDDS-4441. Add metrics for ACL related operations.(Addendum for HA). #1584

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
cxorm merged 1 commit into from
Nov 16, 2020
Merged

HDDS-4441. Add metrics for ACL related operations.(Addendum for HA). #1584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 61 additions & 0 deletions hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.apache.hadoop.ozone.om;

import static org.apache.hadoop.ozone.OzoneAcl.AclScope.ACCESS;
import static org.apache.hadoop.ozone.security.acl.OzoneObj.ResourceType.BUCKET;
import static org.apache.hadoop.ozone.security.acl.OzoneObj.ResourceType.VOLUME;
import static org.apache.hadoop.ozone.security.acl.OzoneObj.StoreType.OZONE;
import static org.apache.hadoop.test.MetricsAsserts.assertCounter;
Expand All @@ -37,13 +38,15 @@
import org.apache.hadoop.ozone.MiniOzoneCluster;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.client.ObjectStore;
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer;
import org.apache.hadoop.ozone.security.acl.OzoneObj;
import org.apache.hadoop.ozone.security.acl.OzoneObjInfo;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -362,6 +365,64 @@ public void testAclOperations() throws IOException {
}
}

@Test
public void testAclOperationsHA() throws Exception {
ObjectStore objectStore = cluster.getClient().getObjectStore();
// Create a volume.
objectStore.createVolume("volumeacl");
// Create a bucket.
objectStore.getVolume("volumeacl").createBucket("bucketacl");
// Create a key.
objectStore.getVolume("volumeacl").getBucket("bucketacl")
.createKey("keyacl", 0).close();

OzoneObj volObj =
new OzoneObjInfo.Builder().setVolumeName("volumeacl").setResType(VOLUME)
.setStoreType(OZONE).build();

OzoneObj buckObj = new OzoneObjInfo.Builder().setVolumeName("volumeacl")
.setBucketName("bucketacl").setResType(BUCKET).setStoreType(OZONE)
.build();

OzoneObj keyObj = new OzoneObjInfo.Builder().setVolumeName("volumeacl")
.setBucketName("bucketacl").setResType(BUCKET).setKeyName("keyacl")
.setStoreType(OZONE).build();

List<OzoneAcl> acls = ozoneManager.getAcl(volObj);

// Test Acl's for volume.
testAclMetricsInternal(objectStore, volObj, acls);

// Test Acl's for bucket.
testAclMetricsInternal(objectStore, buckObj, acls);

// Test Acl's for key.
testAclMetricsInternal(objectStore, keyObj, acls);
}

private void testAclMetricsInternal(ObjectStore objectStore, OzoneObj volObj,
List<OzoneAcl> acls) throws IOException {
// Test addAcl
OMMetrics metrics = ozoneManager.getMetrics();
long initialValue = metrics.getNumAddAcl();
objectStore.addAcl(volObj,
new OzoneAcl(IAccessAuthorizer.ACLIdentityType.USER, "ozoneuser",
IAccessAuthorizer.ACLType.ALL, ACCESS));

Assert.assertEquals(initialValue + 1, metrics.getNumAddAcl());

// Test setAcl
initialValue = metrics.getNumSetAcl();

objectStore.setAcl(volObj, acls);
Assert.assertEquals(initialValue + 1, metrics.getNumSetAcl());

// Test removeAcl
initialValue = metrics.getNumRemoveAcl();
objectStore.removeAcl(volObj, acls.get(0));
Assert.assertEquals(initialValue + 1, metrics.getNumRemoveAcl());
}

/**
* Test volume operations with ignoring thrown exception.
*/
Expand Down
8 changes: 8 additions & 0 deletions ...er/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.common.collect.Lists;
import org.apache.hadoop.ozone.om.OMMetrics;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.util.BooleanBiFunction;
import org.apache.hadoop.util.Time;
Expand Down Expand Up @@ -130,5 +131,12 @@ void onComplete(boolean operationResult, IOException exception,
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumAddAcl();
Copy link
Member

@cxorm cxorm Nov 14, 2020

Choose a reason for hiding this comment

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

How about moving this line after true operationResult branch of onComplete() at line-120 ?

Copy link
Member Author

@ayushtkn ayushtkn Nov 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanx @cxorm for the review. In onComplete(), there doesn't seems to be any instance of ozoneManager available, and I quickly had a check over, couple of other files, they too were incrementing metrics in validateAndUpdateCache, I think we can be in sync with them.

Let me know, if you are ok going ahead this way.

Copy link
Member

@cxorm cxorm Nov 14, 2020

Choose a reason for hiding this comment

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

Thanx @cxorm for the review. In onComplete(), there doesn't seems to be any instance of ozoneManager available, and I quickly had a check over, couple of other files, they too were incrementing metrics in validateAndUpdateCache, I think we can be in sync with them.

Thanks @ayushtkn for the comment.

IMHO the onComplete() is called by super class validateAndUpdateCache (for example, in this part its super class is OMBucketAclRequest), and it has instance of ozoneManager.

An operation scenario is that when we call ozoneManager.getMetrics().incNumAddAcl() before super.validateAndUpdateCache() and failure occurs in validateAndUpdateCache(), the incNumAddAcl() shouldn't take effect, but it did.

Copy link
Member Author

@ayushtkn ayushtkn Nov 14, 2020

Choose a reason for hiding this comment

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

Thanx @cxorm
Are you talking about this onComplete?
https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java#L114

If not can you point me to that. It doesn't have ozonemanager

An operation scenario is that when we call ozoneManager.getMetrics().incNumAddAcl() before super.validateAndUpdateCache() and failure occurs in validateAndUpdateCache(), the incNumAddAcl() shouldn't take effect, but it did.

Aren't these metrics suppose to track the number of calls? not just success?

validateAndUpdateCache too has a metrics getting incremented, omMetrics.incNumVolumeUpdates();, if a failure occurs post that, it isn't bothered.

Copy link
Member Author

@ayushtkn ayushtkn Nov 14, 2020

Choose a reason for hiding this comment

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

A reference from HDFS namenode.
https://github.com/apache/hadoop/blob/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java#L1960

Here too, the metrics is incremented before and the execution is done post that, even if the call fails , the metrics shall still track that call.

Copy link
Contributor

@bharatviswa504 bharatviswa504 Nov 16, 2020

Choose a reason for hiding this comment

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

In Ozone, we increment these metrics represents number of operations, (Not the number of successful operations)
IMHO, this approach matches with all other requests, and LGTM.

Copy link
Member

@cxorm cxorm Nov 16, 2020

Choose a reason for hiding this comment

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

Thank you @bharatviswa504 for the clarification (and sorry for my previous view.)
I'm +1 for the patch.

return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketRemoveAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;

import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.util.Time;
import org.slf4j.Logger;
Expand Down Expand Up @@ -127,5 +128,13 @@ void onComplete(boolean operationResult, IOException exception,
}
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumRemoveAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...er/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.List;

import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.util.Time;
import org.slf4j.Logger;
Expand Down Expand Up @@ -128,5 +129,13 @@ void onComplete(boolean operationResult, IOException exception,
}
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumSetAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyAddAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.key.acl.OMKeyAclResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
Expand Down Expand Up @@ -116,5 +117,13 @@ boolean apply(OmKeyInfo omKeyInfo, long trxnLogIndex) {
// No need to check not null here, this will be never called with null.
return omKeyInfo.addAcl(ozoneAcls.get(0));
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumAddAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...nager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyRemoveAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.key.acl.OMKeyAclResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
Expand Down Expand Up @@ -117,5 +118,13 @@ boolean apply(OmKeyInfo omKeyInfo, long trxnLogIndex) {
// No need to check not null here, this will be never called with null.
return omKeyInfo.removeAcl(ozoneAcls.get(0));
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumRemoveAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeySetAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OzoneAclUtil;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.key.acl.OMKeyAclResponse;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
Expand Down Expand Up @@ -113,5 +114,13 @@ boolean apply(OmKeyInfo omKeyInfo, long trxnLogIndex) {
// No need to check not null here, this will be never called with null.
return omKeyInfo.setAcls(ozoneAcls);
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumSetAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}

9 changes: 9 additions & 0 deletions ...er/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.OMClientResponse;
import org.apache.hadoop.ozone.om.response.volume.OMVolumeAclOpResponse;
Expand Down Expand Up @@ -128,4 +129,12 @@ void onComplete(Result result, IOException ex, long trxnLogIndex) {
getOmRequest());
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumAddAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}
9 changes: 9 additions & 0 deletions ...src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.OMClientResponse;
import org.apache.hadoop.ozone.om.response.volume.OMVolumeAclOpResponse;
Expand Down Expand Up @@ -127,4 +128,12 @@ void onComplete(Result result, IOException ex, long trxnLogIndex) {
getOmRequest());
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumRemoveAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}
9 changes: 9 additions & 0 deletions ...er/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
import org.apache.hadoop.ozone.om.response.OMClientResponse;
import org.apache.hadoop.ozone.om.response.volume.OMVolumeAclOpResponse;
Expand Down Expand Up @@ -124,4 +125,12 @@ void onComplete(Result result, IOException ex, long trxnLogIndex) {
getOmRequest());
}
}

@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
ozoneManager.getMetrics().incNumSetAcl();
return super.validateAndUpdateCache(ozoneManager, trxnLogIndex,
omDoubleBufferHelper);
}
}