Migration followup by EbiRider · Pull Request #1135 · simplyblock/sbcli

EbiRider · 2026-06-25T11:44:38Z

in this follow up we addressing multiple issues for migration

the double m suffix issue from back to back migrations which made subsequent migrations fail
tertiary node not being registered on the target's tertiary node
cleanup of the code for unused variables and parameters
extracting hublvol control logic out of the migration flow to ensure detach and attach of the hublvol respect it's needed cooldown of ten seconds

mxsrc

I'm a bit lost with the PR, the description is not really enough for me to understand what is going on. Ideally we'd split the PR up into indivudal changes, not sure whether that's feasible. Currently the description says the PR does four things, but there is definitely more going on. Questions I have from a quick glance that I can't really answer, that don't seem to correspond to any of the items the description indicates:

What are the test scripts that are being introduced for, who is supposed to execute them?
What's the purpose of the fault injection logic, where is it used from?
Why does the PR introduce replication logic, my understanding is that it's supposed to fix volume migration

Please help me review this, as it stands I don't understand what is going on.

mxsrc · 2026-07-07T15:41:15Z

+    # Undocumented chaos-testing switch: when set at cluster create, a transparent
+    # SPDK delay bdev is inserted between each device's nvme bdev and its alceml so
+    # a device can be made to "hang" on demand (see device_controller device-hang).
+    enable_hang_device: bool = False


I think we need to very carefully think about the implications of integrating fault injection into the business logic. Where is this called from? I cannot find it in this PR. Is there no other way of achieving this?

mxsrc · 2026-07-07T15:49:18Z

+    _lvol_base = lvol.lvol_bdev[:-len(_MIGRATION_BDEV_SUFFIX)] if lvol.lvol_bdev.endswith(_MIGRATION_BDEV_SUFFIX) else lvol.lvol_bdev
+    bdev_short = _lvol_base + _MIGRATION_BDEV_SUFFIX


This is not the only place where this is failing. A proper fix uses the idempotent helper that is already present consistently: 0a9690a.

mxsrc · 2026-07-07T15:49:45Z

+    _lvol_base = lvol.lvol_bdev[:-len(_MIGRATION_BDEV_SUFFIX)] if lvol.lvol_bdev.endswith(_MIGRATION_BDEV_SUFFIX) else lvol.lvol_bdev
+    bdev_short = _lvol_base + _MIGRATION_BDEV_SUFFIX


This is too easy to miss, same comment as above. If you repeat code, use a helper.

…sful transfer

…revious migrations

…igration

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

mxsrc

There's a few minor things that should change, I left comments for that. The two larger things are that quite a few places ignore errors instead of escalating them, sometimes logging a warning, sometimes not. In some places this doesn't seem right, I think this should be revisited. As for the core logic, I can't really judge that, this, and the pre-existing code doesn't really document the intent, but from a code-perspective I think we can go forward.

mxsrc · 2026-07-08T14:26:28Z


+    def bdev_lvol_rename(self, old_name, new_name):
+        params = {"old_name": old_name, "new_name": new_name}
+        return self._request("bdev_lvol_rename", params)


New RPCs should use _request3.

done, using _request3 for the rpc now

mxsrc · 2026-07-08T14:33:19Z

-            current = snap.snap_ref_id
+            current = snap.snap_ref_id or snap.prev_snap_uuid
        except KeyError:
            break


This, and _get_snap_ancestry seem redundant. Is there no way of deduplicating them?
At least _protect_snap_and_ancestors can basically be candidate_set - _collect_snap_ancestry(), can't it?

mxsrc · 2026-07-08T14:35:40Z

        status = primary_rpc.bdev_lvol_get_lvol_delete_status(bdev_name)
-        if status in (0, 2):
-            break
-        if status == 1:
-            time.sleep(0.25)
-        else:
-            logger.warning(f"delete bdev {bdev_name}: unexpected status {status}")
+        if status != 1:


This can be folded into the loop condition.

mxsrc

Gotta look into something else and didn't finish the review, leaving my comments here for now. Something I noticed, not necessarily for now, but a common pattern is to do an operation on a node, as well as their secondary and primary. One single helper would help doing this correctly, ensuring correct error handling.

mxsrc · 2026-07-09T13:02:17Z

+            _short_tgt = _snap_tgt_short_name(_s)          # SNAP_Xm  (in-flight)
+            _short_canonical = _snap_short_name(_s)         # SNAP_X   (post-rename)
+            _short_am = _short_canonical + _MIGRATION_BDEV_SUFFIX_DONE  # SNAP_Xam (fallback)


This will hit the same fault scenario we had with the BDEV_SUFFIX, won't it? If the m suffix is already applied, simply adding this will make it SNAP_Xmam?

after migration we have a rename operation, which should bring the lvols back to the same name they had on source on the target unless there's a different lvol with exact same name so I'm am suffix for those cases
that only happnes if there's an unrelated lvol with the same lvol on the target

mxsrc · 2026-07-09T13:08:19Z

+                        f"_rename_migrated_bdevs: {role} rename {label} (non-fatal): {exc}")
+        if exists:
+            return _EXISTS
+        return True


I'm struggling to follow this function. It returns either a boolean (or rather, always True), or some string sentinel. It also adds relevant logic to RPCClient.bdev_lvol_rename. The comments are a bit unclear to me and violate abstraction layers, the RPCClient should contain any knowledge about RPC workings, like the mapping of error codes.

So the suggestion is to add an exists_ok flag to RPCClient.bdev_lvol_rename, and raise an RPCException subclass that indicates the entity already exists. With that primitive, we can lose the sentinel, and _rename_with_fallback becomes much more readable.

In addition to handling the RPC logic, it does the rename on all three nodes that are responsible for the LVol. It doesn't handle inconsistencies or failures correctly though open questions are:

what happens if the function encounters an existing inconsistency where the target exists on only one of the nodes. There is no way to handle this easily, but at least we need to be aware of it

Errors of any of the rename (except the first) are ignored and flagged as non-fatal. Is that correct? It introduces an inconsistency that follow up code has to deal with.

this i been working on mainly on migration-followup2 branch which it changed I'll save this comment for that branch

mxsrc · 2026-07-09T13:10:43Z

+    """
+    if _SKIP_RENAME_BDEVS:
+        logger.info("_rename_migrated_bdevs: skipped (SKIP_RENAME_BDEVS=True)")
+        return


This seems like a debugging provision? In that case I prefer not to mainline it.

mxsrc · 2026-07-09T13:52:05Z

+def _protect_snap_and_ancestors(snap_uuid, candidate_set):
+    """Remove *snap_uuid* and all its ancestors from *candidate_set*."""
+    candidate_set -= _collect_snap_ancestry(snap_uuid)


At this point we don't need the helper anymore, the operation can simply be used at the call-site.

mxsrc

Thanks for the changes, quite a few good additions in here. I think we need one more iteration though.

The situation with the _get_target_{secondary,tertiary}_node should be handled by:

adding documentation for why in some cases a suspended node should be returned
passing the source node ID parameter consistently
removing the default to enforce this for future changes
reviewing the _get_{secondary,tertiary}_rpc helpers: This needs to mirror the logic for the node get helpers, keeping this in sync is evidently hard. Maybe they should be removed, and instead use _make_rpc(_get_{secondary_target_tertiary_node(...)). If they are used for source nodes as well, probably we best introduce separate helpers for that indicating them.

mxsrc · 2026-07-09T14:27:25Z

+        if (node_offline_devices > 0
+                or (node_online_devices == 0 and node.status != StorageNode.STATUS_REMOVED)
+                or node.status == StorageNode.STATUS_OFFLINE):


This doesn't seem to be related to the other changes, why is it here?

# Conflicts: # simplyblock_core/controllers/migration_controller.py # simplyblock_core/services/tasks_runner_lvol_migration.py

mxsrc

Looks good, let's go forward. Some of the suggestions are still valid, but they shouldn't block merging this.

github-code-quality Bot found potential problems Jun 25, 2026

View reviewed changes

Comment thread simplyblock_core/services/hub_controller_manager.py Fixed

Comment thread simplyblock_core/services/hub_controller_manager.py Fixed

Comment thread simplyblock_core/services/hub_controller_manager.py Fixed

github-code-quality Bot found potential problems Jun 29, 2026

View reviewed changes

Comment thread simplyblock_core/services/tasks_runner_lvol_migration.py Fixed

github-code-quality Bot found potential problems Jun 30, 2026

View reviewed changes

Comment thread simplyblock_core/services/tasks_runner_lvol_migration.py Fixed

EbiRider force-pushed the migration-followup branch from d2676d7 to 77b6059 Compare June 30, 2026 20:43

github-code-quality Bot found potential problems Jul 4, 2026

View reviewed changes

Comment thread simplyblock_core/services/tasks_runner_lvol_migration.py Fixed

mxsrc reviewed Jul 7, 2026

View reviewed changes

EbiRider added 24 commits July 7, 2026 23:00

enabling the hublvol detch on cleanup source

8415629

adding a hublvol controller to control detch behaviour

5d53b47

fixing any issue with back to back migrations

741d056

fixing an issue with tertiary nodes not being registered on the target

a13a1b2

fixed an issue where the migration will report failure desptie succes…

e67146b

…sful transfer

fixing mypy type checker issue for constants

b87e39e

prevent the migration from trying to delete snapshots migrated from p…

79cbe59

…revious migrations

the pre-scan now checks for is_snapshot field

64934f8

removing the logic to generate a snapshot if the source lvol has none

735c420

remvoing forced snapshot creation on start_migration

6c92126

fixing an issue with namespace not being added for tertiary node

696c79a

followup for fixing tertiary ns add issue

4746b1b

fixing the delete logic on the migration

6651352

removed setting migration flag for snapshots to be deleted

6d53c5e

TEMP: testing snapshot resume on retry instead of delete

589b9a2

TEMP: disabling the cleanup from source and intermediates

1302994

TEMP: disabling the cleanup source

6ecbcec

setting the spdk image ref for debugging

fb9c754

fixing the image ref for spdk

05c0e7b

fixing the image ref for spdk again

4231193

setting the env_var to main

9b06344

removing the status cutover as it's no longer needed

53ac4d9

fixing ndcs and npcs

93d7450

setting env_var for spdk image

fe73f8a

EbiRider and others added 17 commits July 7, 2026 23:00

fixing an issue where ancestor snapshots would not be renamed after m…

f775a59

…igration

disabling source cleanup for testing

7ef1a90

enabling cleanup and disabling rename for now

7e16f1e

enabling lvol rename

e4072df

changing the comments after fact checking about rename

d24b0ed

fixing an issue with namespace not being added for tertiary node

c317b76

followup for fixing tertiary ns add issue

d023ee8

TEMP: testing snapshot resume on retry instead of delete

f1a2b07

TEMP: disabling the cleanup from source and intermediates

6f9d2d1

setting the env_var to main

c0eb29f

setting env_var for spdk image

7da99e2

fixing an issue where ancestor snapshots would not be renamed after m…

2effec6

…igration

enabling lvol rename

b56c429

Apply suggestions from code review

f0cec34

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

fixing linting errors

94532a6

fixed an issue that made unit test fail

c99d4f7

setting correct image refernces for env var

e9941c7

EbiRider force-pushed the migration-followup branch from 526b846 to e9941c7 Compare July 7, 2026 19:48

mxsrc requested changes Jul 8, 2026

View reviewed changes

EbiRider added 2 commits July 9, 2026 05:32

using request3 for new rpc call

164e5d6

fix: escalate migration errors and deduplicate ancestry helpers

88bda5b

EbiRider requested a review from mxsrc July 9, 2026 02:10

mxsrc reviewed Jul 9, 2026

View reviewed changes

mxsrc requested changes Jul 9, 2026

View reviewed changes

EbiRider added 3 commits July 10, 2026 15:18

fix: dead-code cleanup, exception-safety fixes, stale docs

bc45556

Merge branch 'main' into migration-followup

74d24b4

# Conflicts: # simplyblock_core/controllers/migration_controller.py # simplyblock_core/services/tasks_runner_lvol_migration.py

fixing a linting error

4d13362

mxsrc approved these changes Jul 10, 2026

View reviewed changes

fix: dedupe migration-target bdev naming into a shared idempotent helper

9a4f7fc

		_lvol_base = lvol.lvol_bdev[:-len(_MIGRATION_BDEV_SUFFIX)] if lvol.lvol_bdev.endswith(_MIGRATION_BDEV_SUFFIX) else lvol.lvol_bdev
		bdev_short = _lvol_base + _MIGRATION_BDEV_SUFFIX

Uh oh!

Conversation

EbiRider commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxsrc left a comment

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!

mxsrc left a comment

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!

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!

mxsrc left a comment

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!

mxsrc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxsrc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants