You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/05/11 05:47:34 UTC
[GitHub] [ozone] bharatviswa504 opened a new pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
bharatviswa504 opened a new pull request #2236:
URL: https://github.com/apache/ozone/pull/2236
## What changes were proposed in this pull request?
Make admin commands like safe mode etc., work on SCM HA cluster.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5205
## How was this patch tested?
Added a smoke test.
--
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.
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
[GitHub] [ozone] bharatviswa504 commented on pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#issuecomment-840455165
Thank You @adoroszlai for the review. I have addressed review comments.
--
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.
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
[GitHub] [ozone] bharatviswa504 commented on pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#issuecomment-840594290
Thank You @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.
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
[GitHub] [ozone] bharatviswa504 merged pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #2236:
URL: https://github.com/apache/ozone/pull/2236
--
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.
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r629978932
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
Review comment:
Can we store result of `Server.getRemoteUser()` and reuse it for both audit and check?
```suggestion
UserGroupInformation remoteUser = Server.getRemoteUser();
auditMap.put("remoteUser", remoteUser.getUserName());
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
try {
- getScm().checkAdminAccess(remoteUser);
+ getScm().checkAdminAccess(Server.getRemoteUser());
Review comment:
```suggestion
getScm().checkAdminAccess(remoteUser);
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -502,7 +497,7 @@ public void closeContainer(long containerID) throws IOException {
auditMap.put("containerID", String.valueOf(containerID));
auditMap.put("remoteUser", remoteUser);
try {
- scm.checkAdminAccess(remoteUser);
+ scm.checkAdminAccess(Server.getRemoteUser());
Review comment:
Same here. If both of these are updated, `getRpcRemoteUsername()` can be deleted.
##########
File path: hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test.sh
##########
@@ -34,6 +34,9 @@ execute_robot_test ${SCM} freon
execute_robot_test ${SCM} basic/links.robot
execute_robot_test ${SCM} s3
+
+export SCM=scm2.org
+execute_robot_test ${SCM} security-ha
Review comment:
Instead of introducing a new test suite (`security-ha`) with only a single test case that is not even HA-specific, I think we should run the complete `admincli` test suite (probably for both scm1 and scm2).
```suggestion
execute_robot_test ${SCM} admincli
export SCM=scm2.org
execute_robot_test ${SCM} admincli
```
Only one test case in each `admincli` test file needs to be changed for this to work: `... on unknown host` fails because `--scm` parameter is ignored for SCM HA cluster. I think it is a bug that should be fixed in the long term. But for now these test cases can be disabled (commented out), as they are not too important. I think it is more important to have full `admincli` test coverage for HA than to run this marginal test case for non-HA.
https://github.com/apache/ozone/blob/49538aaa11f41bf80f565f9ce4f2c49ebac5fd51/hadoop-ozone/dist/src/main/smoketest/admincli/container.robot#L75-L77
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
try {
- getScm().checkAdminAccess(remoteUser);
+ getScm().checkAdminAccess(Server.getRemoteUser());
Review comment:
Also note that `TestStorageContainerManager` integration test is failing, because `Server.getRemoteUser()` returns `null` (no mock set up for this static method, as opposed to `SCMClientProtocolServer.getRpcRemoteUsername()`), so access is granted.
https://github.com/apache/ozone/pull/2236/checks?check_run_id=2553193751#step:4:4223
--
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.
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r631696307
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -404,13 +403,12 @@ private boolean hasRequiredReplicas(ContainerInfo contInfo) {
@Override
public void deleteContainer(long containerID) throws IOException {
- String remoteUser = getRpcRemoteUsername();
boolean auditSuccess = true;
Map<String, String> auditMap = Maps.newHashMap();
auditMap.put("containerID", String.valueOf(containerID));
- auditMap.put("remoteUser", remoteUser);
+ auditMap.put("remoteUser", getRpcRemoteUsername());
try {
- getScm().checkAdminAccess(remoteUser);
+ getScm().checkAdminAccess(Server.getRemoteUser());
Review comment:
Fixed it
--
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.
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r631822221
##########
File path: hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test.sh
##########
@@ -34,6 +34,9 @@ execute_robot_test ${SCM} freon
execute_robot_test ${SCM} basic/links.robot
execute_robot_test ${SCM} s3
+
+export SCM=scm2.org
+execute_robot_test ${SCM} security-ha
Review comment:
I guess SCM service ID and SCM host should be exclusive, only one of these should be allowed.
https://github.com/apache/ozone/blob/03620da6158534ac618354737a8b35d09a25fa33/hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ScmOption.java#L46-L53
--
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.
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r631697348
##########
File path: hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test.sh
##########
@@ -34,6 +34,9 @@ execute_robot_test ${SCM} freon
execute_robot_test ${SCM} basic/links.robot
execute_robot_test ${SCM} s3
+
+export SCM=scm2.org
+execute_robot_test ${SCM} security-ha
Review comment:
For ha --scm should it pass multiple scms? What is the expected behavior, I can open a Jira for this.
--
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.
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2236: HDDS-5205. Make admin check work for SCM HA cluster
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2236:
URL: https://github.com/apache/ozone/pull/2236#discussion_r631686665
##########
File path: hadoop-ozone/dist/src/main/compose/ozonesecure-ha/test.sh
##########
@@ -34,6 +34,9 @@ execute_robot_test ${SCM} freon
execute_robot_test ${SCM} basic/links.robot
execute_robot_test ${SCM} s3
+
+export SCM=scm2.org
+execute_robot_test ${SCM} security-ha
Review comment:
Done. For now, commented them.
--
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.
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