Bug 658284 - Potential bug in repo delete
Summary: Potential bug in repo delete
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Pulp
Classification: Retired
Component: z_other
Version: unspecified
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
: Sprint 20
Assignee: Jeff Ortel
QA Contact: Preethi Thomas
URL:
Whiteboard:
Depends On:
Blocks: verified-to-close 673053
TreeView+ depends on / blocked
 
Reported: 2010-11-29 21:28 UTC by Jay Dobies
Modified: 2011-08-16 13:59 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-16 13:59:30 UTC
Embargoed:


Attachments (Terms of Use)

Description Jay Dobies 2010-11-29 21:28:39 UTC
At the bottom of the repo delete method:

        for field in ['relative_path', 'cert', 'key', 'ca']:
            if field == 'relative_path' and repo[field]:
                fpath = os.path.join(repo_location, repo[field])
            else:
                fpath =  repo[field]
            if fpath and os.path.exists(fpath):
                try:
                    if os.path.isfile(fpath):
                        os.remove(fpath)
                    else: # os.path.isdir(fpath):
                        shutil.rmtree(fpath)
                    log.info("removing repo files .... %s" % fpath)
                except:
                    #file removal failed
                    raise
                    log.error("Unable to cleanup file %s " % fpath)
                    continue

Note the last three lines. The raise call will cause the log and continue to be unreachable. The log isn't as much concern as the difference between stopping the field loop and continuing. I'm not sure what the original intention was, but this should be cleaned up to properly reflect what was desired.

Comment 1 Jay Dobies 2010-11-29 21:31:08 UTC
Same thing in _get_gpg_keys, though it's only a log statement that's lost (both are in api/repo.py)

        gpg_keys = []
        for gpgkey in gpg_key_list:
            label = gpgkey['gpg_key_label']
            uri   = str(gpgkey['gpg_key_url'])
            try:
                if uri.startswith("file://"):
                    key_path = urlparse(uri).path.encode('ascii', 'ignore')
                    ginfo = open(key_path, "rb").read()
                else:
                    ginfo = serv.fetch_gpgkeys(uri)
                gpg_keys.append((label, ginfo))
            except Exception:
                raise
                log.error("Unable to fetch the gpg key info for %s" % uri)

Comment 2 Jeff Ortel 2011-02-01 19:18:16 UTC
Since the operation is not transacted it cannot be rolled back on error.  It makes sense to log errors and continue to clean up as much as possible.

Fixed: 8f82fcc7b32afc0ff7a39423a15089f4922ac44b

Comment 3 Jay Dobies 2011-02-02 20:07:14 UTC
Fixed in 0.134.

Comment 4 Jay Dobies 2011-03-08 21:01:54 UTC
The key thing in both of these cases is that the raising of an error in these two cases was removed; they now just log a message to pulp.log.


To verify my first comment, we'll need to simulate the server not being able to delete one of the files it's trying to. The easiest way is probably to associate a CA certificate with a repo. Then manually go to the location on disk where it is stored and change the permissions so apache cannot delete the file. When repo delete is called after that, it should hit an error when trying to delete the certificate and log an error message "Unable to cleanup file X" where X is the file you munged the permissions on. The repo delete call should not indicate an error.

Unfortunately, I'm not sure where on disk that cert will be stored when added to a repo. Might need to check with jortel or prad.

As for the second comment, that code is only used in the create_product_repo and update_product_repo calls and I'm unsure of how to actually get them called on a server. Again, might have to check with jortel or prad.

Comment 5 Preethi Thomas 2011-07-25 17:37:40 UTC
verified with repo which has gpg key. changed permissions etc. 

[root@preethi ~]# rpm -q pulp
pulp-0.0.213-1.fc14.noarch


1. generated gpg key
2. signed a package with the gpg
3. created a repo
4. Created a pulp repo with the above repo and gpg
5. Changed the permission on the gpg key file in /var/lib/pulp/repos
6. Ran pulp-admin repo delete

Comment 6 Preethi Thomas 2011-08-16 13:59:30 UTC
Closing with Community Release 15

pulp-0.0.223-4.


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