feat(replica_entity_id): adding implementation #1596
Conversation
abhilashshetty04
requested review from
dsavitskiy,
tiagolobocastro and
hrudaya21
| @@ -253,6 +254,9 @@ impl ReplicaRpc for ReplicaService { | |||
| } | |||
| Ok(lvol) => { | |||
| debug!("created lvol {:?}", lvol); | |||
| if let Some(v) = &args.entity_id { | |||
| let _ = lvol.set_blob_attr("entity_id", v.to_string(), true).await; | |||
There was a problem hiding this comment.
If set_blob_attr is failed, how control-plane will know about this error. I think error should be return back from dataplane which can be used in control-plane to mark the replica creation fail.
There was a problem hiding this comment.
Do we want to fail create_replica if set_xattr fails? Even if we return error anyway, Im not sure what control plane can do? Should it call set_replica_entity_id if error type matches?
We will call set_replica_entity_id grpc anyway incase entity_id is None while replica_share
There was a problem hiding this comment.
As We are sending back Option<entity_id> in the response, I think it is okay to mark the operation success but are We planning to do any action if entity_id is set to None in the create replica request or It will use the set_replica_entity_id grpc only during share replica flow ?
There was a problem hiding this comment.
I think we should fail the operation, because clearly the expectation that the replica has entity id is not met. What do you expect the control-plane to do?
| { | ||
| let lvol = Lvol::try_from(bdev)?; | ||
| let _ = lvol | ||
| .set_blob_attr("entity_id", args.entity_id, true) |
There was a problem hiding this comment.
Here also need to catch the error return from set_blob_attr function and return back proper error code to control-plane. Possibly a new error code can be introduced.
There was a problem hiding this comment.
It can hold off volume_publish call btw...
There was a problem hiding this comment.
Do We want to success the volume publish with this grpc failure ? Then how this attribute can be credible for the replica in control-plane, as in some scenario, volume can published without setting the attribute.
There was a problem hiding this comment.
As discussed, We are currently using entity_id for stats metadata. So better we handle error silently.
There was a problem hiding this comment.
Seems a bit to not fail here when the one outcome we want to from this is to set the entity id!??
abhilashshetty04
force-pushed
the
replica_entity_id
branch
from
62b00fd to
81b37f1
Compare
|
bors try |
tryBuild succeeded: |
Signed-off-by: Abhilash Shetty <abhilash.shetty@datacore.com>
abhilashshetty04
force-pushed
the
replica_entity_id
branch
from
81b37f1 to
866f3a8
Compare
|
bors merge |
|
Build succeeded: |
No description provided.