Issue #2274
closed
Uploading duplicate content results in ambiguous error message
Status:
CLOSED - CURRENTRELEASE
Description
This can happen when the content unit is associated with a repository already or when dealing with an orphaned unit. User should be notified that the unit is already in Pulp.
pulp-admin iso repo create --repo-id test
pulp-admin iso repo uploads upload --file isotest.iso --repo-id test
pulp-admin iso repo uploads upload --file isotest.iso --repo-id test
+----------------------------------------------------------------------+
Unit Upload
+----------------------------------------------------------------------+
Extracting necessary metadata for each request...
[==================================================] 100%
Analyzing: isotest.iso
... completed
Creating upload requests on the server...
[==================================================] 100%
Initializing: isotest.iso
... 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: isotest.iso
[==================================================] 100%
11/11 bytes
... completed
Importing into the repository...
This command may be exited via ctrl+c without affecting the request.
[\]
Running...
Task Failed
The importer iso_importer indicated a failed response when uploading iso unit to
repository test.
Deleting the upload request...
You can see some details using -vv option for pulp-admin.
"summary": "ISO already exists"
"error": {
"code": "PLP0047",
"data": {
"unit_type": "iso",
"importer_id": "iso_importer",
"repo_id": "test",
"details": null,
"summary": "ISO already exists"
},
"description": "The importer iso_importer indicated a failed response when uploading iso unit to repository test.",
"sub_errors": []
}
- Triaged changed from No to Yes
- Related to Issue #2006: iso importer fails without useful error message added
We are hitting this bug also. Any idea when it will get fixed?
It fails also on deleted units. In order to fix it , we need to clean all orphans.
I expect this issue to be fixed very soon. Though I can't say which release it will be included in.
The problem is in iso/importer.py, in upload_unit:
if models.ISO.objects.filter(**unit_key).count() > 0:
return {'success_flag': False, 'summary': _('ISO already exists'), 'details': None}
So this flags two problems:
- pulp_admin not exposing the real error
- the ISO importer being too strict
Finally, even in the server's error logs I get the unhelpful "indicated a failed response", but not the actual failure. It is a pain to have to retrieve the task in order to read the error string. Can the server directly propagate that error into the logs?
All of these are regressions from pulp 2.7 (from which I upgraded to 2.10 over the weekend).
I agree completely regarding pulp-admin not logging the correct error and the actual traceback not being included in the logs. I believe @dkliban is planning/working on an effort to improve that throughout platform and the plugins maintained by the core team. I'm not sure where the tracking issues are for that currently. I do not believe that is a regression from 2.7 though since Pulp has been handling it's exceptions in this unhelpful way for a long time.
Regarding this check being too strict, what else could we do? if we attempt to save an ISO unit and there is already one saved with the same unit key, the database will reject it. That uniqueness constraint could be raised all the way up so the message could be improved, but I don't understand that observation that those lines are 'too strict'. Can you elaborate some on that?
I expected the same uniqueness constraint would be raised in Pulp 2.7- also.
My comment about being too strict was that I don't believe the rpm importer behaves like that.
If you re-upload the rpm, and it already exists, it will find the unit in the DB and use that, instead of the one you upload.
See plugins/importers/yum/upload.py:
unit.set_storage_path(os.path.basename(file_path))
try:
unit.save_and_import_content(file_path)
except NotUniqueError:
unit = unit.__class__.objects.filter(**unit.unit_key).first()
if rpm_parse.signature_enabled(config):
rpm_parse.filter_signature(unit, config)
repo_controller.associate_single_unit(repo, unit)
Almost the same code exists in the iso importer's upload_unit, further down, but the additional check (which does not exist in _handle_package for rpm and friends) prevents it from being reached.
- Triaged changed from Yes to No
@mihai.ibanescu Thank you for the reply. I see what you mean. Pulp should catch this exception and recognize the unit is already in the DB for consistency with other unit types in the RPM plugin and compatibility with previous releases.
This is a little bit of a grey area with semantic versioning. Since we've already broken backwards compatibility, the only thing we could do is to break backwards compatibility again (to put back the behavior in 2.12). For users who started with 2.8+, this would break their backwards compatibility. Is that what we should do?
I'm untriaging this based on this new discussion to get input from other devs.
I agree that this is a regression. The behavior should be the same as it was in 2.7.
We do not have an issue that is tracking "better handling of exceptions raised during sync or publish". The behavior varies between all the importers and distributors. Fixing all of them with the same solution would introduce backwards incompatible changes for some of them.
Thanks @dkliban. Backwards incompatible changes are ok if they fix a bug. I think the overly generic error handling falls in that category. Should we create a separate bug to specifically track this one traceback change, or are we still planning to overhaul all of the plugins like we were discussing in Dec?
If we go back to how the importer worked in 2.7, then there is no issue to file for handling the exception. However, there are probably other exceptions that could be raised. We can file an issue for that. However, I don't have any specific reproduction steps for such theoretical problem.
Oh look I made that commit. :/
I agree completely we should have a test for this use case, but I think it would need to be an integration test. The good news is that we have an integration test suite[0] that is getting better each day. Would you want to file an issue to have that test written? They use the github issue tracker for that[1].
Sorry for the regression, but thanks for identifying it!
[0]: http://pulp-smash.readthedocs.io/en/latest/
[1]: https://github.com/PulpQE/pulp-smash/issues
- Severity changed from 2. Medium to 3. High
- Triaged changed from No to Yes
- Sprint/Milestone set to 34
- Status changed from NEW to ASSIGNED
- Assignee set to semyers
- Project changed from Pulp to RPM Support
- Status changed from ASSIGNED to POST
For this fix, I stayed narrowly focused on the regression that generates the error, but intentionally did not address the more generic issue of improving the reporting of errors during upload. I think we do need to untangle that ball of yarn, but in the immediate timeframe I think it's most important to deal with the regression as soon as possible.
https://github.com/pulp/pulp_rpm/pull/1033
I will follow up and find or file a more generic issue related to improving the generic reporting of issues during sync or publish, as mentioned by dkliban in comment 13.
- Status changed from POST to MODIFIED
- Platform Release set to 2.12.2
- Status changed from MODIFIED to 5
- Status changed from 5 to CLOSED - CURRENTRELEASE
- Sprint/Milestone deleted (
34)
Also available in: Atom
PDF
Allow ISOs to be re-uploaded
This addresses a regression introduced in 2.8.0. Prior to this release of Pulp, uploading an ISO that Pulp already had stored would use the existing ISO in the upload step and succeed. This change restores the prior behavior, removing this regression.
https://pulp.plan.io/issues/2274 closes #2274