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

memory_nvdimm: Fix cpu mode to enable test for aarch64 #5378

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

hs0210
Copy link
Contributor

@hs0210 hs0210 commented Jan 4, 2024

The cpu mode is host-passthrough for arm.

Internal snapshots of a VM with pflash based firmware are not supported on arm, so skip this check on arm

The following is the test result on arm, the failed two cases are because arm does not support and they have been skipped in libvirt-ci:

 (1/6) type_specific.io-github-autotest-libvirt.nvdimm.back_file.label.default: PASS (89.60 s)
 (2/6) type_specific.io-github-autotest-libvirt.nvdimm.back_file.label.check_life_cycle: PASS (107.47 s)
 (3/6) type_specific.io-github-autotest-libvirt.nvdimm.back_file.label.discard: PASS (10.98 s)
 (4/6) type_specific.io-github-autotest-libvirt.nvdimm.back_file.label.pmem_alignsize: ERROR: Command '/usr/bin/virsh start avocado-vt-vm1 ' failed.\nstdout: b'\n'\nstderr: b"error: Failed to start domain 'avocado-vt-vm1'\nerror: unsupported configuration: nvdimm pmem property is not available with this QEMU binary\n"\nadditional_info: None (11.85 s)
 (5/6) type_specific.io-github-autotest-libvirt.nvdimm.back_file.no_label: PASS (191.47 s)
 (6/6) type_specific.io-github-autotest-libvirt.nvdimm.hot_plug: FAIL: error: Failed to detach device from /tmp/xml_utils_temp__yvn_jk8.xml\nerror: internal error: unable to execute QEMU command 'device_del': nvdimm device hot unplug is not supported yet.\n (59.04 s)
RESULTS    : PASS 4 | ERROR 1 | FAIL 1 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

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

Others LGTM

virsh.snapshot_create_as(vm_name, vm_s1, ignore_status=False, debug=True)
virsh.snapshot_revert(vm_name, vm_s1, ignore_status=False, debug=True)
virsh.snapshot_delete(vm_name, vm_s1, ignore_status=False, debug=True)
if platform.machine() == 'aarch64':
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use external snapshot instead of simply skipping the steps. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert to external snapshot not supported yet

# virsh snapshot-create-as avocado-vt-vm1 avocado-vt-vm1.s1 --disk-only
Domain snapshot avocado-vt-vm1.s1 created

# virsh snapshot-revert avocado-vt-vm1 avocado-vt-vm1.s1
error: unsupported configuration: revert to external snapshot not supported yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just skip snapshot-revert for external snapshots?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed with feature owner and the decision is we accept the unsupported result in the codes as pass and also keep delete snapshot step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.
For external dish snapshot on ARM
On xxxx-8, both revert and delete are not supported
On xxxx-9, since 9.10.0, both revert and delete are supported, but if the guest is on, the revert will be refused.

Copy link
Contributor

@chunfuwen chunfuwen left a comment

Choose a reason for hiding this comment

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

lgtm

@hs0210 hs0210 force-pushed the memory_nvdimm branch 3 times, most recently from 0a645ad to c5f1efe Compare January 12, 2024 09:13
if not re.search(error_msg_1, revert_result.stderr) \
and not re.search(error_msg_2, revert_result.stderr):
test.fail("The error '{}' or '{}' should be in the output \n'{}'" %
format(error_msg_1, error_msg_2, revert_result.stderr))
Copy link
Contributor

Choose a reason for hiding this comment

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

From line 347 to 357, we could use libvirt.check_result( revert_result, expected_fails=[error_msg_1, error_msg_2])

libvirt/tests/src/memory/nvdimm.py Outdated Show resolved Hide resolved
virsh.snapshot_delete(vm_name, "%s --metadata" % vm_s1, ignore_status=False, debug=True)
domxml = virsh.dumpxml(vm_name).stdout.strip()
xtf_dom = xml_utils.XMLTreeFile(domxml)
snap_file_path = xtf_dom.find("devices/disk/source").get("file")
Copy link
Contributor

Choose a reason for hiding this comment

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

To get the disk source, you might use libvirt_disk.get_first_disk_source(vm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

if os.path.exists(snap_file_path):
os.remove(snap_file_path)
else:
virsh.snapshot_create_as(vm_name, vm_s1, ignore_status=False, debug=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help also update with

virsh.snapshot_create_as(vm_name, "%s --disk-only" % vm_s1,
                                     ignore_status=False, debug=True) 
```for x86_64?
Someone will help to test it on x86_64. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@hs0210 hs0210 force-pushed the memory_nvdimm branch 2 times, most recently from 578d0d7 to 4da8556 Compare January 26, 2024 05:57
Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

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

Thanks

virsh.snapshot_revert(vm_name, vm_s1, ignore_status=False, debug=True)
virsh.snapshot_delete(vm_name, vm_s1, ignore_status=False, debug=True)
if platform.machine() == 'aarch64':
virsh.snapshot_create_as(vm_name, "%s --disk-only" % vm_s1,
Copy link
Contributor

Choose a reason for hiding this comment

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

After I tested on x86_64, the error message is same with on aarch64. That is to say, these operations are arch independant.
So could you remove the arch checking and just keep line 344 to line 355?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL.

The cpu mode is host-passthrough for arm.

Internal snapshots of a VM with pflash based firmware are not supported on arm,
so skip this check on arm.

Signed-off-by: Hu Shuai <[email protected]>
Copy link
Contributor

@dzhengfy dzhengfy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Yingshun Yingshun left a comment

Choose a reason for hiding this comment

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

LGTM

@Yingshun Yingshun merged commit c6a36d8 into autotest:master Feb 22, 2024
5 checks passed
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.

4 participants