-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
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.
Collaborator
Author
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. a cloud-image VM cloned via this module boots correctly for years while it has exactly one virtio disk, then bricks on the first reboot after any tool (CSI driver, manual disk-add, Terraform provider, or even another playbook in the same suite) attaches a second virtio disk. |
||
| 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 |
|---|---|---|
|
|
@@ -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 | ||
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"