Bug 1444659 - AttributeError or OSError when trying to rename a directory if the destination directory exists and not empty
Summary: AttributeError or OSError when trying to rename a directory if the destinatio...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: vdsm
Classification: oVirt
Component: Core
Version: 4.20.0
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ovirt-4.2.0
: ---
Assignee: Nir Soffer
QA Contact: Lilach Zitnitski
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-04-23 20:07 UTC by Nir Soffer
Modified: 2017-12-20 11:31 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: No Doc Update
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-12-20 11:31:58 UTC
oVirt Team: Storage
Embargoed:
rule-engine: ovirt-4.2+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
oVirt gerrit 75791 0 master MERGED pylint: Remove broken and unneeded code 2017-04-27 19:39:51 UTC

Description Nir Soffer 2017-04-23 20:07:13 UTC
Description of problem:

File based storage code could fail with AttributeError when renaming a directory
if the destination name was an existing directory.

Version-Release number of selected component (if applicable):
4.16.0 or later.

How reproducible:
Unknown

Reported by pylint.

Comment 1 Nir Soffer 2017-04-23 23:08:29 UTC
Adding a test revealed another failure. rename(2) can fail with ENOTEMPTY or EEXIST
but the code was checking only the first.

If rename(2) failed with EEXIST, oop.os.rename() was failing with:

    OSError: [Errno 17] File exists

If rename(2) failed with ENOTEMPTY, we tried to fallback to the non-atomic rename
and failed with:

    AttributeError: '_IOProcessOs' object has no attribute 'listdir'

Comment 2 Dan Kenigsberg 2017-04-24 06:24:20 UTC
Is there a ovirt-level functional effect to this bug?

I think that the correct thing for rename() to do is to raise EEXIST, not silently overwrite the destination. Unless there's a very good reason to make our awkward semantics suddenly work, I'd fix outOfProcess to match the POSIX semantics.

Comment 3 Nir Soffer 2017-04-24 09:08:24 UTC
(In reply to Dan Kenigsberg from comment #2)
> Is there a ovirt-level functional effect to this bug?

The only place where this can happen is when deleting a volume. The first step
is renaming the volume directory from <uuid> to remove_me_<uuid>. In is flow
we don't need the functionality that the code tried to provide. 

> I think that the correct thing for rename() to do is to raise EEXIST, not
> silently overwrite the destination. Unless there's a very good reason to
> make our awkward semantics suddenly work, I'd fix outOfProcess to match the
> POSIX semantics.

I agree, I'll remove the unneeded code, so rename will have the POSIX semantics.

Comment 4 Nir Soffer 2017-05-29 23:06:36 UTC
This was merge long time ago, but the bot did not move it to modified because
there one of the patches was abandoned.

Comment 5 Lilach Zitnitski 2017-07-12 14:02:23 UTC
Hi Nir, are there any steps to reproduce this bug?

Comment 6 Nir Soffer 2017-07-12 14:44:01 UTC
We don't know how to reproduce this in normal usage of the system.

Comment 7 Lilach Zitnitski 2017-07-16 13:54:51 UTC
(In reply to Nir Soffer from comment #6)
> We don't know how to reproduce this in normal usage of the system.

According to this comment, moving to VERIFIED without reproducing it.

Comment 8 Sandro Bonazzola 2017-12-20 11:31:58 UTC
This bugzilla is included in oVirt 4.2.0 release, published on Dec 20th 2017.

Since the problem described in this bug report should be
resolved in oVirt 4.2.0 release, published on Dec 20th 2017, it has been closed with a resolution of CURRENT RELEASE.

If the solution does not work for you, please open a new bug report.


Note You need to log in before you can comment on or make changes to this bug.