Change in vdsm[ovirt-3.5]: tests: Fix and simplify cleanup after failures

Hello Piotr Kliczewski, Sandro Bonazzola, Shmuel Leib Melamud, Dan Kenigsberg,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/47326

to review the following change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

tests: Fix and simplify cleanup after failures

If mount or umount failed, temporary images were not removed because of
incorrect cleanup code. This was not an issue since the temporary files
were created in a temporary directory, but we had two of them, so
failures to remove the first would leave also the second.

This patch simplify cleanup by removing the incorrect cleanup of the
temporary images, as they are removed anyway when the temporary
directory is removed.

Instead of two temporary directories, we use now one directory, and
create the two subdirectories inside it. Now we have to remove only one
directory in tearDown. To make it easier to debug, the temporary
directory is created with a prefix.

Change-Id: Iae829de5ae37008eff3154ad97d5f86c92a41eb2
Signed-off-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/46387
Continuous-Integration: Jenkins CI
Reviewed-by: Shmuel Leib Melamud 
Reviewed-by: Sandro Bonazzola 
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Dan Kenigsberg 
---
M tests/mkimageTests.py
1 file changed, 6 insertions(+), 6 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/26/47326/1

diff --git a/tests/mkimageTests.py b/tests/mkimageTests.py
index 027516c..44f7085 100644
--- a/tests/mkimageTests.py
+++ b/tests/mkimageTests.py
@@ -60,8 +60,11 @@
             "DISKIMAGE_GROUP": mkimage.DISKIMAGE_GROUP,
             "_P_PAYLOAD_IMAGES": mkimage._P_PAYLOAD_IMAGES
         }
-        self.workdir = mkdtemp()
-        self.img_dir = mkdtemp()
+        self.tempdir = mkdtemp(prefix="vdsm-mkimage-tests.")
+        self.workdir = os.path.join(self.tempdir, "work")
+        os.mkdir(self.workdir)
+        self.img_dir = os.path.join(self.tempdir, "images")
+        os.mkdir(self.img_dir)
         mkimage.DISKIMAGE_USER = -1
         mkimage.DISKIMAGE_GROUP = -1
         mkimage._P_PAYLOAD_IMAGES = self.img_dir
@@ -82,8 +85,7 @@
         Removes the workdir and its content when finished.
         Restore original values of mkimage constants.
         """
-        rmtree(self.workdir)
-        rmtree(self.img_dir)
+        rmtree(self.tempdir)
         mkimage.DISKIMAGE_USER = self.orig_mkimage["DISKIMAGE_USER"]
         mkimage.DISKIMAGE_GROUP = self.orig_mkimage["DISKIMAGE_GROUP"]
         # pylint: disable=W0212
@@ -174,7 +176,6 @@
             self._check_label(floppy, label)
         finally:
             m.umount(force=True, freeloop=True)
-            os.unlink(floppy)

@permutations([[None], ['fslabel']])
     def test_mkIsoFs(self, label):
@@ -204,7 +205,6 @@
                                      (stat.S_IXOTH, False)))
         finally:
             m.umount(force=True, freeloop=True)
-            os.unlink(iso_img)

def test_removeFs(self):
         """

[e-mail] has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 1: Verified-1

* Update tracker::IGNORE, no Bug-Url found

* Check Bug-Url::ERROR, At least one bug-url is required for the stable branch
* warn_if_not_merged_to_previous_branch: OK

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 1: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 1: Code-Review+2

[e-mail] has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 2:

* Update tracker::#1267851::OK
* Check Bug-Url::OK
* Check Public Bug::#1267851::OK, public bug
* Check Product::#1267851::OK, Correct classification oVirt
* Check TM::#1267851::OK, correct target milestone ovirt-3.5.6
* Check merged to previous::OK, change not open on any previous branch

Francesco Romani has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 2: Continuous-Integration+1 Verified+1

CI scripts run manually
verified running the test itself.

Francesco Romani has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 2: Code-Review+2

Francesco Romani has submitted this change and it was merged.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

tests: Fix and simplify cleanup after failures

If mount or umount failed, temporary images were not removed because of
incorrect cleanup code. This was not an issue since the temporary files
were created in a temporary directory, but we had two of them, so
failures to remove the first would leave also the second.

This patch simplify cleanup by removing the incorrect cleanup of the
temporary images, as they are removed anyway when the temporary
directory is removed.

Instead of two temporary directories, we use now one directory, and
create the two subdirectories inside it. Now we have to remove only one
directory in tearDown. To make it easier to debug, the temporary
directory is created with a prefix.

Change-Id: Iae829de5ae37008eff3154ad97d5f86c92a41eb2
Bug-Url: https://bugzilla.redhat.com/1267851
Signed-off-by: Nir Soffer 
Reviewed-on: https://gerrit.ovirt.org/46387
Continuous-Integration: Jenkins CI
Reviewed-by: Shmuel Leib Melamud 
Reviewed-by: Sandro Bonazzola 
Reviewed-by: Piotr Kliczewski 
Reviewed-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/47326
Continuous-Integration: Francesco Romani 
Tested-by: Francesco Romani 
Reviewed-by: Francesco Romani 
---
M tests/mkimageTests.py
1 file changed, 6 insertions(+), 6 deletions(-)

Approvals:
  Francesco Romani: Verified; Looks good to me, approved; Passed CI tests

[e-mail] has posted comments on this change.

Change subject: tests: Fix and simplify cleanup after failures
......................................................................

Patch Set 3:

* Update tracker::#1267851::OK
* Set MODIFIED::bug 1267851::::#1267851::::IGNORE, not oVirt prod but vdsm