Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 9
370 - vm_clone produces VMs that are silently fragile to multi-disk attach#371
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -218,7 +218,7 @@ def __init__( | ||
| power_state=None, | ||
| power_action=None, | ||
| nics=None, # nics represents a list of type Nic | ||
| disks=None, # disks represents a list of type Nic | ||
| disks=None, # disks represents a list of type Disk | ||
| # boot_devices are stored as list of nics and/or disks internally. | ||
| boot_devices=None, | ||
| attach_guest_tools_iso=False, | ||
| @@ -423,6 +423,27 @@ def create_export_or_import_vm_payload(ansible_dict, cloud_init, is_export): | ||
| payload["template"]["cloudInitData"] = cloud_init | ||
| return payload | ||
| @staticmethod | ||
| def clone_add_user_data_to_cloud_init(cloud_init): | ||
| # Task 370 - the generated cloud-init should include a runcmd | ||
| # to fix the grub UUID issue at first boot | ||
| user_data = cloud_init.get("user_data") | ||
| if not user_data: | ||
| return cloud_init | ||
| cloud_init["user_data"] = ( | ||
| user_data.rstrip() | ||
| + """ | ||
| runcmd: | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might end adding a user_data section when we already have a user_data section.
I guess we want to always comment out GRUB_DISABLE_LINUX_UUID ? | ||
| - sed -i 's/^GRUB_DISABLE_LINUX_UUID=true/#GRUB_DISABLE_LINUX_UUID=true/' /etc/default/grub | ||
| - update-grub | ||
| """ | ||
| ) | ||
| return cloud_init | ||
| @classmethod | ||
| def create_clone_vm_payload( | ||
| cls, | ||
| @@ -446,6 +467,7 @@ def create_clone_vm_payload( | ||
| hypercore_tags.append(tag) | ||
| data["template"]["tags"] = ",".join(hypercore_tags) | ||
| if cloud_init: | ||
| cloud_init = cls.clone_add_user_data_to_cloud_init(cloud_init) | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have impression we want to alway run this? | ||
| data["template"]["cloudInitData"] = cloud_init | ||
| if preserve_mac_address: | ||
| data["template"]["netDevs"] = [ | ||
| @@ -610,6 +632,13 @@ def find_disk(self, slot): | ||
| if disk.slot == slot: | ||
| return disk | ||
| # primary disk is the largest Virtio disk | ||
| def get_primary_disk(self): | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could clone a VM with IDE or SCSI disk(s). Not sure why is the largest disk the bootable one. VM with 100 kB cloud-init ISO, 20 GB OS disk, 100 GB installed-apps disk. Maybe the first disk on bus is the OS disk, but if we add/remove disk, we likely can change that too. :( :( Ideally we could ask HC3 about this setting for the source VM. But source VM might never be booted before clonning. Or, we could clone VM, boot it, then assuming cloned VM does boot (really boot, not just wait on A NIC could be a boot device too.
| ||
| virtio_disks = [disk for disk in self.disk_list if disk.type == "virtio_disk"] | ||
| if not virtio_disks: | ||
| return None | ||
| return max(virtio_disks, key=lambda disk: disk.size) | ||
| def post_vm_payload(self, rest_client, ansible_dict): | ||
| # The rest of the keys from VM_PAYLOAD_KEYS will get set properly automatically | ||
| # Cloud init will be obtained through ansible_dict - If method will be reused outside of vm module, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -166,10 +166,29 @@ def run(module, rest_client): | ||
| TaskTag.wait_task(rest_client, task) | ||
| task_status = TaskTag.get_task_status(rest_client, task) | ||
| if task_status and task_status.get("state", "") == "COMPLETE": | ||
| return ( | ||
| True, | ||
| f"Virtual machine - {module.params['source_vm_name']} - cloning complete to - {module.params['vm_name']}.", | ||
| ) | ||
| # Get cloned VM | ||
| virtual_machine_cloned_obj = VM.get_or_fail(query={"name": module.params["vm_name"]}, rest_client=rest_client)[ | ||
| 0 | ||
| ] | ||
| # Set boot devices after cloning Issue-370 (VM starts failing as soon as another disk is attahed if boot is not specified) | ||
| # By default we always set the largest Virtio disk which is the "primary disk" | ||
| primary_disk = virtual_machine_cloned_obj.get_primary_disk() | ||
| boot_items = [primary_disk.to_ansible()] if primary_disk else [] | ||
| # previous boot order after cloning is always empty | ||
| previous_boot_order = [] | ||
| changed = virtual_machine_cloned_obj.set_boot_devices(boot_items, module, rest_client, previous_boot_order) | ||
domendobnikar marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if changed: | ||
| msg = "and boot order was set - you can change it with vm_boot_devices module" | ||
| return ( | ||
| True, | ||
| f"Virtual machine - {module.params['source_vm_name']} - cloning complete to - {module.params['vm_name']} {msg}.", | ||
| ) | ||
| else: | ||
| msg = "and boot order was not set - you can set it with vm_boot_devices module" | ||
| return ( | ||
| True, | ||
| f"Virtual machine - {module.params['source_vm_name']} - cloning complete to - {module.params['vm_name']} {msg}.", | ||
| ) | ||
| raise errors.ScaleComputingError( | ||
| f"There was a problem during cloning of {module.params['source_vm_name']}, cloning failed." | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -18,7 +18,7 @@ | ||
| - ansible.builtin.assert: | ||
| that: | ||
| - output is changed | ||
| - output.msg == "Virtual machine - XLAB-vm_clone-CI_test - cloning complete to - XLAB-vm_clone_CI-test-clone." | ||
| - output.msg == "Virtual machine - XLAB-vm_clone-CI_test - cloning complete to - XLAB-vm_clone_CI-test-clone and boot order was set - you can change it with vm_boot_devices module." | ||
| - name: Retrieve XLAB-vm_clone_CI-test-clone | ||
| scale_computing.hypercore.vm_info: | ||
| @@ -29,11 +29,12 @@ | ||
| - cloned_info.records | length == 1 | ||
| - source_info.records.0.vcpu == cloned_info.records.0.vcpu | ||
| - source_info.records.0.tags != cloned_info.records.0.tags | ||
| - source_info.records.0.boot_devices | length == cloned_info.records.0.boot_devices | length | ||
| - source_info.records.0.disks | length == cloned_info.records.0.disks | length | ||
| - source_info.records.0.nics | length == cloned_info.records.0.nics | length | ||
| - source_info.records.0.nics.0.mac != cloned_info.records.0.nics.0.mac | ||
| - source_info.records.0.node_affinity == cloned_info.records.0.node_affinity | ||
| # Cloned VM's boot devices should be set | ||
| - cloned_info.records.0.boot_devices | length != 0 | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to test with "==". I guess And | ||
| # ----------------------------------Idempotence check------------------------------------------------------------------------ | ||
| - name: Clone XLAB-vm_clone_CI-test into XLAB-vm_clone_CI-test-clone Idempotence | ||
| @@ -57,8 +58,9 @@ | ||
| - cloned_info.records | length == 1 | ||
| - source_info.records.0.vcpu == cloned_info.records.0.vcpu | ||
| - source_info.records.0.tags != cloned_info.records.0.tags | ||
| - source_info.records.0.boot_devices | length == cloned_info.records.0.boot_devices | length | ||
| - source_info.records.0.disks | length == cloned_info.records.0.disks | length | ||
| - source_info.records.0.nics | length == cloned_info.records.0.nics | length | ||
| - source_info.records.0.nics.0.mac != cloned_info.records.0.nics.0.mac | ||
| - source_info.records.0.node_affinity == cloned_info.records.0.node_affinity | ||
| # Cloned VM's boot devices should be set | ||
| - cloned_info.records.0.boot_devices | length != 0 | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
No need to fix grub config in this case?
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.
I'm not 100% sure, this is from the task description: "If the module accepts cloud_init.user_data, the generated cloud-init should include a runcmd to fix the grub UUID issue at first boot".
I read it as "if the the user_data was passed"