fix(volume): don't allow unpublish from non-frontend node #889
Conversation
dsharma-dc
requested review from
tiagolobocastro,
Abhinandan-Purkait and
abhilashshetty04
Signed-off-by: Diwakar Sharma <diwakar.sharma@datacore.com>
dsharma-dc
force-pushed
the
fix_github_1724
branch
from
ec0ad51
to
19e150e
Compare
| @@ -323,6 +323,8 @@ message UnpublishVolumeRequest { | |||
| // the nexus. Note: this option should be used only when we know the node will not become | |||
| // accessible again and it is safe to do so. | |||
| bool force = 2; | |||
| // frontend host requesting for volume unpublish. | |||
| optional string frontend_host = 3; | |||
There was a problem hiding this comment.
In-keeping with publish
| optional string frontend_host = 3; | |
| repeated string frontend_nodes = 3; |
| @@ -1618,6 +1618,8 @@ pub trait UnpublishVolumeInfo: Send + Sync + std::fmt::Debug { | |||
| fn uuid(&self) -> VolumeId; | |||
| /// Force unpublish | |||
| fn force(&self) -> bool; | |||
| /// Frontend node requesting unpublish. | |||
| fn frontend_host(&self) -> Option<NodeId>; | |||
There was a problem hiding this comment.
| fn frontend_host(&self) -> Option<NodeId>; | |
| fn frontend_nodes(&self) -> Vec<NodeId>; |
| And the next token should be empty | ||
|
|
||
| Scenario: unpublish volume from non-frontend node |
There was a problem hiding this comment.
We should cover the returned errors when the nodes don't match
| .map_err(|e| { | ||
| Status::not_found(format!( | ||
| .map_err(|e| match e { | ||
| ApiClientError::PreconditionFailed(_) => { |
There was a problem hiding this comment.
this feels somewhat wrong, what if target was not deleted because of some other precondition error.
Maybe we need a different error here that is clearer? Something more similar to NOT_FOUND perhaps? Given that the target is not available for this node.
There was a problem hiding this comment.
How about NotAcceptable? I intended to do that earlier
There was a problem hiding this comment.
Yeah that seems good. wdyt @Abhinandan-Purkait ?
There was a problem hiding this comment.
Yep. Although there is one more mapping of not acceptable but looking at other options this sounds reasonable.
In case application and volume target have already moved to a different node and there is a lagging ControllerUnpublish request that comes later, then this unpublish call can delete the newly created target, which is undesirable and leads to volume staging failures.