feat(persistence): update replica health condition as atomic operation #1879

dsharma-dc · 2025-06-20T04:23:31Z

Fail the transaction operation and shutdown the nexus to avoid succeeding IO via remaining replicas. This ensures data integrity in case the replica being marked here has been picked up as source of truth elsewhere for the volume.

dsharma-dc · 2025-06-20T06:06:22Z

bors try

bors-openebs-mayastor · 2025-06-20T06:44:07Z

try

Build succeeded:

bors-ci

io-engine/src/bdev/nexus/nexus_bdev_error.rs

tiagolobocastro · 2025-06-20T09:08:12Z

io-engine/src/bdev/nexus/nexus_persistence.rs

    /// Information about children.
    pub children: Vec<ChildInfo>,
 }

+impl Display for NexusInfo {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{{children: [")?;


Could you just call the debug impl here?

Could you just call the debug impl here?

Sure, will remove this, was using during initial testing.

io-engine/src/bdev/nexus/nexus_persistence.rs

tiagolobocastro · 2025-06-20T09:18:01Z

io-engine/src/bdev/nexus/nexus_persistence.rs

                nexus_info.children.iter_mut().for_each(|c| {
                    if c.uuid == uuid {
                        c.healthy = *healthy;
                    }
                });
+
+                let mut txn = NexusInfoTxn {
+                    key_info: &mut persistent_nexus_info,


do we need this cross call mutation?

mutating this in the failure case to set the new flag's value in the in-core object key_info here, just in case required to debug.

io-engine/src/bdev/nexus/nexus_persistence.rs

dsharma-dc · 2025-06-23T04:57:03Z

bors try

bors-openebs-mayastor · 2025-06-23T05:38:25Z

try

Build succeeded:

bors-ci

dsharma-dc · 2025-06-24T05:57:18Z

bors try

bors-openebs-mayastor · 2025-06-24T06:38:44Z

try

Build succeeded:

bors-ci

Fail the transaction operation and shutdown the nexus to avoid succeeding IO via remaining replicas. This ensures data integrity in case the replica being marked here has been picked up as source of truth elsewhere for this volume. Signed-off-by: Diwakar Sharma <[email protected]>

dsharma-dc · 2025-07-15T18:09:51Z

bors try

bors-openebs-mayastor · 2025-07-15T18:46:35Z

try

Build succeeded:

bors-ci

Abhinandan-Purkait · 2025-07-16T08:08:29Z

io-engine/src/bdev/nexus/nexus_persistence.rs

+//impl Display for NexusInfo {
+//    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+//        write!(f, "{{children: [")?;
+//
+//        for (i, cinfo) in self.children.iter().enumerate() {
+//            if i != 0 {
+//                write!(f, ", ")?;
+//            }
+//            write!(f, "uuid: {}, healthy: {}", cinfo.uuid, cinfo.healthy)?;
+//        }
+//        write!(
+//            f,
+//            "], clean_shutdown: {}, do_self_shutdown: {}}}",
+//            self.clean_shutdown, self.do_self_shutdown
+//        )
+//    }
+//}


Do we need this?

tiagolobocastro · 2025-07-16T09:05:11Z

io-engine/src/store/etcd.rs

+            })?;
+
+        if !txn_resp.succeeded() {
+            if let TxnOpResponse::Get(g) = &txn_resp.op_responses()[0] {


Need to check if len > 1?

tiagolobocastro · 2025-07-16T09:05:33Z

io-engine/src/store/etcd.rs

+
+        if !txn_resp.succeeded() {
+            if let TxnOpResponse::Get(g) = &txn_resp.op_responses()[0] {
+                if let Some(kv) = g.kvs().first() {


better to take here rather copying the value on return

tiagolobocastro · 2025-07-16T09:08:01Z

io-engine/src/store/etcd.rs

+                // This is needed because absence of above response data shouldn't get
+                // implicitly assumed as success. With this the top level caller can
+                // compare this returned blank value with some expected value.
+                return Ok(Some(vec![]));


Shouldn't we return a StoreError here as this seems like an etcd bug if we hit this?

tiagolobocastro · 2025-07-16T09:09:55Z

io-engine/src/bdev/nexus/nexus_persistence.rs

+                    if let Some(current_value) = txn_resp {
+                        let val = serde_json::from_slice::<NexusInfo>(&current_value).unwrap();
+
+                        warn!("current state found: key - {key}, value - {val:?}");


move this to after the if statement?

tiagolobocastro · 2025-07-16T09:16:27Z

io-engine/src/bdev/nexus/nexus_persistence.rs

+                            name: self.name.clone(),
+                        });
+                    }
+                    error!("{self:?}: failed to persist nexus info transaction: {err}");


Use the same log once strategy we have in save

tiagolobocastro · 2025-07-16T09:16:47Z

io-engine/src/bdev/nexus/nexus_persistence.rs

+                        });
+                    } else {
+                        // Don't need to check individual op responses.
+                        debug!(?key, "{self:?}: state save transaction successful");


same as save

Suggested change

debug!(?key, "{self:?}: state save transaction successful");

trace!(?key, "{self:?}: the state was saved successfully");

tiagolobocastro · 2025-07-16T09:22:44Z

io-engine/src/store/store_defs.rs

@@ -1,7 +1,7 @@
 //! Definition of a trait for a key-value store together with its error codes.

 use async_trait::async_trait;
-use etcd_client::{Compare, Error, TxnOp, TxnResponse};
+use etcd_client::Error;


Not needed now I hope?

tiagolobocastro · 2025-07-16T09:23:54Z

io-engine/src/persistent_store.rs

-        ops_success: Vec<TxnOp>,
-        ops_failure: Option<Vec<TxnOp>>,
-    ) -> Result<TxnResponse, StoreError> {
+        new_value: &[u8],


These should be &impl StoreValue ?

tiagolobocastro · 2025-07-16T09:24:32Z

io-engine/src/bdev/nexus/nexus_persistence.rs

+pub struct NexusInfoTxn<'a> {
+    pub key_info: &'a mut PersistentNexusInfo,
+    // Expected value for the key.
+    pub expected: NexusInfo,
+}


Do any of these need to be pub?

dsharma-dc requested a review from a team as a code owner June 20, 2025 04:23

auto-assign bot requested review from Abhinandan-Purkait and tiagolobocastro June 20, 2025 04:23

dsharma-dc requested a review from abhilashshetty04 June 20, 2025 04:23

bors-openebs-mayastor bot pushed a commit that referenced this pull request Jun 20, 2025

Try #1879:

90386b3

tiagolobocastro reviewed Jun 20, 2025

View reviewed changes

dsharma-dc force-pushed the nexus_do_shutdown branch from c85178e to c9a0c46 Compare June 23, 2025 04:56

bors-openebs-mayastor bot pushed a commit that referenced this pull request Jun 23, 2025

Try #1879:

3afbfc6

dsharma-dc force-pushed the nexus_do_shutdown branch from c9a0c46 to c436075 Compare June 23, 2025 13:16

bors-openebs-mayastor bot pushed a commit that referenced this pull request Jun 24, 2025

Try #1879:

b5b011c

dsharma-dc force-pushed the nexus_do_shutdown branch from c436075 to f20e613 Compare July 15, 2025 18:07

bors-openebs-mayastor bot pushed a commit that referenced this pull request Jul 15, 2025

Try #1879:

d433638

dsharma-dc requested a review from tiagolobocastro July 16, 2025 04:51

Abhinandan-Purkait approved these changes Jul 16, 2025

View reviewed changes

tiagolobocastro reviewed Jul 16, 2025

View reviewed changes

	debug!(?key, "{self:?}: state save transaction successful");
	trace!(?key, "{self:?}: the state was saved successfully");

feat(persistence): update replica health condition as atomic operation #1879

Are you sure you want to change the base?

feat(persistence): update replica health condition as atomic operation #1879

Uh oh!

Conversation

dsharma-dc commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsharma-dc commented Jun 20, 2025

Uh oh!

bors-openebs-mayastor bot commented Jun 20, 2025

try

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dsharma-dc commented Jun 23, 2025

Uh oh!

bors-openebs-mayastor bot commented Jun 23, 2025

try

Uh oh!

dsharma-dc commented Jun 24, 2025

Uh oh!

bors-openebs-mayastor bot commented Jun 24, 2025

try

Uh oh!

dsharma-dc commented Jul 15, 2025

Uh oh!

bors-openebs-mayastor bot commented Jul 15, 2025

try

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsharma-dc commented Jun 20, 2025 •

edited

Loading