RPM Support

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": []
  }

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.

I agree that this is a regression. The behavior should be the same as it was in 2.7.

@dkliban: I agree with you because semantic versioning also does[0].

Separate from that aspect, do you know if and where we are tracking the work identified in Comment 7? If this bug is purposed for fixing this backwards incompatible change, we need a separate one for the traceback request.

[0]: http://semver.org/#what-do-i-do-if-i-accidentally-release-a-backwards-incompatible-change-as-a-minor-version

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.

You've probably played the "git blame" game already. Problem was introduced in 2.8.0:

https://github.com/pulp/pulp_rpm/commit/d3b3e5c4d58b98930c7041489274a98c229227aa

One would hope that there were a unit test to make sure re-uploading a unit does not break :-)

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