Description of problem: teardownImage reports OK even in cases where disk has already been deleted Version-Release number of selected component (if applicable): rhevm 3.5 vt13.7 How reproducible: 100% Steps to Reproduce: 1. Create a disk and attach it to a VM 2. Run prepareImage and teardownImage on the disk, noting the disk is activated or deactivated 3. Delete the disk, again running prepareImage and then teardownImage Actual results: prepareImage reports the volume does not exist, but teardownImage reports 'OK' Expected results: teardownImage should note that the volume does not exist like with prepareImage Additional info: Here's the output from my automated run which demonstrates the output of running prepareImage and teardownImage on a disk after it has been deleted: 2015-01-21 22:47:10,497 - MainThread - plmanagement.trace - INFO - art.rhevm_api.tests_lib.low_level.disks.deleteDisk(async=True, alias=disk_4_389924_iscsi_1_alias, disk_id=None, positive=True) 2015-01-21 22:47:10,869 - MainThread - disks - INFO - Using Correlation-Id: disks_delete_88587b39-cb57-4f31 2015-01-21 22:47:12,299 - MainThread - rhevmtests.storage.storage_prepareImage_teardownImage.test_prepareImage_teardownImage - INFO - The returned output from running prepareImage on the disk being deleted was 'Volume does not exist: ('41f761d3-07a4-4042-81d3-8e207cadc57d',) ' 2015-01-21 22:47:12,881 - MainThread - rhevmtests.storage.storage_prepareImage_teardownImage.test_prepareImage_teardownImage - INFO - The returned output from running teardownImage on the disk being deleted was 'OK '
(In reply to Gilad Lazarovich from comment #0) > Description of problem: > teardownImage reports OK even in cases where disk has already been deleted > > Version-Release number of selected component (if applicable): > rhevm 3.5 vt13.7 > > How reproducible: > 100% > > Steps to Reproduce: > 1. Create a disk and attach it to a VM > 2. Run prepareImage and teardownImage on the disk, noting the disk is > activated or deactivated > 3. Delete the disk, again running prepareImage and then teardownImage How exactly to you run these verbs? Using vdsClient?
Yes, this is using vdsClient, I provided the output of my automation which makes use of the vdsClient commands
(In reply to Gilad Lazarovich from comment #0) > Description of problem: > teardownImage reports OK even in cases where disk has already been deleted ... > Steps to Reproduce: > 1. Create a disk and attach it to a VM > 2. Run prepareImage and teardownImage on the disk, noting the disk is > activated or deactivated > 3. Delete the disk, again running prepareImage and then teardownImage > > Actual results: > prepareImage reports the volume does not exist, but teardownImage reports > 'OK' > > Expected results: > teardownImage should note that the volume does not exist like with > prepareImage (In reply to Gilad Lazarovich from comment #0) > Description of problem: > teardownImage reports OK even in cases where disk has already been deleted ... > Steps to Reproduce: > 1. Create a disk and attach it to a VM > 2. Run prepareImage and teardownImage on the disk, noting the disk is > activated or deactivated > 3. Delete the disk, again running prepareImage and then teardownImage > > Actual results: > prepareImage reports the volume does not exist, but teardownImage reports > 'OK' > > Expected results: > teardownImage should note that the volume does not exist like with > prepareImage prepareImage must access the volume to prepare it so mising volume it fails when a volume is missing. teardownImage is cleaning up a prepared image. If there is nothing to teardown, why should it fail? If it should fail, it should complain about not finding a prepared image and not about missing volume. On file storage, teardownImage is removing symbolic links so there is no reason that it should fail like prepareImage. On block storage, teardownImage must deactivate an lv, and this probably fail when lv does not exists, so reporting this failure can make sense. Where is it documented that teardownImage should fail when a volume is missing? Changing vdsm behavior can break backward compatibility, for example, one can assume it is safe to call teardownImage during cleanup, and this change will break this assumption. What is the real world problem requiring this change? > Additional info: > Here's the output from my automated run which demonstrates the output of > running prepareImage and teardownImage on a disk after it has been deleted: Why do you run teardownImage if prepareImage failed? there is nothing to teardown. Your code should be something like this: prepareImage() try: use image... finally: teardownImage() teardownImage run only if prepareImage succeeded (assuming that prepareImage raises). I don't see the bug, maybe RFE.
The coverage (automated and manual) is per for test plan for prepareImage/teardownImage, here's a sample case that attempts to prepareImage and teardownImage on a disk after it has been deleted: https://tcms.engineering.redhat.com/case/389924/?from_plan=14466 > On block storage, teardownImage must deactivate an lv, and this probably > fail when lv does not exists, so reporting this failure can make sense. Note that the main focus for automation in this area is block devices, the lv parameters are checked with both prepareImage and teardownImage to ensure volume is activated or deactivated. I haven't seen any detailed documentation for this feature that goes down to the level we're discussing here, but handling for negative cases (where user provides invalid input, has insufficient privileges, etc.) is typically handled with an error or informational message and not with a success message.
(In reply to Gilad Lazarovich from comment #4) Please answer my questions in comment 3.
Nir, what questions do you still need a response for?
(In reply to Nir Soffer from comment #3) > Why do you run teardownImage if prepareImage failed? there is nothing to > teardown. > > Your code should be something like this: > > prepareImage() > try: > use image... > finally: > teardownImage() I'm leaning towards CLOSED NOTBUG based on this comment. PM stakeholders - any objection?
(In reply to Allon Mureinik from comment #7) > (In reply to Nir Soffer from comment #3) > > Why do you run teardownImage if prepareImage failed? there is nothing to > > teardown. > > > > Your code should be something like this: > > > > prepareImage() > > try: > > use image... > > finally: > > teardownImage() > > I'm leaning towards CLOSED NOTBUG based on this comment. > PM stakeholders - any objection? Nope. Looks like this is the design and seems valid.
In case we use the product in such a safe way no bugs will be found. This flow is a negative test case the was done on purpose. In that way we found that even when not providing a valid volume id the vdsClient action of teardownImage the operation still succeeds. From a user POV, it will be much clearer and reasonable to warn them about invalid parameters or operations I'm reopening this bug since it definitely a valid bug. * To be more precise, it is like removing a missing object and not to block with CDA
(In reply to ratamir from comment #9) > * To be more precise, it is like removing a missing object and not to block > with CDA No, it isn't. Fixing this bug will mean breaking a public API.