Unit tests improperly mocking mkdtemp

Bug #1714306 reported by Ben Nemec on 2017-08-31
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
tripleo
High
Ben Nemec

Bug Description

This caused the following failure in a unit test job:

2017-08-31 04:19:50.942058 | FAIL: tripleo_common.tests.actions.test_logging_to_swift.PrepareLogDownloadActionTest.test_run_success
2017-08-31 04:19:50.942070 | tags: worker-7
2017-08-31 04:19:50.942098 | ----------------------------------------------------------------------
2017-08-31 04:19:50.942116 | Traceback (most recent call last):
2017-08-31 04:19:50.942164 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/site-packages/mock/mock.py", line 1305, in patched
2017-08-31 04:19:50.942182 | return func(*args, **keywargs)
2017-08-31 04:19:50.942232 | File "/home/jenkins/workspace/gate-tripleo-common-python35/tripleo_common/tests/actions/test_logging_to_swift.py", line 143, in test_run_success
2017-08-31 04:19:50.942248 | action.run(self.ctx)
2017-08-31 04:19:50.942315 | File "/home/jenkins/workspace/gate-tripleo-common-python35/tripleo_common/actions/logging_to_swift.py", line 181, in run
2017-08-31 04:19:50.942347 | shutil.rmtree(tmp_dir)
2017-08-31 04:19:50.942391 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/shutil.py", line 465, in rmtree
2017-08-31 04:19:50.942412 | onerror(os.lstat, path, sys.exc_info())
2017-08-31 04:19:50.942453 | File "/home/jenkins/workspace/gate-tripleo-common-python35/.tox/py35/lib/python3.5/shutil.py", line 463, in rmtree
2017-08-31 04:19:50.942469 | orig_st = os.lstat(path)
2017-08-31 04:19:50.942497 | FileNotFoundError: [Errno 2] No such file or directory: '/tmp/test123'

I'm not entirely sure how it ever works, but for some reason it's not failing all the time. We shouldn't be mocking mkdtemp in unit tests anyway though - we already have NestedTempfile to clean up any tempfiles we create, and forcing a write to a known system location is probably a security issue too.

Ben Nemec (bnemec) on 2017-08-31
tags: added: tripleo-common
removed: tripleo-comm
Changed in tripleo:
assignee: nobody → Ben Nemec (bnemec)
status: Triaged → In Progress

Reviewed: https://review.openstack.org/499708
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=0be202dcd102ec06c78059472912938175cc5740
Submitter: Jenkins
Branch: master

commit 0be202dcd102ec06c78059472912938175cc5740
Author: Ben Nemec <email address hidden>
Date: Thu Aug 31 16:29:30 2017 +0000

    Don't mock mkdtemp

    As far as I can tell there's no reason to do this, and it breaks
    the ability for NestedTempfile to clean up after the tests by
    forcing the file to go to the system /tmp directory (which I'm
    pretty sure is a security issue too...). It's also causing some
    weird errors in the gate so let's remove it.

    Change-Id: I04f3f569ff904e002758098cfd90b5bbb31c6afc
    Closes-Bug: 1714306

Changed in tripleo:
status: In Progress → Fix Released
Honza Pokorny (hpokorny) wrote :

What about the other instances in the test suite? e.g.

https://github.com/openstack/tripleo-common/blob/master/tripleo_common/tests/actions/test_plan.py#L305

I don't really understand the issue but it seems weird that it's fine in some places and not others.

Ben Nemec (bnemec) wrote :

You're right, we shouldn't be doing it there either. I just fixed the one that caused the ci job failure. I'll push another patch to remove that one too.

Reviewed: https://review.openstack.org/500083
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=9a6bcf5ab602258c3caa30d57d242d33e6fb79b9
Submitter: Jenkins
Branch: stable/pike

commit 9a6bcf5ab602258c3caa30d57d242d33e6fb79b9
Author: Ben Nemec <email address hidden>
Date: Thu Aug 31 16:29:30 2017 +0000

    Don't mock mkdtemp

    As far as I can tell there's no reason to do this, and it breaks
    the ability for NestedTempfile to clean up after the tests by
    forcing the file to go to the system /tmp directory (which I'm
    pretty sure is a security issue too...). It's also causing some
    weird errors in the gate so let's remove it.

    Change-Id: I04f3f569ff904e002758098cfd90b5bbb31c6afc
    Closes-Bug: 1714306
    (cherry picked from commit 0be202dcd102ec06c78059472912938175cc5740)

tags: added: in-stable-pike

Reviewed: https://review.openstack.org/500129
Committed: https://git.openstack.org/cgit/openstack/tripleo-common/commit/?id=2edc35c840a1b2eb133dd4775a158c61b12924a8
Submitter: Jenkins
Branch: master

commit 2edc35c840a1b2eb133dd4775a158c61b12924a8
Author: Ben Nemec <email address hidden>
Date: Fri Sep 1 16:44:44 2017 +0000

    Don't mock mkdtemp in test_plan.py either

    Same reasoning as I04f3f569ff904e002758098cfd90b5bbb31c6afc.

    Change-Id: I3be7c8e315f1e765c0512619f04a5cb4bc9bb6fc
    Closes-Bug: 1714306

This issue was fixed in the openstack/tripleo-common 7.6.0 release.

This issue was fixed in the openstack/tripleo-common 8.0.0 release.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers