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

Warn if cgroups-v1 #21431

Merged
merged 1 commit into from Feb 6, 2024
Merged

Warn if cgroups-v1 #21431

merged 1 commit into from Feb 6, 2024

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Jan 30, 2024

Podman v5 will not support cgroups-v1. This commit will print a warning if it detects a cgroups-v1 system.

Resolves: https://issues.redhat.com/browse/RUN-1957

Does this PR introduce a user-facing change?

cgroups-v1 is deprecated in favor of cgroups-v2 with Podman v5 and will be removed in a future version. Users on cgroups-v1 systems will see warnings to that effect and should consider switching to cgroups-v2.

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 30, 2024
Copy link
Contributor

openshift-ci bot commented Jan 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2024
@dustymabe
Copy link
Contributor

So will podman v5 user systems fail if on cgroups v1 OR will they just see this warning?

i.e. deprecated OR not supported at all ?

@rhatdan
Copy link
Member

rhatdan commented Jan 30, 2024

It is not supported. I believe. Not sure if this should warn or fail.

@dustymabe
Copy link
Contributor

It is not supported. I believe. Not sure if this should warn or fail.

A better user experience would be for it to fail with a helpful error message rather than warn and then let things fail later with possibly unclear errors.

@lsm5
Copy link
Member Author

lsm5 commented Jan 31, 2024

It is not supported. I believe. Not sure if this should warn or fail.

A better user experience would be for it to fail with a helpful error message rather than warn and then let things fail later with possibly unclear errors.

@mheon wdyt?

@lsm5
Copy link
Member Author

lsm5 commented Jan 31, 2024

It is not supported. I believe. Not sure if this should warn or fail.

A better user experience would be for it to fail with a helpful error message rather than warn and then let things fail later with possibly unclear errors.

@dustymabe AFAIK, we need to support v1 on rhel9 for life, so this part will need to be patched out from the rhel9 rpms. I'm open to suggestions on how we deal with other environments.

@lsm5
Copy link
Member Author

lsm5 commented Feb 1, 2024

@dustymabe are there fcos or rhcos configs with cgroups-v1 still being supported, apart from rhel9-based ones? Asking because AFAIK, fedora is on v2 already so this change will have no effect. RHEL 10 will also be v2 AFAIK and on RHEL 9, this warning will be patched out at rpm build time. So, best I can tell, fedora / fcos and their downstreams / derivatives won't be affected with this change.

With this warning in place, users on other envs with v1 won't be broken right away and we can do the actual removal / failure in a later version. @mheon correct me if I'm wrong or forgetting anything from prior discussion.

@giuseppe
Copy link
Member

giuseppe commented Feb 1, 2024

we still need to keep the code around so IMO a warning is better. We don't break users stuck on cgroup v1 for any reason, and we can ignore cgroup v1 issues as the warning message is clear. Maybe we can just mention that it will be removed in future:

"WARNING! You are using cgroups v1 which is not supported on Podman v5 and will be removed in a future version. Consider switching to cgroups v2."

@lsm5
Copy link
Member Author

lsm5 commented Feb 1, 2024

we still need to keep the code around so IMO a warning is better. We don't break users stuck on cgroup v1 for any reason, and we can ignore cgroup v1 issues as the warning message is clear. Maybe we can just mention that it will be removed in future:

"WARNING! You are using cgroups v1 which is not supported on Podman v5 and will be removed in a future version. Consider switching to cgroups v2."

@giuseppe thanks updated.

I'd rather not change anything in the docs right now as rhel9 will still need cgroups-v1. I feel the warning and the RELEASE_NOTES update should suffice.

I'll update the rpm spec to patch out this warning in an additional commit.
EDIT: added rpm spec patch in the same commit so it's easier to revert with the main change.

@mheon
Copy link
Member

mheon commented Feb 1, 2024

Yeah, you captured the discussion @lsm5 - we can't entirely remove v1 support but we definitely want people to be aware they are using it (and also we want to make it very obvious in upstream bug reports so we can ask folks to move off deprecated code).

@lsm5 lsm5 marked this pull request as ready for review February 1, 2024 13:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2024
@lsm5
Copy link
Member Author

lsm5 commented Feb 1, 2024

@containers/podman-maintainers PTAL

cmd/podman/main.go Outdated Show resolved Hide resolved
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

I would assume this needs some doc changes as well. The man page should state that it is not supported if we say this in the code.
Although I must say I find saying "not supported" extremely confusing the code clearly supports it today and it should work (I think). Wouldn't it be better to say it is deprecated in favor of v2?

cmd/podman/main.go Outdated Show resolved Hide resolved
@lsm5
Copy link
Member Author

lsm5 commented Feb 1, 2024

I would assume this needs some doc changes as well. The man page should state that it is not supported if we say this in the code. Although I must say I find saying "not supported" extremely confusing the code clearly supports it today and it should work (I think). Wouldn't it be better to say it is deprecated in favor of v2?

I'm fine with changing to deprecated in favor of v2 assuming everyone else is too.

@lsm5 lsm5 marked this pull request as draft February 1, 2024 14:46
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2024
@lsm5 lsm5 marked this pull request as ready for review February 2, 2024 13:00
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@lsm5
Copy link
Member Author

lsm5 commented Feb 2, 2024

need to fix debian tests.

test/system/helpers.bash Outdated Show resolved Hide resolved
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@edsantiago
Copy link
Collaborator

LGTM apart from the non-namespaced envariable. I won't block over that.

@lsm5 lsm5 added the 5.0 label Feb 5, 2024
@lsm5
Copy link
Member Author

lsm5 commented Feb 5, 2024

LGTM apart from the non-namespaced envariable. I won't block over that.

Thanks. That was intentional cause we wanna do similar for buildah

@edsantiago
Copy link
Collaborator

Repeating my comment, which may have been missed:

I have pretty strong feelings about namespace pollution. Someone may add this to their rc files, then a year later have no recollection what it's for. May I encourage us all to rethink this? I would really prefer PODMAN_etc, BUILDAH_etc, SKOPEO_etc. It's more annoying but for something like this annoying is good.

Comment on lines 156 to 159
# FIXME: Temporary until podman fully removes cgroupsv1 support; see #21431
if [[ -n "$IGNORE_CGROUPSV1_WARNING" ]]; then
echo "Environment=IGNORE_CGROUPSV1_WARNING=1" >>$quadlet_file
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, so much for that idea. I guess quadlet sends this to the container itself, and the podman command never sees it. Back to the drawing board.

Copy link
Member

Choose a reason for hiding this comment

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

you would need to set the environment section under the systemd [Service] section not [Container] as this maps to --env which of course does not effect podman itself.

@lsm5
Copy link
Member Author

lsm5 commented Feb 5, 2024

Repeating my comment, which may have been missed:

I have pretty strong feelings about namespace pollution. Someone may add this to their rc files, then a year later have no recollection what it's for. May I encourage us all to rethink this? I would really prefer PODMAN_etc, BUILDAH_etc, SKOPEO_etc. It's more annoying but for something like this annoying is good.

alright, I can update the PR.

@edsantiago
Copy link
Collaborator

Please wait for a solution to the quadlet problem

@edsantiago
Copy link
Collaborator

Many rabbit holes later, I think the best solution is:

diff --git a/test/system/252-quadlet.bats b/test/system/252-quadlet.bats
index 5a4ecf775..69c55fb10 100644
--- a/test/system/252-quadlet.bats
+++ b/test/system/252-quadlet.bats
@@ -155,7 +155,7 @@ EOF
 
     # FIXME: Temporary until podman fully removes cgroupsv1 support; see #21431
     if [[ -n "$IGNORE_CGROUPSV1_WARNING" ]]; then
-        echo "Environment=IGNORE_CGROUPSV1_WARNING=1" >>$quadlet_file
+        skip "Way too complicated to test under cgroupsv1, and not worth the effort"
     fi
 
     run_quadlet "$quadlet_file"

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
Podman v5 will not support cgroups-v1. This commit will print a warning
if it detects a cgroups-v1 system. The warning can be hidden by setting
envvar `PODMAN_CGROUPSV1_WARNING`.

This warning is patched out for RHEL 9 builds as cgroups-v1 will still
be supported on RHEL 9 systems.

Resolves: https://issues.redhat.com/browse/RUN-1957

[NO NEW TESTS NEEDED]

Co-authored-by: Ed Santiago <santiago@redhat.com>
Co-authored-by: Sascha Grunert <sgrunert@redhat.com>
Co-authored-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2024
@lsm5
Copy link
Member Author

lsm5 commented Feb 6, 2024

@ashley-cui @cevich mind checking out the machine-mac failure before I go rerunning it again?

@ashley-cui
Copy link
Member

@lsm5 expected to fail for now, this should be good to go

@edsantiago
Copy link
Collaborator

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2024
@lsm5
Copy link
Member Author

lsm5 commented Feb 6, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit f439f4e into containers:main Feb 6, 2024
88 of 89 checks passed
@lsm5 lsm5 deleted the cgroupv1-warn branch February 6, 2024 14:27
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 7, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet