-
Notifications
You must be signed in to change notification settings - Fork 168
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
add case for dimm memory with access and discard setting #5328
add case for dimm memory with access and discard setting #5328
Conversation
ff7ac0a
to
caf9c33
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.
Others LGTM
from virttest.utils_libvirtd import Libvirtd | ||
|
||
|
||
def get_vm_attrs(test, params): |
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 same function is in #5290. Maybe we can move this function to provide/memory/memory_base.py. What do you think?
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 liping , Good idea. Currently , It's not very common for all the cases , only suitable for these two prs, Could I move it if exists more usage ?
def get_vm_attrs(test, params): | ||
""" | ||
Get vm attrs. | ||
:param test: test 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.
Add a blank line above this line
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 dzheng, Done
variants memory_backing: | ||
- file: | ||
source_type = 'file' | ||
source_attr = "'source_type':'${source_type}'" |
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.
Move this line and all same occurences below to line 53
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 dzheng, Done
return vm_attrs | ||
|
||
|
||
def get_dimm_mem(test, params): |
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.
This function is to create a list of memory devices xml. So let's make the name accurate, like create_dimm_mem_list()
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 dzheng, Done
|
||
test.log.info("TEST_STEP5: Hot unplug all dimm memory device") | ||
for mem in all_dimms: | ||
dimm = memory.Memory() |
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.
There is no need to create new objects again for hotunplugging, I think.
We could put dimm
in line 150 to a list and reuse them here.
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 dzheng, Done
dimm device. | ||
""" | ||
test.log.info("TEST_STEP1: Define vm with dimm memory") | ||
vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) |
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.
It is duplicated with line 207 original_xml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
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.
hi dzheng,Thanks for your comment. L144 is used for step1, original_xml is used for step8
test.fail("Expected '%s', but got '%s'" % (path_error, res)) | ||
|
||
test.log.info("TEST_STEP8: Define guest without any dimm memory") | ||
original_xml.setup_attrs(**vm_attrs) |
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 do not understand why we use vm_attrs
to configure vm xml?
Is it required attributes?
And could we make sure original_xml has no dimm memory device? If not, we need explicitly to remove existing dimm device if any.
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.
Hi dzheng, Thanks your comment. For these two questions:
vm_attrs
should be used to define required attributes as the polarion case- Done , Removed dimm
|
||
test.log.info("TEST_STEP9: Hot plug all dimm memory device") | ||
for mem in all_dimms: | ||
dimm = memory.Memory() |
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.
Reuse above created memory device objects.
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 dzheng, Done
caf9c33
to
a0c7cf6
Compare
xxxx-299048:Dimm memory device with access and discard settings Signed-off-by: nanli <[email protected]>
a0c7cf6
to
fc2ea57
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.
LGTM
Signed-off-by: nanli [email protected]
x86+rhel9 & rhel8
aarm+rhel9