batch migration web api endpoints

[EbiRider]

this PR includes changes to make the web api structure consistent with the cli approach
at some places batch migration was named "group" renamed those occurrences to batch to avoiud confusion

another fix on this PR is that lvol migration now honors the max namespaced assigned to the migrated lvol where as before it was always set to 32 regardless of migrated lvol assigned number this fix takes effect on both the batch migration and single lvol migration

[noctarius]

I think it's good from my perspective. @mxsrc can you have a quick look?

[Hamdy-khader]

Looks good in general, please fix the failing Unit tests

[mxsrc]

Can you elaborate a bit on the batch concept? My understanding would be that a batch migration migrates multiple volumes at once. This PR introduced them as volume specific, and when e.g. creating one exactly one volume is transferred. What's the difference to regular migrations?

The changes duplicate lookup of migrations in each of the introduced endpoints, this is something that we do via dependencies. The PR seems to revert that, not sure what the motivation is for that.

[noctarius]

Any volume in the subsystem can be used to kick off the migration for all of them. I don't think having a separate API call for something that can be expressed as a parameter is more meaningful. Theoretically, you wouldn't even need the parameter, but if you know that the migration will a be a non-batch, it can a little bit more lightweight.

I would see it as a "migrate this volume". That others are migrated as part of it is a detail. It's not possible to group or ungroup them right now. I think, in the future, we may have a subsystem resource and can move the whole migration into a subsystem which would make more sense in general.

[EbiRider]

Can you elaborate a bit on the batch concept? My understanding would be that a batch migration migrates multiple volumes at once. This PR introduced them as volume specific, and when e.g. creating one exactly one volume is transferred. What's the difference to regular migrations?

The changes duplicate lookup of migrations in each of the introduced endpoints, this is something that we do via dependencies. The PR seems to revert that, not sure what the motivation is for that.

i understand the confusion since it's named batch migration, it doesn't mean it take a list of lvol to migrate, it migrates every lvol under a namespaced subsystem it automatically picks up on the siblings
creating a master process that oversee individual lvol migrations syncing them at key points so the overall operation performs correctly now we didn't exactly need the batch query here it's just to let the user know that they are initiating a batch migration, because when you call lvol migrate and it happen to be a part of subsystem you'll migrate all of them and that might not be the intention here
so to make it clear i added this flag calling lvol migration on a lvol in a shared subsystem will not work unless batch prop is given
and for the lookups it's calling a different entity MigrationGroup it just holds the ids to the lvol migrations related to one job of batch migration

[EbiRider]

I'm adding two changes to this PR

1. a guard against an lvol joining a subsystem while it has a migration on it
2. adding a timeout to the state of pre_create for lvol migration before canceling it to make sure no migration stay in that state for long and interfere with cluster operations

[mxsrc]

i understand the confusion since it's named batch migration, it doesn't mean it take a list of lvol to migrate, it migrates every lvol under a namespaced subsystem it automatically picks up on the siblings creating a master process that oversee individual lvol migrations syncing them at key points so the overall operation performs correctly now we didn't exactly need the batch query here it's just to let the user know that they are initiating a batch migration, because when you call lvol migrate and it happen to be a part of subsystem you'll migrate all of them and that might not be the intention here so to make it clear i added this flag calling lvol migration on a lvol in a shared subsystem will not work unless batch prop is given and for the lookups it's calling a different entity MigrationGroup it just holds the ids to the lvol migrations related to one job of batch migration

I think in that case the API needs to be split: the existing migration API is lvol-specific, it migrates the LVol identified by the hierarchy. The batch API affects multiple volumes, so it needs to be present under the cluster (or storage pool if they always belong to the same one). The endpoints could be something like /clusters/{cluster_id}/subsystems/{identifier}/migrate. I think the existing structure is confusing to the user.

Looking at the code this should be relatively straight forward, the implementations seem to have two completely independent paths gated by the batch parameters.

[EbiRider]

i understand the confusion since it's named batch migration, it doesn't mean it take a list of lvol to migrate, it migrates every lvol under a namespaced subsystem it automatically picks up on the siblings creating a master process that oversee individual lvol migrations syncing them at key points so the overall operation performs correctly now we didn't exactly need the batch query here it's just to let the user know that they are initiating a batch migration, because when you call lvol migrate and it happen to be a part of subsystem you'll migrate all of them and that might not be the intention here so to make it clear i added this flag calling lvol migration on a lvol in a shared subsystem will not work unless batch prop is given and for the lookups it's calling a different entity MigrationGroup it just holds the ids to the lvol migrations related to one job of batch migration

I think in that case the API needs to be split: the existing migration API is lvol-specific, it migrates the LVol identified by the hierarchy. The batch API affects multiple volumes, so it needs to be present under the cluster (or storage pool if they always belong to the same one). The endpoints could be something like /clusters/{cluster_id}/subsystems/{identifier}/migrate. I think the existing structure is confusing to the user.

Looking at the code this should be relatively straight forward, the implementations seem to have two completely independent paths gated by the batch parameters.

hmm fair point, @noctarius are you oki with me reverting it back to split api endpoints?

[noctarius]

i understand the confusion since it's named batch migration, it doesn't mean it take a list of lvol to migrate, it migrates every lvol under a namespaced subsystem it automatically picks up on the siblings creating a master process that oversee individual lvol migrations syncing them at key points so the overall operation performs correctly now we didn't exactly need the batch query here it's just to let the user know that they are initiating a batch migration, because when you call lvol migrate and it happen to be a part of subsystem you'll migrate all of them and that might not be the intention here so to make it clear i added this flag calling lvol migration on a lvol in a shared subsystem will not work unless batch prop is given and for the lookups it's calling a different entity MigrationGroup it just holds the ids to the lvol migrations related to one job of batch migration

I think in that case the API needs to be split: the existing migration API is lvol-specific, it migrates the LVol identified by the hierarchy. The batch API affects multiple volumes, so it needs to be present under the cluster (or storage pool if they always belong to the same one). The endpoints could be something like /clusters/{cluster_id}/subsystems/{identifier}/migrate. I think the existing structure is confusing to the user.

Looking at the code this should be relatively straight forward, the implementations seem to have two completely independent paths gated by the batch parameters.

I agree it is confusing, but I think it's even worse underneath cluster or storage pool. At the moment, we don't have the subsystem concept. That other volumes are moved as part of this one specific volume should be an ugly implementation detail right now. It is what we have for now, but this should be fixed in the future. In general, the whole api was created to assume that the volume is migrated by itself which now falls apart.

Moving it to /subsystems/{identifier} is the perfectly right move, for both cases, because even when a volume is not namespaced, it's just a special case of a single-volume subsystem. Meaning, we're always moving subsystems, never actual volumes (technically). It would also open the option to add functionality to attach/detach volumes from a subsystem later on.

If we find a way to cleanly identify a "subsystem" without the object in the backend, I'm all for this change. Breaking it apart, if we don't have a subsystem id, I'd prefer to not do this anymore. It'll have a code branch in the frontend which is absolutely unnecessary that late in the release process.

[mxsrc]

I agree it is confusing, but I think it's even worse underneath cluster or storage pool. At the moment, we don't have the subsystem concept. That other volumes are moved as part of this one specific volume should be an ugly implementation detail right now. It is what we have for now, but this should be fixed in the future. In general, the whole api was created to assume that the volume is migrated by itself which now falls apart.

Moving it to /subsystems/{identifier} is the perfectly right move, for both cases, because even when a volume is not namespaced, it's just a special case of a single-volume subsystem. Meaning, we're always moving subsystems, never actual volumes (technically). It would also open the option to add functionality to attach/detach volumes from a subsystem later on.

If we find a way to cleanly identify a "subsystem" without the object in the backend, I'm all for this change. Breaking it apart, if we don't have a subsystem id, I'd prefer to not do this anymore. It'll have a code branch in the frontend which is absolutely unnecessary that late in the release process.

The subsystem is identified by its NQN. That's a bit cumbersome to pass along, but the NQN is f"{cluster.nqn}:lvol:{lvol.uuid}", of some LVol that is. We can probably figure out a way to remove the prefix, but probably it's easier to just use as-is, I bet there is some inconsistency in the code base that breaks deriving it.

I get the "late in the release" argument, but I'd really like to design a clean API since that will be difficult to change.

[noctarius]

I agree it is confusing, but I think it's even worse underneath cluster or storage pool. At the moment, we don't have the subsystem concept. That other volumes are moved as part of this one specific volume should be an ugly implementation detail right now. It is what we have for now, but this should be fixed in the future. In general, the whole api was created to assume that the volume is migrated by itself which now falls apart.
Moving it to /subsystems/{identifier} is the perfectly right move, for both cases, because even when a volume is not namespaced, it's just a special case of a single-volume subsystem. Meaning, we're always moving subsystems, never actual volumes (technically). It would also open the option to add functionality to attach/detach volumes from a subsystem later on.
If we find a way to cleanly identify a "subsystem" without the object in the backend, I'm all for this change. Breaking it apart, if we don't have a subsystem id, I'd prefer to not do this anymore. It'll have a code branch in the frontend which is absolutely unnecessary that late in the release process.

The subsystem is identified by its NQN. That's a bit cumbersome to pass along, but the NQN is f"{cluster.nqn}:lvol:{lvol.uuid}", of some LVol that is. We can probably figure out a way to remove the prefix, but probably it's easier to just use as-is, I bet there is some inconsistency in the code base that breaks deriving it.

I get the "late in the release" argument, but I'd really like to design a clean API since that will be difficult to change.

I like that. Yes, a nicer ID would be better (do we need to encode the @ sign?) but it'll work.

[mxsrc]

Yeah, I think we should be good on the encoding, the only potential issue I see is with relative paths erroneously being parsed as absolute due to the domain. To me that's an acceptable risk though.

[noctarius]

I think we'd be safe to use the last segment (uuid) from the NQN since it shouldn't change but I'm fine using the full NQN. @EbiRider would you be able to find the volumes / subsystem by the NQN?

[mxsrc]

My thinking was that the current NQN includes the lvol-derived ID. In the future we might want to migrate this to something else, s.t. subsystems get proper identiities of their own.

[EbiRider]

HubControllerManager cross-process fix: TasksRunnerLVolMigration and TasksRunnerBatchMigration run as separate processes, each previously holding its own in-memory copy of the 10s detach→attach cooldown via a module-level singleton — meaning the cooldown wasn't actually enforced across both processes. Replaced the singleton with explicit per-process instantiation (HubControllerManager(db)), and moved the cooldown timestamp into a DB-backed record (models/hub_cooldown.py, HubDetachCooldown) keyed by the (src_node_id, tgt_node_id) pair, so the cooldown is honored regardless of which process performed the detach.

[EbiRider]

last commit fixes a lint issue that appeared after rebase, the change on the rpc client is unrelated to this branch but fixing it was required