Remove non-functional save_storage_drives from testapi

[mimi1vx]

https://progress.opensuse.org/issues/130922

save_storage_drives never worked and isn't used by any test
(os-autoinst-distri-*) and with high probability in any other instance
of openQA because nobody reported an issue with it.

As for why it can't work --> qemu-img works only with offline images, it
can't work on images used by running or paused VM. Any use of this method
will end with:

qemu-img: Could not open '/var/lib/openqa/pool/2/raid/hd0-overlay0': Failed to get shared "write" lock
  Is another process using the image [/var/lib/openqa/pool/2/raid/hd0-overlay0]?

A fix for this is non-trivial and there is minimum users for this
functionality is better to remove this method first to prevent users to use
it and also to remove practically dead code from os-autoinst

[github-actions]

Great PR! Please pay attention to the following items before merging:

Files matching backend/**.pm:

• Consider running manual verification tests, especially for non-qemu backends

Files matching testapi.pm:

• Consider bumping the version number in OpenQA/Isotovideo/Interface.pm for changes in behaviour

This is an automatically generated QA checklist based on modified files.

[okurz]

The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see https://commit.style/ or http://chris.beams.io/posts/git-commit/ as a helpful guide how to write good commit messages.

Make sure that the git commit message subject line states the Why because that goes into the changelog. Think from the changelog reader perspective what you want to read there.

You can not know if this is used by any tests by any user of openQA. I guess you just grepped os-autoinst-distri-opensuse?

I am not convinced we should remove that function. I am sure it has worked in a certain way when it was introduced. Maybe we introduced a regression since then and our unit tests are of course not going deep enough to really cover the functionality.

Did you try to make use of it and try to fix it?

[mimi1vx]

Nobody ever reported it’s wrong by design and it can’t work … so the premise it isn’t used is with big probability correct + all other instances of openQA are using much simpler tests than o3 and osd

[foursixnine]

You can not know if this is used by any tests by any user of openQA. I guess you just grepped os-autoinst-distri-opensuse?

I am not convinced we should remove that function. I am sure it has worked in a certain way when it was introduced. Maybe we introduced a regression since then and our unit tests are of course not going deep enough to really cover the functionality.

Did you try to make use of it and try to fix it?

@mimi1vx how is it failing? Back when I introduced the function, it was meant for developers interested in getting both, a memory dump and a dump of the storage devices available for the SUT... I I have no attachments to the code, however I would suggest deprecating it first and removing it in the future.

Looking at the unit test, it's kind of snake-oily though... but the backend calls should work otherwise we should have found about this long time ago.

[mimi1vx]

@mimi1vx how is it failing? Back when I introduced the function, it was meant for developers interested in getting both, a memory dump and a dump of the storage devices available for the SUT... I I have no attachments to the code, however I would suggest deprecating it first and removing it in the future.

as I wrote in the commit message it can't work -> qemu-img convert doesn't work on attached images. And nobody newer used this function --> so nobody found it faulty.

as for deprecating .. no need on function which isn't used by anything

[Martchus]

Maybe this is intended to be called when the VM is not in a running state?

[mimi1vx]

Maybe this is intended to be called when the VM is not in a running state?

no, it was intended to run in post_fail_hook after the freeze_vm call, which pauses VM, so it still can't work as freeze_vm don't destroy the VM and free img ...

[Martchus]

Then it really looks like we should remove this function. I'm only wondering why we ever thought this was working (and thus merged it).

[okurz]

Ok for me then as well. I will leave it for @foursixnine to decide then how we should proceed

[foursixnine]

Then it really looks like we should remove this function. I'm only wondering why we ever thought this was working (and thus merged it).

The code changed after the Qemu rewrite, in any case I'm also fine with removing it then too.

[foursixnine]

Hell... I forgot about Mergify! (force of habits) my bad!