-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add Podman v5 breaking changes checks. #2818
Conversation
d3bc418
to
82adb54
Compare
Thanks, I think this is a good start. Now, before going further and refining this we need to do two things:
Once we have that info, let's write up a nice comment in e.g. coreos/fedora-coreos-tracker#1629 that we can then link to from these issue dropins. |
Also, note we'll need a systemd service to actually run the script. |
82adb54
to
769957c
Compare
53861f2
to
675c504
Compare
I see that we have a script coreos-check-cgroups that already checks and warns the users who use cgroupv1. |
a779112
to
c7f2ae7
Compare
# Netavark is the default network backend and was added in | ||
# Podman version 4.0. CNI is deprecated and is removed | ||
# in Podman version 5.0, in preference of Netavark. | ||
podmanBackend=$(podman info --format "{{.Host.NetworkBackend}}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to somehow not run this check if the users are using docker and not podman. Just running the podman
or docker
commands on systems that are using the other runtime can cause issues.
@jlebon - do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure but I would think podman info
at least would be safe regardless. The larger point of only warning if podman is used makes sense, though I'm not sure there's a reliable way to know which runtime is in use. E.g. just checking for running containers and not finding any at that moment in time doesn't necessarily mean it's not in use.
If podman info
is safe (/cc @mheon), then implicitly it's checking for the "in use" bit as well since it'll only return something other than the netavark default if the machine has state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think running any podman
command does make changes to the system. @mheon would know better than I.
One thing we could try to do is look under certain known directories to see if files have been created there and assume podman is in use if some files exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big issue is usually conflicts in firewall rules, and that only happens if a container is started. podman info
on a Docker system should not be a problem.
I think we should keep the checks separate (i.e. a new check for netavark) BUT we should somehow rename the cgroups check so that if they previously ignored the warning by running We should also update the text of the cgroup check to reflect the current state. |
# Netavark is the default network backend and was added in | ||
# Podman version 4.0. CNI is deprecated and is removed | ||
# in Podman version 5.0, in preference of Netavark. | ||
podmanBackend=$(podman info --format "{{.Host.NetworkBackend}}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not sure but I would think podman info
at least would be safe regardless. The larger point of only warning if podman is used makes sense, though I'm not sure there's a reliable way to know which runtime is in use. E.g. just checking for running containers and not finding any at that moment in time doesn't necessarily mean it's not in use.
If podman info
is safe (/cc @mheon), then implicitly it's checking for the "in use" bit as well since it'll only return something other than the netavark default if the machine has state.
podmanBackend=$(podman info --format "{{.Host.NetworkBackend}}") | ||
|
||
if [[ $podmanBackend != "netavark" ]]; then | ||
echo -e "${warn} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just echoing, we need to generate an motd.
########################################################################### | ||
WARNING: This system is using CNI networking. CNI is deprecated and will be | ||
removed in the upcoming Podman v5.0, in preference of Netavark. To switch | ||
from CNI networking to netavark, you must run 'podman system reset --force' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Inconsistent capitalization. Also let's maybe put the command on its own line.
WARNING: This system is using CNI networking. CNI is deprecated and will be | ||
removed in the upcoming Podman v5.0, in preference of Netavark. To switch | ||
from CNI networking to netavark, you must run 'podman system reset --force' | ||
command. This will delete all of your images,containers, and custom networks.${nc}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
command. This will delete all of your images,containers, and custom networks.${nc}" | |
command. This will delete all of your images, containers, and custom networks.${nc}" |
Should we also update the warning to show how to disable this service if needed for eg. add |
I think it depends on if |
if [[ $cgroupVersion == "tmpfs" ]]; then | ||
echo -e "${warn} | ||
########################################################################## | ||
WARNING: This system is using cgroups v1. Podman is dropping support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update here: We have formally deprecated CGv1, but are not actually removing until v6.0 (I have no idea when that will come out, but at least a year from now). There are too many stragglers still on v1 (including WSL2), for us to completely remove at this point. So Podman will still work on v1 systems, but will pop up an annoying warning when you do so unless silenced by environment variable.
fa5e27c
to
9026b5b
Compare
# CNI networking. If so, they will be warned to move | ||
# their nodes to netavark respectively. | ||
[Unit] | ||
Description=Check if nodes are still using CNI networking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: this should be Title Case
podmanBackend=$(podman info --format "{{.Host.NetworkBackend}}") | ||
|
||
if [[ $podmanBackend != "netavark" ]]; then | ||
motd_path=/run/motd.d/35_cni_warning.motd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: weird indentation level here. Let's do 2 or 4?
WARNING: This system is using CNI networking. CNI is deprecated and will be | ||
removed in the upcoming Podman v5.0, in preference of netavark. To switch | ||
from CNI networking to netavark, you must run 'podman system reset --force' | ||
command. This will delete all of your images, containers, and custom networks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing ###
divider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should mention that "depending on your setup it may be preferable to reprovision the whole machine from the latest images". because reprovisioning will set things up the right way too.
removed in the upcoming Podman v5.0, in preference of netavark. To switch | ||
from CNI networking to netavark, you must run 'podman system reset --force' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Netavark (I think? Based on upstream docs, it seems like it's capitalized when talking about the project.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will podman system reset --force
delete your existing containers? If so, maybe we should have a FAQ entry in our docs we should point people to instead for more context so they can make the right decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I see line 27 below now. :(
cat << EOF > "${motd_path}" | ||
${warn} | ||
########################################################################### | ||
WARNING: This system is using CNI networking. CNI is deprecated and will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: This system is using CNI networking. CNI is deprecated and will be | |
WARNING: Podman is using CNI networking. CNI is deprecated and will be |
from CNI networking to netavark, you must run 'podman system reset --force' | ||
command. This will delete all of your images, containers, and custom networks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. whether to show instructions to disable the warning, I think we probably always should provide them. Users should always be free to disregard warnings if they know why it doesn't apply to them.
to use cgroups v2, see: | ||
########################################################################## | ||
WARNING: This system is using cgroups v1. Podman has dropped support for | ||
cgroups v1. Move your nodes to cgroups v2 if not already. For instructions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be adjusted given #2818 (comment). We could delay warning it until when Podman v6.0 arrives, but meh probably good to give an early heads up.
I think also we shouldn't lose the point about it benefiting the whole system, not just Podman.
OK, how about something like this:
This system is using cgroups v1. For increased reliability
it is strongly recommended to migrate this system and your workloads
to use cgroups v2. A future version of Podman will also drop support
for cgroups v1. For instructions on how to adjust kernel arguments
to use cgroups v2, see:
manifests/fedora-coreos.yaml
Outdated
@@ -36,6 +36,7 @@ conditional-include: | |||
|
|||
ostree-layers: | |||
- overlay/15fcos | |||
- overlay/14container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this overlay something clearer, e.g. 14podman-v5
. And add a README to document it. Also, let's make it conditional on Fedora 39? It doesn't hurt on Fedora 40 (we just will never warn since podman can't support anything other than Netavark), but it makes it clearer when we can drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just confirming, we don't really need it to be conditional right? Aiming to keep it and update the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it to be conditional, but we also don't need it to be in Fedora 40. What I mean is making it conditional will make it more likely to get cleaned up when we scan all the overlays we can drop when we rebase on top of f40.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything alarming. I added some comments.
I'll trust @jlebon to review from here on out.
9026b5b
to
c34ec2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some superficial things, but LGTM overall. Thanks!
@@ -0,0 +1,36 @@ | |||
#!/usr/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing set -euo pipefail
WARNING: Podman is using CNI networking. CNI is deprecated and will be | ||
removed in the upcoming Podman v5.0, in preference of Netavark. To switch | ||
from CNI networking to Netavark, you must run 'podman system reset --force' | ||
command. This will delete all of your images, containers, and custom networks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, "command." looks weird here. Either e.g. "you must run the command '...'." or "you must run '...'.".
sudo systemctl disable coreos-check-cgroups.service | ||
############################################################################ | ||
sudo systemctl disable coreos-check-cgroups-version.service | ||
########################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor: this is longer than the first divider.
# CNI networking. If so, they will be warned to move | ||
# their nodes to netavark respectively. | ||
[Unit] | ||
Description=Check if Nodes Are Still Using CNI Networking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description=Check if Nodes Are Still Using CNI Networking | |
Description=Check If Podman Is Still Using CNI Networking |
Podman 5 will come with breaking changes affecting upgradability. CGroups v1 environments will be required to switch to CGroups v2 and CNI plugin environemnts will need to switch to netavark. Updated the existing cgroups-version check and added the check for CNI networking
c34ec2b
to
25b962a
Compare
Podman 5 will come with breaking changes affecting upgradability. CGroups v1 environments will be required to switch to CGroups v2 and CNI plugin environemnts will need to switch to netavark. In the above script we added the CLHM helpers to notifiy people of the incoming changes.
Ref: coreos/fedora-coreos-tracker#1629