[PULP-1125] Pulp does not correctly handle 2 NVRA (different epoch) packages in one repository

[dralley]

Version

any

Describe the bug

This is a specific subvariant of #2678 (comment)

Package filenames are generated NVRA. Repository uniqueness is constrained by NEVRA. Thus, two packages can co-exist in a repository with the same NVRA but different epochs, and they will declare that their packages are at the same filename, but only one can "win". Currently that probably means the package with the later build time "wins", but DNF downgrade would likely be broken in this instance as there is no way to actually fetch the older package.

The solution needs to also supplant the previous, incomplete solution for a similar issue

pulp_rpm/pulp_rpm/app/tasks/publishing.py

Lines 523 to 549 in c06278a

	# TODO: this is meant to be a !! *temporary* !! fix for
	# https://github.com/pulp/pulp_rpm/issues/2407
	pkg_pks_to_ignore = set()
	latest_build_time_by_nevra = defaultdict(list)
	packages = Package.objects.filter(pk__in=content)
	for pkg in packages.values(
	"pk",
	"name",
	"epoch",
	"version",
	"release",
	"arch",
	"time_build",
	).iterator():
	nevra = format_nevra(pkg["name"], pkg["epoch"], pkg["version"], pkg["release"], pkg["arch"])
	latest_build_time_by_nevra[nevra].append((pkg["time_build"], pkg["pk"]))
	for nevra, pkg_data in latest_build_time_by_nevra.items():
	# sort the packages by when they were built
	if len(pkg_data) > 1:
	pkg_data.sort(key=lambda p: p[0], reverse=True)
	pkg_pks_to_ignore |= set(entry[1] for entry in pkg_data[1:])
	log.warning(
	"Duplicate packages found competing for NEVRA {nevra}, selected the one with "
	"the most recent build time, excluding {others} others.".format(
	nevra=nevra, others=len(pkg_data[1:])
	)
	)

To Reproduce
Put two packages of the same NVRA but different epochs into one repository - they should co-exist yet when the repository is published only one package will be accessible.

Expected behavior

We need a better method of filename conflict resolution than simply "pick the package with the latest build time" to "win" when publishing. Discussion needs to happen on exactly what that is.

[sdherr]

We need a better method of filename conflict resolution than simply "pick the package with the latest build time" to "win" when publishing. Discussion needs to happen on exactly what that is.

The answer is probably that the one with the highest epoch should "win". That is the "newest" package by NEVRA, which is definitely the one to keep. Build-time is a fine fall-back if two packages have the same NEVRA.

I'll also note if my PR in #4237 gets accepted this is not actually a conflict anymore if you choose to use the layout_by_digest or layout_by_both options instead of the default layout_alphabetically. As currently written that PR will still "de-duplicate" these same-NVRA packages that collide on url, however seems to be easily fixable.

[sdherr]

I'll also note that "same-NVRA-different-E" packages should be rare to the point of vanishing. Although the RPM docs don't do a good job of saying so, I think the point of incrementing the Epoch field is if you want to change versioning scheme. How do you instruct dnf that 2.0.0 is "higher than" 2024.06.31? You increment the Epoch. Incrementing the Epoch and not changing the versioning scheme seems like all-around a bad idea. Which is not to say it doesn't happen...

[ggainey]

The RPM ecosystem is kind of full of "things that are not Expressly Forbidden", that are still Bad Ideas :)

The other major use of epoch is when a packaging-error has happened - e.g., a distribution has foo-1.0.0 and someone releases foo-2.0.0 with breaking changes into the current GA distribution instead of the next one. One would then package and release foo-1:1.0.1, in order to be "newer than" the mistakenly-released foo-2.0.0, and future releases of foo would all carry the epoch along with them.

[dralley]

I'll also note that "same-NVRA-different-E" packages should be rare to the point of vanishing. Although the RPM docs don't do a good job of saying so, I think the point of incrementing the Epoch field is if you want to change versioning scheme. How do you instruct dnf that 2.0.0 is "higher than" 2024.06.31? You increment the Epoch. Incrementing the Epoch and not changing the versioning scheme seems like all-around a bad idea. Which is not to say it doesn't happen...

Fully agree that it's rare, but like Grant says there's definitely cases where it happens - pretty sure I've even seen one or two instances of it.

[sdherr]

I'll also note if my PR in #4237 gets accepted this is not actually a conflict anymore if you choose to use the layout_by_digest or layout_by_both options instead of the default layout_alphabetically.

PR #4237 now allows you to work around this conflict if you choose nested_by_digest or nested_by_both layout on either the repository or publication (overrides repository setting). Both packages with the same NVRA will be published on unique URLs and both will be present in the repo metadata, making them available to clients. The current behavior is still default with the nested_alphabetically layout, so this is currently only really a work-around not a proper fix. I leave it to the pulp_rpm maintainers to decide if one of these options should become the default, or if there's another appropriate fix here.

I don't believe there are currently any user docs discussing the layout feature, but it is present in the api docs.

[pedro-psb]

IHO having the nested_by_digest as the default should be the way to "officialy" fix this issue. It's a more robust naming form and has the side benefit of the cache-friendly names. I can't see any major down sides. It may require existing installations to do something (maybe ?), but it fits the requirements for a breaking type of change (pulp 4?).

Also, one may argue about that it's not human-friendly. IHO our main target audience are clients like dnf, and this is not a problem at all for dnf and friends. For those who really want the human-friendly layout (which I believe is a minority), they can opt-in for the alphabetic one (with warning that there might be edge cases it can't handle) or the the nested_by_both (which handles the edge cases but creates double the entries, if that is a concern).

[dralley]

IHO having the nested_by_digest as the default should be the way to "officialy" fix this issue. It's a more robust naming form and has the side benefit of the cache-friendly names. I can't see any major down sides.

I mean, the downside is that the repository becomes practically unbrowsable, yeah? A bunch of directory named after digest fragments is unhelpful in terms of looking at the repository at a glance.

True, DNF and such is the main consumer so it's not exactly the main case, but for how rarely this is a real issue it's worth considering.

I mean, most repos are created with createrepo_c, and if you think about it like createrepo_c, then both packages couldn't exist in the same directory with the same name in the first place. In which case changing the repo uniqueness constraint to remove "epoch" would give you similar behavior, you would avoid having 2 packages with such a name conflict in the repo the first place. So one way to resolve it would be to just emulate that.

Or else, we could add deconfliction to nested_alphabetically that doesn't just ignore all but one package. Maybe if we did that we wouldn't necessarily need nested_by_both

[ggainey]

I mean, the downside is that the repository becomes practically unbrowsable, yeah? A bunch of directory named after digest fragments is unhelpful in terms of looking at the repository at a glance.

There are a lot of rpm-repo-users In The Wild who depend on being able to browse repositories. While that's not what rpm-repos are for - it's absolutely a significant de-facto usecase.

The number of times a package is "re-epoch'd" without changing its version would be waaaay less frequent. Changing the default layout to something nobody has ever seen before, to fix a very rare edge-case that arguably shouldn't be allowed in the first place, is a suboptimal response.