Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use rpmio (add support for zstd) #251

Closed
wants to merge 2 commits into from

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Mar 2, 2021

This adds support for rpmio compression modes, it is possible to use --general-compress-type 9.zstd , 9T4.zstd or 7T16.xzdio for parallel (de)compression with createrepo_c.

It does break the ABI of the C library because cr_CompressionType is no longer an enum but instead a const char* and all its values CR_CW_*_COMPRESSION become defined as const char *. However I believe the API is preserved and anything linking with the C library should work after a recompile. This change is required in order to encompass all the possible rpmio modes.

Should help with: #53. #82

The plan is to keep both rpmio and the current compression methods for a while and after some transition period keep just rpmio and zchunk. This would allow to remove quite a lot of code.

@kontura kontura mentioned this pull request Mar 8, 2021
@Conan-Kudo
Copy link
Member

I spoke to @dmach about this a while back, and I'm not sure if we want to do this. One of the major reasons for createrepo_c's somewhat weird way of handling RPMs (internal definitions of some tags, internal handling for compression algorithms and payload data, etc.) is to avoid the problem where RPM needs to be upgraded or have backports for createrepo_c to be useful for newer distribution targets. This has saved our bacon in Fedora and openSUSE enough times that I'm not sure I would want to accept changing to use librpmio for this. I can easily foresee this biting us again later as RPM continues to change and people are required to use older distributions for production while wanting to support newer distributions over time.

@dralley
Copy link
Contributor

dralley commented Mar 29, 2021

I have to agree with @Conan-Kudo. Pulp / Satellite would inevitably run into the same problems - any significant changes to the metadata may result in installations on older RHEL (or any other OS) being unable to publish the metadata expected by newer RHEL (or any other OS). The support timeframes make it a very difficult problem.

@lukash
Copy link
Contributor

lukash commented Mar 30, 2021

I may not be fully understanding the problem, so bear with me:

From what I can see we can use any compression functions that are present in rpm-libs (as long as they are present in rpm-libs versions on all systems we build createrepo_c for). If we need to add support for a new compression that is not present in rpm-libs on older systems for which we build createrepo_c, we'd need to add our own implementation (or dependency). So we should keep the code in such a way it's possible to add different compression implementations other than rpm, but that's all? Am I missing something?

@dralley
Copy link
Contributor

dralley commented Mar 30, 2021

That's the crux of it, yes

@kontura
Copy link
Contributor Author

kontura commented Aug 18, 2021

I couldn't get this to work properly because:

  • we cannot be completely transparent to the compression since createrepo needs to know appropriate compression suffix but there is not way to get it from rpmio for any arbitrary rpmio compression mode
  • there is no straightforward way to validate the rpmio compression modes and when an invalid mode is specified by the user rpmio creates the file normally but doesn't use any compression

Based on this and the concerns raised above I have reduced this PR to just adding zstd support through rpmio. That should make things simpler hopefully.

@kontura
Copy link
Contributor Author

kontura commented Aug 18, 2021

Also created some ci tests: rpm-software-management/ci-dnf-stack#1029

@@ -156,7 +156,7 @@ static GOptionEntry cmd_entries[] =
{ "xz", 0, 0, G_OPTION_ARG_NONE, &(_cmd_options.xz_compression),
"Use xz for repodata compression.", NULL },
{ "compress-type", 0, 0, G_OPTION_ARG_STRING, &(_cmd_options.compress_type),
"Which compression type to use.", "COMPRESSION_TYPE" },
"Which compression type to use. Supported compressions are: bzip2, gzip, zck, zstd, xz.", "COMPRESSION_TYPE" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if "none" were also an option

@Conan-Kudo
Copy link
Member

I'd rather not have rpmio used for compression at all.

@dralley
Copy link
Contributor

dralley commented Aug 18, 2021

As long as it's supported on RHEL8, we can probably live with it. It will cause problems for the binary Python packages though, which is unfortunate. I do agree about preferring not to rely on rpmio but we can see what problems it causes for us in practice and perhaps update the implementation later.

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Aug 18, 2021

The problem is that it's not supported on RHEL 7, and that's where the majority of Foreman+Pulp/Satellite deployments are. And if RPM is not built with zstd support, this will break (which was the case for RHEL 8.0~8.2 and SLE 15 SP0 and SP1).

@dralley
Copy link
Contributor

dralley commented Aug 18, 2021

It partially depends on how fast the uptake is. If it's not actually used for another 2 years then it's less of a problem than if third parties and Fedora start using it immediately (requisite on having a new-enough createrepo_c available to begin with).

But you're right that such assumptions are probably not a good idea.

@Conan-Kudo
Copy link
Member

It was a pain working through the crap caused by RHEL stripping out zstd support when forking from Fedora, and then needing to put it back afterward when Fedora wanted to use zstd for RPM payloads. If I don't have to, I don't want to take on an rpmio dependency for this because I definitely don't want this problem again.

@kontura
Copy link
Contributor Author

kontura commented Sep 22, 2021

I see, ok then I am closing this.

@kontura kontura closed this Sep 22, 2021
@dralley
Copy link
Contributor

dralley commented Sep 22, 2021

#145 (comment)

@dralley
Copy link
Contributor

dralley commented Jan 11, 2023

@kontura @Conan-Kudo For what it's worth, as of the most recent releases of Foreman / Satellite only EL8+ is supported. I'm not sure if that changes anything for you.

I would be OK with this moving forwards with this as long as there is an understanding that leaning too heavily on rpmio can be problematic in the general case.

@Conan-Kudo
Copy link
Member

I don't think it'd be a good idea for us, because it creates unusual constraints for things like Koji and OBS servers too. They are often deployed on the oldest platforms even when builds are being made more newer ones on those systems.

@dralley
Copy link
Contributor

dralley commented Jan 11, 2023

Ack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants