Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MTV-1211 Max concurrent virtual machine migrations #580

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RichardHoch
Copy link
Collaborator

@RichardHoch RichardHoch commented Oct 31, 2024

MTV 2.7

Resolves https://issues.redhat.com/browse/MTV-1211 by adding information about controller_max_vm_inflight (shown in the UI as Max concurrent virtual machine migrations).

Previews:

@nunzy1
Copy link

nunzy1 commented Oct 31, 2024

Your call as to what you incorporate. :-)

I might change:

"Maximum number of virtual machines (VMs) or disks per plan that can be migrated simultaneously."
To:
"Maximum number of virtual machines (VMs) or disks per plan that can migrate simultaneously."

(---unless “can be” means that you may decide not to and have that option)

"The maximum number of disks that can be transferred simultaneously."
To:
"The maximum number of disks that can transfer simultaneously."

"The maximum number of VMs that can be migrated simultaneously."
To:
"The maximum number of VMs that can migrate simultaneously."

"…can be transferred simultaneously. In these migrations, the disks are migrated in parallel. This means that if the combined number of disks that you want to migrate is greater than the value of the setting, additional disks wait until the queue is free without regard for whether a VM has completely finished being migrated."
To:
"…can be transferred simultaneously. In these migrations, the disks migrate in parallel. If the combined number of disks that you want to migrate is greater than the value of the setting, additional disks migrate only once the queue is free, without regard for whether a VM has finished migrating."

"Once any of them has migrated, the 16th disk can be migrated, whether or not all the disks on VM A and VM B have finished migrating."
To:
"Once any of them has migrated, the 16th disk migrates, whether or not all the disks on VM A and VM B have finished migrating."

(-- unless “can be migrated” means that it’s an option to migrate the disk)

@RichardHoch RichardHoch force-pushed the max_concur_vm branch 2 times, most recently from fd2de05 to c9a5e37 Compare November 3, 2024 14:18
@RichardHoch
Copy link
Collaborator Author

RichardHoch commented Nov 3, 2024

@mnecas @sgratch Please review this PR. Thanks.

@sgratch
Copy link
Contributor

sgratch commented Nov 6, 2024

@RichardHoch @mnecas
Isn't it worth renaming the parameter to reflect its meaning to something like:
Max concurrent virtual machine or disks migrations?

@RichardHoch
Copy link
Collaborator Author

RichardHoch commented Nov 7, 2024

@sgratch Changing the name of the parameter is up to @mnecas as far as I am concerned. But if we do it, I would suggest controller_max_vm_disk_inflight and Max concurrent virtual machine or disk migrations [drop the "s" in "disks" -- English is weird]. Does the PR look OK otherwise?

Copy link
Contributor

@sgratch sgratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except the comments below.
But let's wait for @mnecas final approval as well.

|The maximum number of VMs per plan that can be migrated simultaneously.
a|*Varies with provider as follows*:

* {rhv-full}, {osp}, VMware warm migrations _or_ remote cold migrations: The maximum number of disks that {project-short} can transfer simultaneously.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all occurrences of remote cold migrations:
I suggest to explain what does "remote migration" mean and therefore rephrase to something like:
cold migrations to a remote OpenShift cluster


* {rhv-full}, {osp}, VMware warm migrations _or_ remote cold migrations: The maximum number of disks that {project-short} can transfer simultaneously.

* OVA or VMware local cold migrations: The maximum number of VMs that {project-short} can migrate simultaneously.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all occurrences of local cold migrations:
I suggest to explain what does "local migration" mean and therefore rephrase to something like:
cold migrations to a local OpenShift cluster

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not apply for all local cold migrations but afaik only for virt-v2v related migrations.

documentation/modules/max-concurrent-vms.adoc Outdated Show resolved Hide resolved
@sgratch
Copy link
Contributor

sgratch commented Nov 11, 2024

Changing the name of the parameter is up to @mnecas as far as I am concerned. But if we do it, I would suggest controller_max_vm_disk_inflight and Max concurrent virtual machine or disk migrations [drop the "s" in "disks" -- English is weird].

Sounds good to me to change it. @mnecas ?

@mnecas
Copy link
Member

mnecas commented Nov 14, 2024

Changing the name of the parameter is up to @mnecas as far as I am concerned. But if we do it, I would suggest controller_max_vm_disk_inflight and Max concurrent virtual machine or disk migrations [drop the "s" in "disks" -- English is weird].

Sounds good to me to change it. @mnecas ?

I am a bit afraid that we would lose the previous settings during the upgrade.
I don't want to change it to the "or" as we might even change the logic to always transfer disks.

@sgratch
Copy link
Contributor

sgratch commented Nov 14, 2024

Changing the name of the parameter is up to @mnecas as far as I am concerned. But if we do it, I would suggest controller_max_vm_disk_inflight and Max concurrent virtual machine or disk migrations [drop the "s" in "disks" -- English is weird].

Sounds good to me to change it. @mnecas ?

I am a bit afraid that we would lose the previous settings during the upgrade. I don't want to change it to the "or" as we might even change the logic to always transfer disks.

OK, so let's leave it as is for now.

@RichardHoch
Copy link
Collaborator Author

@sgratch @mnecas I made the change Sharon suggested. Please review this PR again. Thanks.

@RichardHoch RichardHoch changed the title Max concurrent virtual machine migrations MTV-1211 Max concurrent virtual machine migrations Nov 24, 2024
@sgratch
Copy link
Contributor

sgratch commented Nov 26, 2024

@RichardHoch did you miss those 2 comments ? :)
#580 (comment)
#580 (comment),

@RichardHoch
Copy link
Collaborator Author

RichardHoch commented Nov 28, 2024

@sgratch @mnecas I made some more changes to the PR. Please review them. Thanks.
@sgratch I think I included both your changes -- let me know if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants