Project

Profile

Help

Issue #2500

ValueError exception is raised when uploading module with invalid name

Added by mhrivnak almost 3 years ago. Updated 6 months ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
Severity:
2. Medium
Version:
2.8.7
Platform Release:
2.11.1
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Easy Fix, Pulp 2
QA Contact:
Complexity:
Smash Test:
Verified:
Yes
Verification Required:
No
Sprint:
Sprint 13

Description

When uploading a puppet module that has an unparsable name, pulp raises a ValueError, which bubbles up and results in:

PLP0047: The importer puppet_importer indicated a failed response when uploading puppet_module unit to repository

Pulp should raise a PulpCodedException with a more helpful error message.

From Bugzilla:

The underlying issue appears to be:

~~~
Oct 31 16:21:57 ndhrhnsat2 pulp: pulp_puppet.plugins.importers.importer:ERROR: (17600-26528)   File "/usr/lib/python2.7/site-packages/pulp_puppet/plugins/db/models.py", line 166, in split_filename
Oct 31 16:21:57 ndhrhnsat2 pulp: pulp_puppet.plugins.importers.importer:ERROR: (17600-26528)     author, name = filename.split("/", 1)
Oct 31 16:21:57 ndhrhnsat2 pulp: pulp_puppet.plugins.importers.importer:ERROR: (17600-26528) ValueError: need more than 1 value to unpack
~~~

Models.py does not seem to like name values in the manifest that use underscores:

For example, changing

  "name": "eodendahl_ssh",

to:

  "name": "eodendahl-ssh",

bypasses the error.

Related issues

Related to Puppet Support - Issue #2512: Puppet Importer swallows exception when one is raised during upload CLOSED - CURRENTRELEASE Actions

Associated revisions

Revision a7adae36 View on GitHub
Added by daviddavis almost 3 years ago

Capture ValueError and raise a more informative error

fixes #2500
https://pulp.plan.io/issues/2500

Revision 39c63f22 View on GitHub
Added by daviddavis almost 3 years ago

Capture ValueError and raise a more informative error

fixes #2500
https://pulp.plan.io/issues/2500

History

#1 Updated by bizhang almost 3 years ago

  • Sprint/Milestone set to 31
  • Tags Easy Fix added

#2 Updated by bizhang almost 3 years ago

  • Triaged changed from No to Yes

#3 Updated by daviddavis almost 3 years ago

  • Assignee set to daviddavis

#4 Updated by daviddavis almost 3 years ago

  • Status changed from NEW to ASSIGNED

#5 Updated by daviddavis almost 3 years ago

The PLP0047 error is what I get even if the exception is properly handled currently in the database. Below is an example where I uploaded an invalid tarball which we handle in the code here:

https://github.com/pulp/pulp_puppet/blob/3a07fd8fabf8d79e2c42ead5051fc09888177845/pulp_puppet_plugins/pulp_puppet/plugins/importers/metadata.py#L107-L108

$ pulp-admin puppet repo create --repo-id blah
$ touch blah-blah-1.2.1.tar.gz
$ pulp-admin puppet repo uploads upload -f blah-blah-1.2.1.tar.gz --repo-id blah
+----------------------------------------------------------------------+
                              Unit Upload
+----------------------------------------------------------------------+

Extracting necessary metadata for each request...
[==================================================] 100%
Analyzing: blah-blah-1.2.1.tar.gz
... completed

Creating upload requests on the server...
[==================================================] 100%
Initializing: blah-blah-1.2.1.tar.gz
... completed

Starting upload of selected units. If this process is stopped through ctrl+c,
the uploads will be paused and may be resumed later using the resume command or
canceled entirely using the cancel command.

Uploading: blah-blah-1.2.1.tar.gz
... completed

Importing into the repository...
This command may be exited via ctrl+c without affecting the request.

[\]
Running...

Task Failed

The importer puppet_importer indicated a failed response when uploading
puppet_module unit to repository blah.

Deleting the upload request...
... completed

Here's what journalctl looks like and it seems correct:

Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752) Invalid properties: [u'/var/lib/pulp/uploads/d99efe05-f4d1-47d3-b4f4-e03d04fc7ce9']
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752) Traceback (most recent call last):
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)   File "/home/vagrant/devel/pulp_puppet/pulp_puppet_plugins/pulp_puppet/plugins/importers/importer.py", line 82, in upload_unit
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)     conduit)
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)   File "/home/vagrant/devel/pulp_puppet/pulp_puppet_plugins/pulp_puppet/plugins/importers/upload.py", line 50, in handle_uploaded_unit
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)     extracted_data = metadata_parser.extract_metadata(file_path, repo.working_dir)
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)   File "/home/vagrant/devel/pulp_puppet/pulp_puppet_plugins/pulp_puppet/plugins/importers/metadata.py", line 59, in extract_metadata
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)     metadata = _extract_json(filename, temp_dir)
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)   File "/home/vagrant/devel/pulp_puppet/pulp_puppet_plugins/pulp_puppet/plugins/importers/metadata.py", line 104, in _extract_json
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)     tgz = tarfile.open(name=filename)
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)   File "/usr/lib64/python2.7/tarfile.py", line 1678, in open
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752)     raise ReadError("file could not be opened successfully")
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp_puppet.plugins.importers.importer:ERROR: (4747-26752) InvalidTarball: Invalid properties: [u'/var/lib/pulp/uploads/d99efe05-f4d1-47d3-b4f4-e03d04fc7ce9']
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp.server.managers.content.upload:ERROR: (4747-26752) Error from the importer while importing uploaded unit to repository [saz]
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp.server.managers.content.upload:ERROR: (4747-26752) Traceback (most recent call last):
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp.server.managers.content.upload:ERROR: (4747-26752)   File "/home/vagrant/devel/pulp/server/pulp/server/managers/content/upload.py", line 223, in import_uploaded_unit
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp.server.managers.content.upload:ERROR: (4747-26752)     unit_type=unit_type_id, summary=result['summary'], details=result['details']
Jan 03 20:10:03 dev.example.com pulp[4747]: pulp.server.managers.content.upload:ERROR: (4747-26752) PulpCodedException: The importer puppet_importer indicated a failed response when uploading puppet_module unit to repository saz.

Is this the correct behavior? Or should we be showing something more useful to the user? I ask because I am planning on making a similar change in which we'll be raising a similar coded pulp error.

#6 Updated by bmbouter almost 3 years ago

+1 to doing something more useful for the user.

In terms of making that change, I think @dkliban can give good advice in this area. If you have specific questions mhrivnak or I could also give some advice.

#7 Updated by daviddavis almost 3 years ago

Talked with @dkilban and we opened a new issue to handle how error messages during uploads are displayed to the user as it's a problem that predates this current bug, it's going to be a bigger effort, and there might be some backwards compatibility issues:

https://pulp.plan.io/issues/2512

#8 Updated by daviddavis almost 3 years ago

  • Status changed from ASSIGNED to POST

#9 Updated by daviddavis almost 3 years ago

  • Related to Issue #2512: Puppet Importer swallows exception when one is raised during upload added

#10 Updated by daviddavis almost 3 years ago

  • Subject changed from Error message is generic when uploading module with invalid name to ValueError exception is raised when uploading module with invalid name

#11 Updated by daviddavis almost 3 years ago

  • Status changed from POST to MODIFIED

#13 Updated by semyers almost 3 years ago

  • Platform Release set to 2.11.1

#14 Updated by semyers almost 3 years ago

wrote:

Applied in changeset a7adae36e79d13a3e825e1e0e8f35d7b374ab712.

This is the commit from the PR referenced in an earlier comment, which was merged to master.

Applied in changeset 39c63f22e992a123fe36bd1b4438354369e274c4.

This is a cherry-pick of the a7adae commit back to 2.11-dev, which is how this change becomes available in 2.11.1.

#15 Updated by semyers almost 3 years ago

  • Status changed from MODIFIED to ON_QA

#16 Updated by pthomas@redhat.com almost 3 years ago

  • Status changed from ON_QA to VERIFIED

verified


[root@qe-blade-05 ~]# pulp-admin puppet repo create --repo-id blah
Successfully created repository [blah]

[root@qe-blade-05 ~]# touch blah-blah-1.2.1.tar.gz
[root@qe-blade-05 ~]# pulp-admin puppet repo uploads upload -f blah-blah-1.2.1.tar.gz --repo-id blah
+----------------------------------------------------------------------+
                              Unit Upload
+----------------------------------------------------------------------+

Extracting necessary metadata for each request...
[==================================================] 100%
Analyzing: blah-blah-1.2.1.tar.gz
... completed

Creating upload requests on the server...
[==================================================] 100%
Initializing: blah-blah-1.2.1.tar.gz
... completed

Starting upload of selected units. If this process is stopped through ctrl+c,
the uploads will be paused and may be resumed later using the resume command or
canceled entirely using the cancel command.

Uploading: blah-blah-1.2.1.tar.gz
... completed

Importing into the repository...
This command may be exited via ctrl+c without affecting the request.

[\]
Running...

Task Failed

The importer puppet_importer indicated a failed response when uploading
puppet_module unit to repository blah.

Deleting the upload request...
... completed

#17 Updated by semyers almost 3 years ago

  • Status changed from VERIFIED to CLOSED - CURRENTRELEASE

#18 Updated by pulpbot over 2 years ago

  • Verified changed from No to Yes

#19 Updated by daviddavis over 2 years ago

To clarify the change here is that we're now raising an InvalidModuleName error instead of a ValueError error. This should be visible in the logs. The end user should see this change in the CLI/API after https://pulp.plan.io/issues/2512 is fixed.

#20 Updated by bmbouter over 1 year ago

  • Sprint set to Sprint 13

#21 Updated by bmbouter over 1 year ago

  • Sprint/Milestone deleted (31)

#22 Updated by bmbouter 6 months ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF