-
Notifications
You must be signed in to change notification settings - Fork 88
Add details about non-migratable VMs #849
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
Conversation
dba517a
to
97bcd4b
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some nits, overall LGTM!
7c1b590
to
91034b9
Compare
To reviewers: As the issue harvester/harvester#8823 mentioned, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good content. I think, if possible, we should concentrate the info on the upgrade and live migration pages. Specifically, how upgrade can potentially disrupt some workloads, what Harvester is doing to help mitigate these disruptions and if things go wrong, what can the users do.
Right now, I am not sure if we need to repeat live migration info in the vm, volume, storage class pages. I'll need to re-read it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@w13915984028, The picture link seems wrong, I saw below error:
Error: Image static/img/v1.6/vm/batch-migrations.png used in docs/vm/live-migration.md not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review done
d66a3b3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - a few suggestions and nits to help clarify the context and intention.
@ihcsim I try to breakdown the 4 migrations on v1.6.0: (1) Migrate menu: straightforward, the reverse operation is some users are keen to know those details, the doc helps us from explaining it now and then; and even Harvester engineers can benefit from those details. (2)(3)(4) need improvements more than harvester/harvester#8924 and this doc PR (it also can't cover all cases atm. like this open question #849 (comment)) suppose there is a confirm pop-up window when user clicks Abort Migration saying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second review done.
@ihcsim I agree that we are getting into the weeds here. If folks decide that such info must be added to the doc, we can store the info in pages dedicated to reference content. Users shouldn't be forced to wade through implementation details when they're simply trying to complete tasks. This approach will also help us tame the out-of-control linking that makes the doc difficult to maintain.
|
||
:::tip | ||
Create a backup or snapshot for each non-migratable VM before modifying the settings that bind it to the node that you want to remove. | ||
- Create a backup or snapshot for each non-migratable virtual machine. | ||
- Change the virtual machines to let them run on other nodes as more as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any missing words here? What do you mean by 'change the virtual machines'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'change the virtual machines'
Yes, it is not fully expressed, as non-migratable vm
mentioned:
User can try to remove the node specific volumes/devices, remove the node selector ... to finally let vm be scheduled to other nodes if possible. As current node is to be deleted, the un-saved/un-migrated VM will be lost.
docs/vm/live-migration.md
Outdated
- The **Abort Migration** menu item is available when the virtual machine already has a running or pending migration process. | ||
|
||
- Don't click `Abort Migration` if it is created by the [batch-migrations](#automatically-triggered-batch-migrations). See [The `VirtualMachineInstanceMigration` Object](#the-virtualmachineinstancemigration-object) for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The **Abort Migration** menu item is available when the virtual machine already has a running or pending migration process. | |
- Don't click `Abort Migration` if it is created by the [batch-migrations](#automatically-triggered-batch-migrations). See [The `VirtualMachineInstanceMigration` Object](#the-virtualmachineinstancemigration-object) for more details. | |
The **Abort Migration** menu item is already running or has a pending migration process. | |
Do not use this UI feature if the migration process was created using [batch migration](#automatically-triggered-batch-migration). For more information, see [`VirtualMachineInstanceMigration` Object](#virtualmachineinstancemigration-object). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Abort Migration menu item is already running or has a pending migration process.
This description is not accurate, it needs to express below information:
When the VM has already a related virtualmachineinstancemigration object (created by Migrate menu or batch-migration or others) and the latter is in state running
or pending
(it has other state like failed
, successful
, aborted
), then the VM has the menu Abort Migration.
Sorry I can't agree with the point that the document is over complexed. In the long time of customer help, troubleshooting, community coordinator..., what I heared is that the document is missing details, out-dated or even wrong, but very rare compalins saying the document is too complex. |
@votdev You will be online next week, please also help review and check if the PR meets the expectations on issue harvester/harvester#8823, thanks. |
How VM is marked as non-migratable How VM is processed on upgrade process How VM is processed on node maintenance process How `migrate` menu comes Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
Signed-off-by: Jian Wang <[email protected]>
FYI: |
@w13915984028 I think the word "bloated" is more aligned with what Jillian and I are trying to communicate about the "How Migration Works" section. It's not "complex" especially, after we spent the time to help you to clarify the wording to express your intention in the doc. We think that when a user reads the "Live Migration" documentation, their immediate goal is to use live migration, not to understand or troubleshoot live migration in-depth. Hence, we recommended moving the content somewhere else. For example, can we have a new architecture page for live migration, can we move the "How Migration Works" section to the lower half of the same page etc.? We didn't say remove it completely. But since you are committed to the current layout, I said in one of my comments above that you can keep it as-is. I hope this makes sense. |
If anybody wants to approve the PR so it can be merged, please go ahead. cc: @ihcsim @Vicente-Cheng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks all reviewers. It has taken such a long time to write and review. I merged the PR for now, The massive details might not meeting all requirements/formatting/expectations from all of us, but we also believe those details are helpful to end users. Let's keep enhancing the doc. Thanks again. |
How VM is marked as non-migratable
How VM is processed on upgrade process
How VM is processed on node maintenance process
How
migrate
menu comesHow hot-plug works with live-migration
Problem:
Live-migrate invloves upgarde, node-maintenance, vm-creation, vm operation, hot-plug and more.
Documents are not always up-to-date and cross-referred.
Solution:
Update the document.
Related Issue(s):
harvester/harvester#8823
Test plan:
Additional documentation or context
Refer
https://github.com/harvester/harvester/blob/b9a3dd4b2b5ede2648b4d76e339c436bff1aa987/pkg/util/virtualmachineinstance/virtualmachineinstance.go#L17 used by upgrade
and
https://github.com/harvester/harvester/blob/b9a3dd4b2b5ede2648b4d76e339c436bff1aa987/pkg/util/virtualmachineinstance/virtualmachineinstance.go#L70 used by
canMigrate
there are minor differences.
Issue harvester/harvester#7128 is also updated on this PR