adoroszlai commented on code in PR #8251:
URL: https://github.com/apache/ozone/pull/8251#discussion_r2043734071
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1421,17 +1607,30 @@ public DecommissionScmResponseProto decommissionScm(
getScm().checkAdminAccess(getRemoteUser(), false);
decommissionScmResponseBuilder
.setSuccess(scm.removePeerFromHARing(scmId));
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId",
scmId)));
} catch (IOException ex) {
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId",
scmId), ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/ozone/audit/SCMAction.java:
##########
@@ -54,7 +54,20 @@ public enum SCMAction implements AuditAction {
GET_REPLICATION_MANAGER_REPORT,
RESET_DELETED_BLOCK_RETRY_COUNT,
TRANSFER_LEADERSHIP,
- GET_FAILED_DELETED_BLOCKS_TRANSACTION;
+ GET_CONTAINER_REPLICAS,
+ GET_FAILED_DELETED_BLOCKS_TRANSACTION,
Review Comment:
Note: usually enum values should be added at the end, keeping existing ones
unchanged, for compatibility. In this case I don't think it matters, since
values of `SCMAction` are not persisted or sent to remote node.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +808,32 @@ public Pipeline
createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ try {
+ Pipeline pipeline =
scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID",
String.valueOf(pipelineID))));
+ return pipeline;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID",
String.valueOf(pipelineID)), ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -903,8 +983,8 @@ public List<DeletedBlocksTransactionInfo>
getFailedDeletedBlockTxn(int count,
@Override
public int resetDeletedBlockRetryCount(List<Long> txIDs) throws IOException {
Map<String, String> auditMap = Maps.newHashMap();
- getScm().checkAdminAccess(getRemoteUser(), false);
try {
+ getScm().checkAdminAccess(getRemoteUser(), false);
int count = scm.getScmBlockManager().getDeletedBlockLog().
resetCount(txIDs);
auditMap.put("txIDs", txIDs.toString());
Review Comment:
Move `auditMap.put` before `try` block. Also in `closePipeline`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -815,33 +901,29 @@ public ScmInfo getScmInfo() {
.setClusterId(scm.getScmStorageConfig().getClusterID())
.setScmId(scm.getScmStorageConfig().getScmId())
.setPeerRoles(scm.getScmHAManager().getRatisServer().getRatisRoles());
- return builder.build();
+ ScmInfo info = builder.build();
Review Comment:
nit:
```suggestion
ScmInfo info = builder.build();
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -627,45 +670,72 @@ public HddsProtos.Node queryNode(UUID uuid)
.build();
}
} catch (NodeNotFoundException e) {
- throw new IOException(
+ IOException ex = new IOException(
"An unexpected error occurred querying the NodeStatus", e);
+ AUDIT.logReadFailure(buildAuditMessageForFailure(SCMAction.QUERY_NODE,
+ Collections.singletonMap("uuid", String.valueOf(uuid)), ex));
+ throw ex;
}
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(SCMAction.QUERY_NODE,
+ Collections.singletonMap("uuid", String.valueOf(uuid))));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1421,17 +1607,30 @@ public DecommissionScmResponseProto decommissionScm(
getScm().checkAdminAccess(getRemoteUser(), false);
decommissionScmResponseBuilder
.setSuccess(scm.removePeerFromHARing(scmId));
+ AUDIT.logWriteSuccess(buildAuditMessageForSuccess(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId",
scmId)));
} catch (IOException ex) {
decommissionScmResponseBuilder
.setSuccess(false)
.setErrorMsg(ex.getMessage());
+ AUDIT.logWriteFailure(buildAuditMessageForFailure(
+ SCMAction.DECOMMISSION_SCM, Collections.singletonMap("scmId",
scmId), ex));
}
return decommissionScmResponseBuilder.build();
}
@Override
public String getMetrics(String query) throws IOException {
- FetchMetrics fetchMetrics = new FetchMetrics();
- return fetchMetrics.getMetrics(query);
+ try {
+ FetchMetrics fetchMetrics = new FetchMetrics();
+ String metrics = fetchMetrics.getMetrics(query);
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_METRICS, Collections.singletonMap("query", query)));
+ return metrics;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_METRICS, Collections.singletonMap("query", query),
ex));
Review Comment:
nit: please store audit map in variable to avoid duplication
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -737,17 +808,32 @@ public Pipeline
createReplicationPipeline(HddsProtos.ReplicationType type,
}
@Override
- public List<Pipeline> listPipelines() {
- AUDIT.logReadSuccess(
- buildAuditMessageForSuccess(SCMAction.LIST_PIPELINE, null));
- return scm.getPipelineManager().getPipelines();
+ public List<Pipeline> listPipelines() throws IOException {
+ try {
+ List<Pipeline> pipelines = scm.getPipelineManager().getPipelines();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, null));
+ return pipelines;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, null, ex));
+ throw ex;
+ }
}
@Override
public Pipeline getPipeline(HddsProtos.PipelineID pipelineID)
throws IOException {
- return scm.getPipelineManager().getPipeline(
- PipelineID.getFromProtobuf(pipelineID));
+ try {
+ Pipeline pipeline =
scm.getPipelineManager().getPipeline(PipelineID.getFromProtobuf(pipelineID));
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID",
String.valueOf(pipelineID))));
+ return pipeline;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.LIST_PIPELINE, Collections.singletonMap("pipelineID",
String.valueOf(pipelineID)), ex));
Review Comment:
This should be `SCMAction.GET_PIPELINE`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -1270,62 +1402,116 @@ public List<HddsProtos.DatanodeUsageInfoProto>
getDatanodeUsageInfo(
boolean mostUsed, int count, int clientVersion)
throws IOException, IllegalArgumentException {
- // check admin authorisation
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("mostUsed", String.valueOf(mostUsed));
+ auditMap.put("count", String.valueOf(count));
+ auditMap.put("clientVersion", String.valueOf(clientVersion));
+
try {
+ // check admin authorisation
getScm().checkAdminAccess(getRemoteUser(), true);
- } catch (IOException e) {
- LOG.error("Authorization failed", e);
- throw e;
- }
+ if (count < 1) {
+ throw new IllegalArgumentException("The specified parameter count must
" +
+ "be an integer greater than zero.");
+ }
+ List<DatanodeUsageInfo> datanodeUsageInfoList =
+ scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
- if (count < 1) {
- throw new IllegalArgumentException("The specified parameter count must "
+
- "be an integer greater than zero.");
- }
+ // if count is greater than the size of list containing healthy,
+ // in-service nodes, just set count to that size
+ if (count > datanodeUsageInfoList.size()) {
+ count = datanodeUsageInfoList.size();
+ }
- List<DatanodeUsageInfo> datanodeUsageInfoList =
- scm.getScmNodeManager().getMostOrLeastUsedDatanodes(mostUsed);
+ // return count number of DatanodeUsageInfoProto
+ List<HddsProtos.DatanodeUsageInfoProto> result =
datanodeUsageInfoList.stream()
+ .map(each -> each.toProto(clientVersion))
+ .limit(count)
+ .collect(Collectors.toList());
- // if count is greater than the size of list containing healthy,
- // in-service nodes, just set count to that size
- if (count > datanodeUsageInfoList.size()) {
- count = datanodeUsageInfoList.size();
+ AUDIT.logReadSuccess(buildAuditMessageForSuccess(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap));
+ return result;
+ } catch (Exception ex) {
+ AUDIT.logReadFailure(buildAuditMessageForFailure(
+ SCMAction.GET_DATANODE_USAGE_INFO, auditMap, ex));
+ throw ex;
}
-
- // return count number of DatanodeUsageInfoProto
- return datanodeUsageInfoList.stream()
- .map(each -> each.toProto(clientVersion))
- .limit(count)
- .collect(Collectors.toList());
}
@Override
public Token<?> getContainerToken(ContainerID containerID)
throws IOException {
- UserGroupInformation remoteUser = getRemoteUser();
- getScm().checkAdminAccess(remoteUser, true);
+ final Map<String, String> auditMap = Maps.newHashMap();
+ auditMap.put("containerID", String.valueOf(containerID));
+ try {
+ UserGroupInformation remoteUser = getRemoteUser();
+ getScm().checkAdminAccess(getRemoteUser(), true);
- return scm.getContainerTokenGenerator()
- .generateToken(remoteUser.getUserName(), containerID);
+ Token<?> token = scm.getContainerTokenGenerator()
Review Comment:
nit:
```suggestion
Token<?> token = scm.getContainerTokenGenerator()
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -863,17 +945,14 @@ public void transferLeadership(String newLeaderId)
RatisHelper.transferRatisLeadership(scm.getConfiguration(), group,
targetPeerId, tlsConfig);
+
} catch (Exception ex) {
Review Comment:
nit: let's not add empty lines at the end of blocks
--
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]
