You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Tejaskriya (via GitHub)" <gi...@apache.org> on 2025/02/04 08:40:09 UTC
[PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Tejaskriya opened a new pull request, #7805:
URL: https://github.com/apache/ozone/pull/7805
## What changes were proposed in this pull request?
As a part of SCM audits for datanode's heartbeats, the command.toString for the commands being sent to the datanodes are logged. For most commands this results in a short string, but for deleteBlocks command, all the delete transactions are also added. This leads to a very verbose audit log.
SCM should add only the metadata response to the logs appropriately for each of the commands.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12175
## How was this patch tested?
Tested manually
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2642116114
You can add the debug log in `SCMDatanodeHeartbeatDispatcher#dispatch`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2654589654
Thanks @Tejaskriya for the contribution! Thanks @adoroszlai for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7805:
URL: https://github.com/apache/ozone/pull/7805#discussion_r1942900830
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,22 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append('[');
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType:" + cmd.getCommandType());
+ auditMap.append(" No. of deleteTransactions:");
+ auditMap.append(cmd.getDeleteBlocksCommandProto().getDeletedBlocksTransactionsList().size());
+ } else {
Review Comment:
I guess some separator is also needed between commands, maybe between command attributes, too. The goal is to most closely match previous output while ommitting unwanted details.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,22 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append('[');
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType:" + cmd.getCommandType());
+ auditMap.append(" No. of deleteTransactions:");
Review Comment:
Avoid space in key:
```suggestion
auditMap.append(" deleteTransactionsCount:");
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,22 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append('[');
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType:" + cmd.getCommandType());
Review Comment:
Append separately:
```suggestion
auditMap.append("commandType:").append(cmd.getCommandType());
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2638777438
@adoroszlai I have addressed all the comments, Thanks for suggesting these improvements!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2637765303
@adoroszlai Thanks for the review! I have made the changes and added the old and new audit log in the PR description.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 merged PR #7805:
URL: https://github.com/apache/ozone/pull/7805
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2653391580
Thanks for the review @nandakumar131 , I have incorporated the suggestions
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7805:
URL: https://github.com/apache/ozone/pull/7805#discussion_r1943675167
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,29 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append("[");
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType: ").append(cmd.getCommandType());
+ auditMap.append(" deleteTransactionsCount:");
+ auditMap.append(cmd.getDeleteBlocksCommandProto().getDeletedBlocksTransactionsList().size());
+ } else {
+ auditMap.append(cmd);
+ }
+ auditMap.append(" , ");
Review Comment:
```suggestion
auditMap.append(", ");
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,29 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append("[");
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType: ").append(cmd.getCommandType());
+ auditMap.append(" deleteTransactionsCount:");
+ auditMap.append(cmd.getDeleteBlocksCommandProto().getDeletedBlocksTransactionsList().size());
+ } else {
+ auditMap.append(cmd);
+ }
+ auditMap.append(" , ");
+ }
+ int len = auditMap.toString().length();
Review Comment:
Let's not call `toString()` twice.
```suggestion
int len = auditMap.length();
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java:
##########
@@ -280,6 +280,29 @@ public static SCMRegisteredResponseProto getRegisteredResponse(
return cmd.getProtoBufMessage();
}
+ private String constructCommandAuditMap(List<SCMCommandProto> cmds) {
+ StringBuilder auditMap = new StringBuilder();
+ auditMap.append("[");
+ for (SCMCommandProto cmd : cmds) {
+ if (cmd.getCommandType().equals(deleteBlocksCommand)) {
+ auditMap.append("commandType: ").append(cmd.getCommandType());
+ auditMap.append(" deleteTransactionsCount:");
+ auditMap.append(cmd.getDeleteBlocksCommandProto().getDeletedBlocksTransactionsList().size());
+ } else {
+ auditMap.append(cmd);
+ }
+ auditMap.append(" , ");
+ }
+ int len = auditMap.toString().length();
+ if (len > 3) {
+ auditMap.deleteCharAt(len - 1);
+ auditMap.deleteCharAt(len - 2);
+ auditMap.deleteCharAt(len - 3);
+ }
Review Comment:
I think this can be simplified.
(3 should be 2 if ` ,` is changed to `, `.)
```suggestion
if (len > 3) {
auditMap.delete(len - 3, len);
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7805:
URL: https://github.com/apache/ozone/pull/7805#discussion_r1952368320
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -200,6 +200,17 @@ public List<SCMCommand> dispatch(SCMHeartbeatRequestProto heartbeat) {
}
}
}
+ if (LOG.isDebugEnabled()) {
+ StringBuilder allCommands = new StringBuilder();
+ for (SCMCommand cmd : commands) {
+ allCommands.append(cmd).append(", ");
+ }
+ int len = allCommands.length();
+ if (len > 2) {
+ allCommands.delete(len - 2, len);
+ }
+ LOG.debug("Heartbeat dispatched: datanode=" + datanodeDetails.getUuid() + ", Commands= [" + allCommands + "]");
Review Comment:
```suggestion
LOG.debug("Heartbeat dispatched: datanode=" + datanodeDetails.getUuid() + ", Commands= " + commands);
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/protocol/commands/DeleteBlocksCommand.java:
##########
@@ -68,4 +68,25 @@ public DeleteBlocksCommandProto getProto() {
.setCmdId(getId())
.addAllDeletedBlocksTransactions(blocksTobeDeleted).build();
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append(getType())
+ .append(": cmdID: ").append(getId())
+ .append(", encodedToken: \"").append(getEncodedToken()).append("\"")
+ .append(", term: ").append(getTerm())
+ .append(", deadlineMsSinceEpoch: ").append(getDeadline())
+ .append(", deletedBlocksTransaction: [");
+ for (DeletedBlocksTransaction txn : blocksTobeDeleted) {
+ sb.append(" txnID:").append(txn.getTxID())
+ .append(",containerID:").append(txn.getContainerID())
+ .append(",deleteBlockCount:").append(txn.getLocalIDCount())
+ .append(",count:").append(txn.getCount())
+ .append(",");
+ }
+ sb.deleteCharAt(sb.length() - 1);
Review Comment:
```suggestion
for (DeletedBlocksTransaction txn : blocksTobeDeleted) {
sb.append("{ txnID: ").append(txn.getTxID())
.append(", containerID: ").append(txn.getContainerID())
.append(", deleteBlockCount: ").append(txn.getLocalIDCount())
.append(", count: ").append(txn.getCount())
.append("}, ");
}
if (!blocksTobeDeleted.isEmpty()) {
sb.delete(sb.length() - 2, sb.length());
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #7805:
URL: https://github.com/apache/ozone/pull/7805#discussion_r1946761559
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -200,6 +203,34 @@ public List<SCMCommand> dispatch(SCMHeartbeatRequestProto heartbeat) {
}
}
}
+ if (LOG.isDebugEnabled()) {
+ StringBuilder allCommands = new StringBuilder();
+ for (SCMCommand cmd : commands) {
+ allCommands.append("cmdID: ").append(cmd.getId());
+ allCommands.append(" encodedToken: \"").append(cmd.getEncodedToken()).append("\"");
+ allCommands.append(" term: ").append(cmd.getTerm());
+ allCommands.append(" deadlineMsSinceEpoch: ").append(cmd.getDeadline());
+ if (cmd.getType().equals(deleteBlocksCommand)) {
+ DeleteBlocksCommand delCmd = (DeleteBlocksCommand) cmd;
+ allCommands.append(" deleteBlocksTransactions: {");
+ for (DeletedBlocksTransaction txn : delCmd.blocksTobeDeleted()) {
+ allCommands.append(" txnId:").append(txn.getTxID());
+ allCommands.append(" containerID:").append(txn.getContainerID());
+ allCommands.append(" deleteBlockCount:").append(txn.getLocalIDCount());
+ allCommands.append(" count:").append(txn.getCount());
+ allCommands.append(",");
+ }
+ allCommands.deleteCharAt(allCommands.length() - 1);
+ allCommands.append(" }");
+ } else {
+ allCommands.append(" ").append(cmd.getProto());
+ }
+ allCommands.append(", ");
+ }
+ int len = allCommands.length();
+ allCommands.delete(len - 2, len);
Review Comment:
`SCMCommand` is not a protobuf object, you can override `toString` method in all the `SCMCommand` implementation and just logging the `SCMCommand#toString` here should be enough.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on PR #7805:
URL: https://github.com/apache/ozone/pull/7805#issuecomment-2642307998
Thanks for the review @nandakumar131 , I have added the extra details to the audit log and also added a debug log in `SCMDatanodeHeartbeatDispatcher#dispatch`.
The updated output is added to the PR description
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12175. Audit logs in SCM shouldn't print delete txns [ozone]
Posted by "Tejaskriya (via GitHub)" <gi...@apache.org>.
Tejaskriya commented on code in PR #7805:
URL: https://github.com/apache/ozone/pull/7805#discussion_r1950436991
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -200,6 +203,34 @@ public List<SCMCommand> dispatch(SCMHeartbeatRequestProto heartbeat) {
}
}
}
+ if (LOG.isDebugEnabled()) {
+ StringBuilder allCommands = new StringBuilder();
+ for (SCMCommand cmd : commands) {
+ allCommands.append("cmdID: ").append(cmd.getId());
+ allCommands.append(" encodedToken: \"").append(cmd.getEncodedToken()).append("\"");
+ allCommands.append(" term: ").append(cmd.getTerm());
+ allCommands.append(" deadlineMsSinceEpoch: ").append(cmd.getDeadline());
+ if (cmd.getType().equals(deleteBlocksCommand)) {
+ DeleteBlocksCommand delCmd = (DeleteBlocksCommand) cmd;
+ allCommands.append(" deleteBlocksTransactions: {");
+ for (DeletedBlocksTransaction txn : delCmd.blocksTobeDeleted()) {
+ allCommands.append(" txnId:").append(txn.getTxID());
+ allCommands.append(" containerID:").append(txn.getContainerID());
+ allCommands.append(" deleteBlockCount:").append(txn.getLocalIDCount());
+ allCommands.append(" count:").append(txn.getCount());
+ allCommands.append(",");
+ }
+ allCommands.deleteCharAt(allCommands.length() - 1);
+ allCommands.append(" }");
+ } else {
+ allCommands.append(" ").append(cmd.getProto());
+ }
+ allCommands.append(", ");
+ }
+ int len = allCommands.length();
+ allCommands.delete(len - 2, len);
Review Comment:
I have changed the toString method for all commands to print the required information along with the common details like cmdID, deadline, term and encodedToken in the latest commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org