action #104520
openMove svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry
Added by tinita almost 3 years ago. Updated over 2 years ago.
0%
Description
Observation¶
The test https://openqa.suse.de/tests/7917723 failed because the code was changed to always execute do_extract_assets
when there were PUBLISH_HDD_*
settings.
However, do_extract_assets
is only implemented in 3 backend classes in os-autoinst currently.
The test in question, svirt_upload_assets.pm
implements extract_assets itself, and it does this using the same special setting PUBLISH_HDD_*
as os-autoinst:
https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/tests/shutdown/svirt_upload_assets.pm
This only worked because PUBLISH_HDD_ was simply ignored for anything but qemu so far.
The code above should be moved to backend::svirt, and after that we can hopefully remove the check for qemu.
Acceptance criteria¶
- AC1: Assets for svirt are published via os-autoinst backend code
Suggestions¶
- Remove svirt_upload_assets from the schedule after deploying the new backend code
- Early return in svirt_upload_assets if the backend method is implement
if backend::svirt->can('do_extract_assets')
Updated by tinita almost 3 years ago
- Copied from action #104499: s390 tests fail with backend died: unable to extract assets: Too many arguments for subroutine 'backend::baseclass::do_extract_assets added
Updated by livdywan almost 3 years ago
I changed my existing PR to an untested proof of concept https://github.com/os-autoinst/os-autoinst/pull/1895
Updated by livdywan almost 3 years ago
- Status changed from New to Feedback
- Assignee set to livdywan
tinita wrote:
Is this in progress?
I guess it's in Feedback. Wasn't consciously my intention to take it right away, but then I started reaching out to relevant experts to make sure we know what we're doing here.
Updated by livdywan almost 3 years ago
- Subject changed from Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm to Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M
- Description updated (diff)
Updated by livdywan almost 3 years ago
- Status changed from Feedback to In Progress
- Discussed on Slack
- Make
svirt_upload_assets
a no-op if the backend implements the functionality https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13974 - Implement
do_extract_assets
for svirt https://github.com/os-autoinst/os-autoinst/pull/1895
Updated by livdywan almost 3 years ago
cdywan wrote:
- Make
svirt_upload_assets
a no-op if the backend implements the functionality https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13974
./script/openqa-clone-job --within-instance http://openqa.suse.de --skip-deps 7917723 _GROUP=0 CASEDIR="https://github.com/kalikiana/os-autoinst-distri-opensuse.git#svirt_upload_assets_skip"
Created job #7968548: sle-12-SP4-Server-DVD-Updates-s390x-Build20211229-1-mru-install-minimal-with-addons@s390x-kvm-sle12 -> http://openqa.suse.de/t7968548
Updated by livdywan almost 3 years ago
- Related to action #76813: [tools] Test using svirt backend fails with auto_review:"Error connecting to VNC server.*: IO::Socket::INET: connect: Connection refused" added
Updated by livdywan almost 3 years ago
- Due date changed from 2022-01-13 to 2022-01-21
I'll admit that context switching with topics has slowed me down here, and then I ran into #76813 trying verify the distri-opensuse changes. So I'm bumping the due date now.
Updated by livdywan almost 3 years ago
I put some more verbose thoughts into review comments. Currently I'd describe my progress as follows:
- I'd like to use TDD here, since there was some confusion on the expected behavior of the function, but I'm unsure from reading e.g. qemu code what exactly is expected (e.g. why would svirt need its own image path handling and uploading) and we don't seem to have a test specifically for
do_extract_assets
. - I tried to re-use the
Qemu
module to avoid duplication but I'm not sure it really makes sense to set it up just to convert images - so I'm thinking to split out Qemu:LDriveDevice::gen_qemu_img_convert, if that's all we need
Updated by okurz almost 3 years ago
- Related to action #99663: Use more perl signatures - os-autoinst size:M added
Updated by livdywan almost 3 years ago
- Due date changed from 2022-01-21 to 2022-01-28
Moved from "defined" to "can" now, made the unit test work and added docs for the variables which is now required. Bumping the due date since this https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/13974 also hasn't been merged yet.
Updated by livdywan almost 3 years ago
Unfortunately it looks like something went wrong and causes failures in jobs now:
console svirt does not exist at /usr/lib/os-autoinst/backend/baseclass.pm line 686.
backend::baseclass::console(backend::svirt=HASH(0x10042805f70), "svirt") called at /usr/lib/os-autoinst/backend/svirt.pm line 181
backend::svirt::do_extract_assets(backend::svirt=HASH(0x10042805f70), HASH(0x10042349640)) called at /usr/lib/os-autoinst/backend/driver.pm line 80
backend::driver::extract_assets(backend::driver=HASH(0x10042912848), HASH(0x10042349640)) called at /usr/lib/os-autoinst/OpenQA/Isotovideo/Utils.pm line 163
eval {...} called at /usr/lib/os-autoinst/OpenQA/Isotovideo/Utils.pm line 163
OpenQA::Isotovideo::Utils::handle_generated_assets(OpenQA::Isotovideo::CommandHandler=HASH(0x10042cb0e78), 1) called at /usr/bin/isotovideo line 409
[2022-01-26T08:26:33.975697+01:00] [warn] !!! bmwqemu::serialize_state: unable to extract assets: Can't call method "name" on an undefined value at /usr/lib/os-autoinst/backend/svirt.pm line 181.
backend::svirt::do_extract_assets(backend::svirt=HASH(0x10042805f70), HASH(0x10042349640)) called at /usr/lib/os-autoinst/backend/driver.pm line 80
backend::driver::extract_assets(backend::driver=HASH(0x10042912848), HASH(0x10042349640)) called at /usr/lib/os-autoinst/OpenQA/Isotovideo/Utils.pm line 163
eval {...} called at /usr/lib/os-autoinst/OpenQA/Isotovideo/Utils.pm line 163
OpenQA::Isotovideo::Utils::handle_generated_assets(OpenQA::Isotovideo::CommandHandler=HASH(0x10042cb0e78), 1) called at /usr/bin/isotovideo line 409
Edit: Rollback triggered, will propose a fix shortly.
Updated by livdywan almost 3 years ago
- Subject changed from Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M to Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry
Updated by tinita almost 3 years ago
btw, do we still need the auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry
in the title?
edit: oh sorry, you just added it
Updated by livdywan almost 3 years ago
tinita wrote:
btw, do we still need the
auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry
in the title?
edit: oh sorry, you just added it
For the record, I used host=openqa.suse.de ./openqa-monitor-incompletes
to identify affected jobs anyway but put the regex in case I missed one.
The above branch now has test coverage for all uses of the "vmname" and addresses the problem by not relying on the console when assets are being prepared.
Updated by livdywan almost 3 years ago
- Status changed from In Progress to Feedback
Branch merged, waiting to confirm that this is working fine in production
Updated by okurz almost 3 years ago
- Related to action #105690: s390x svirt jobs incomplete with auto_review:"unable to extract assets:.*/var/lib/libvirt/images/a.img":retry added
Updated by livdywan almost 3 years ago
- Blocked by coordination #105699: [epic] 5 whys follow-up to s390x svirt jobs incomplete with unable to extract assets:.*/var/lib/libvirt/images/a.img" size:S added
Updated by livdywan almost 3 years ago
- Due date deleted (
2022-01-28) - Status changed from Feedback to Blocked
The obvious next step would be to understand where the unit tests deviate from production since the vmname was not initialized as expected. But we should conduct #105690 first.
Updated by livdywan almost 3 years ago
- Status changed from Blocked to Workable
- Assignee deleted (
livdywan)
Updated by okurz almost 3 years ago
- Target version changed from Ready to future
Updated by okurz over 2 years ago
- Category changed from Regressions/Crashes to Feature requests
- Parent task deleted (
#105699)