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/22 11:49:56 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
adoroszlai opened a new pull request #2277:
URL: https://github.com/apache/ozone/pull/2277
## What changes were proposed in this pull request?
Close container command from SCM fails due to lack of token. This change adds the token to `SCMCommand`.
https://issues.apache.org/jira/browse/HDDS-5264
## How was this patch tested?
Updated acceptance test to verify that the container actually gets `CLOSED` (previously it was stuck in `CLOSING`).
--
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] xiaoyuyao commented on a change in pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#discussion_r641713924
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/CloseContainerEventHandler.java
##########
@@ -99,6 +101,14 @@ public void onMessage(ContainerID containerID, EventPublisher publisher) {
}
}
+ private String getContainerToken(ContainerID containerID) {
+ StorageContainerManager scm = scmContext.getScm();
+ return scm != null
+ ? scm.getContainerTokenGenerator()
+ .generateEncodedToken(getClass().getSimpleName(), containerID)
Review comment:
Yes. I mean a class name can be confusing here as the close container command are sent by scm. There are other cases where scm admin cli can send the close container command to datanode, where should also use proper user from UGI.
--
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 pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#issuecomment-846825790
Thanks @GlenGeng for taking a look.
> The purpose is to add token support for SCMCommand, better change the PR title to make it more straightforward.
>
> Question: will other SCMCommand fail due to lack of token, since we have quite a few subclass of SCMCommand ?
As far as I see, currently only `CloseContainer` command needs this token, hence the title. I added the token to the enclosing `SCMCommand` to allow other commands (existing or new ones) to use it later, if needed.
--
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 pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#issuecomment-852416252
Thanks @GlenGeng and @xiaoyuyao 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] GlenGeng commented on pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#issuecomment-846799456
The purpose is to add token support for SCMCommand, better change the PR title to make it more straightforward.
Question: will other SCMCommand fail due to lack of token, since we have quite a few subclass of SCMCommand ?
--
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] GlenGeng commented on pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#issuecomment-846851315
Thanks @adoroszlai for the clarificaiton!
From the fix, it shows that `CloseContainerCommand` is the only `SCMCommand` translated into a `ContainerCommandRequestProto`, which go through ratis. I guess it is why the token is needed for `CloseContainerCommand `.
Will it be better to move the token to `DeleteContainerCommandProto` to narrow down its visibility ?
--
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 merged pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #2277:
URL: https://github.com/apache/ozone/pull/2277
--
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 #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#discussion_r641185076
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/CloseContainerEventHandler.java
##########
@@ -99,6 +101,14 @@ public void onMessage(ContainerID containerID, EventPublisher publisher) {
}
}
+ private String getContainerToken(ContainerID containerID) {
+ StorageContainerManager scm = scmContext.getScm();
+ return scm != null
+ ? scm.getContainerTokenGenerator()
+ .generateEncodedToken(getClass().getSimpleName(), containerID)
Review comment:
Thanks @xiaoyuyao for the review. Do you mean we should use the UGI that SCM is running with?
--
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] xiaoyuyao commented on a change in pull request #2277: HDDS-5264. SCM should send token for CloseContainer command
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2277:
URL: https://github.com/apache/ozone/pull/2277#discussion_r640972808
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/CloseContainerEventHandler.java
##########
@@ -99,6 +101,14 @@ public void onMessage(ContainerID containerID, EventPublisher publisher) {
}
}
+ private String getContainerToken(ContainerID containerID) {
+ StorageContainerManager scm = scmContext.getScm();
+ return scm != null
+ ? scm.getContainerTokenGenerator()
+ .generateEncodedToken(getClass().getSimpleName(), containerID)
Review comment:
should we pass meaningful name here instead of the class name as user for the container token?
--
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