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

[master] Build for CentOS 9. #634

Merged
merged 1 commit into from
May 5, 2022
Merged

Conversation

Romain-Geissler-1A
Copy link
Contributor

This shall be backported to branch 20.10 too.

@Romain-Geissler-1A
Copy link
Contributor Author

Romain-Geissler-1A commented Mar 7, 2022

Note: this will require #633 to be merged first.

@Romain-Geissler-1A
Copy link
Contributor Author

Note: when this will be merged and docker/containerd-packaging#270 will be merged/delivered, I have the local changes to build for CentOS 9 too.

@Romain-Geissler-1A Romain-Geissler-1A changed the title Don't dependend on libcgroup on RHEL/CentOS, as it is deprecated (and removed from RHEL/CentOS >= 9) Don't depend on libcgroup on RHEL/CentOS, as it is deprecated (and removed from RHEL/CentOS >= 9) Mar 18, 2022
@Romain-Geissler-1A
Copy link
Contributor Author

Ping

@@ -22,7 +22,10 @@ Requires: container-selinux >= 2:2.74
Requires: libseccomp >= 2.3
Requires: systemd
Requires: iptables
%if ! 0%{?rhel}
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this conditional based on rhel (/centos) version? If it's not deprecated in rhel7 and centos7, (or 8) we could make it something like;

Suggested change
%if ! 0%{?rhel}
%if 0%{?rhel} == 7 || 0%{?rhel} == 8

Copy link
Member

Choose a reason for hiding this comment

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

Using straight (==) comparisons to account for other distros who would like not have the rhel macro (but double check if my suggestion is correct 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, libcgroup is deprecated in RHEL 7 already: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/resource_management_guide/chap-using_libcgroup_tools

The libcgroup package, which was the main tool for cgroup management in previous versions of Red Hat Enterprise Linux, is now deprecated. To avoid conflicts, do not use libcgroup tools for default resource controllers (listed in Available Controllers in Red Hat Enterprise Linux 7) that are now an exclusive domain of systemd. This leaves a limited space for applying libcgroup tools, use it only when you need to manage controllers not currently supported by systemd, such as net_prio.

Copy link
Member

Choose a reason for hiding this comment

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

The devil can be in the detail

This leaves a limited space for applying libcgroup tools, use it only when you need to manage controllers not currently supported by systemd, such as net_prio.

so I think we should consider keeping it (for now) for the versions that still carry the package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then. I used the expression %if 0%{?rhel} <= 8 does it fly for you ?

@thaJeztah thaJeztah changed the title Don't depend on libcgroup on RHEL/CentOS, as it is deprecated (and removed from RHEL/CentOS >= 9) [master] Don't depend on libcgroup on RHEL/CentOS, as it is deprecated (and removed from RHEL/CentOS >= 9) Mar 27, 2022
@Romain-Geissler-1A
Copy link
Contributor Author

Romain-Geissler-1A commented Mar 29, 2022

Note that another prerequisite to building for RHEL/CentOS 9 is considering something like docker/containerd-packaging#270 (which is failing the CI tests, but since external contributors can't see the Jenkins logs, I don't know why it's failing).

@Romain-Geissler-1A
Copy link
Contributor Author

Ping

1 similar comment
@Romain-Geissler-1A
Copy link
Contributor Author

Ping

@Romain-Geissler-1A
Copy link
Contributor Author

Ping

@Romain-Geissler-1A Romain-Geissler-1A changed the title [master] Don't depend on libcgroup on RHEL/CentOS, as it is deprecated (and removed from RHEL/CentOS >= 9) [master] Build for CentOS 9. May 4, 2022
@Romain-Geissler-1A
Copy link
Contributor Author

@thaJeztah Pull request rebased and updated to adopt your style as in docker/containerd-packaging#283. I have also modified the pull request purpose, now it removes libcgroup dependency for RHEL >= 9 and also builds a package for CentOS 9 at the same time. Most likely the CI will remain red until a containerd package for CentOS 9 is delivered in the official docker repositories.

Comment on lines 21 to 24
# In aarch64 (arm64) images, the altarch repo is specified as repository, but
# failing, so replace the URL.
RUN if [ -f /etc/yum.repos.d/CentOS-Stream-Sources.repo ]; then sed -i 's/altarch/centos/g' /etc/yum.repos.d/CentOS-Stream-Sources.repo; fi

Copy link
Member

Choose a reason for hiding this comment

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

This one was an odd one; I know we added it at some point, then we removed it (because it appeared it didn't have the altarch anymore); #416 (comment)

But later, we had to add it back, because for some reason, our pipeline started failing again on this; see #446

Could it be these can be set dynamically, depending on what mirror is hit?

Comment on lines 21 to 24
# In aarch64 (arm64) images, the altarch repo is specified as repository, but
# failing, so replace the URL.
RUN if [ -f /etc/yum.repos.d/CentOS-Stream-Sources.repo ]; then sed -i 's/altarch/centos/g' /etc/yum.repos.d/CentOS-Stream-Sources.repo; fi

Copy link
Member

Choose a reason for hiding this comment

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

This one was an odd one; I know we added it at some point, then we removed it (because it appeared it didn't have the altarch anymore); #416 (comment)

But later, we had to add it back, because for some reason, our pipeline started failing again on this; see #446

Could it be these can be set dynamically, depending on what mirror is hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was some old hack no longer necessary. But if you say it's still needed, I will put it back.

Copy link
Member

Choose a reason for hiding this comment

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

The honest answer: "I don't know!" 😂 (I also thought it was redundant, and then... all of a sudden, it failed in one of our internal pipelines)

Perhaps we should keep it "just to be safe" (it looks like it doesn't hurt to have it, and would be a no-op if it's not needed).

Signed-off-by: Romain Geissler <romain.geissler@amadeus.com>
@thaJeztah
Copy link
Member

Overall; changes look good to me; I'm running a build locally, as CI doesn't pick up changes in Jenkinsfiles for users without write-access

@thaJeztah
Copy link
Member

Working on a (temporary) fix for failing CI; looks like someone's vanity URL went down; #684

@thaJeztah
Copy link
Member

I kicked CI, but looks like GitHub actions doesn't do a merge before running again (whereas jenkins does).

I tested this locally, and it looks to be working as expected

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit abbac4e into docker:master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants