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

make: allow setting the default network locking backend #2583

Open
wants to merge 7 commits into
base: criu-dev
Choose a base branch
from

Conversation

adrianreber
Copy link
Member

@adrianreber adrianreber commented Feb 4, 2025

This adds the possibility to select a network locking backend during build without the need to change the sources.

It also introduces a test run with iptables uninstalled only relying on nftables.

As different Linux distributions are switching away from iptables
to nftables, this makes it easier to compile CRIU with a different
default network locking backend. Instead of changing the source
code it is now possible to select the nft backend like this:

    make NETWORK_LOCK_DEFAULT=NETWORK_LOCK_NFTABLES

Signed-off-by: Adrian Reber <[email protected]>
@adrianreber adrianreber force-pushed the 2025-02-04-nftables-make branch 8 times, most recently from 3b74eb1 to c359297 Compare February 4, 2025 18:50
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
The building section also contains the information how to change the
network locking backend without source code changes.

Signed-off-by: Adrian Reber <[email protected]>
This uses the same formatting for the make command examples as seen in
README.md.

Signed-off-by: Adrian Reber <[email protected]>
@adrianreber adrianreber force-pushed the 2025-02-04-nftables-make branch 6 times, most recently from 15df0a2 to ac55630 Compare February 5, 2025 15:50
The tests in others/rpc are running as non-root and
fail silently if the nftables network locking backend is used.

This switches those tests to skip the network locking.

Signed-off-by: Adrian Reber <[email protected]>
If the tests in others/rpc are failing no information about that error
can be seen in a CI run. This change displays the log files if the test
fails.

Signed-off-by: Adrian Reber <[email protected]>
There are a couple of tests that require the iptables binary.

Instead of adding a checkskip script, which could also handle this,
this change now uses CRIU's feature detection to see if the CRIU
feature 'has_ipt_legacy' exists.

Signed-off-by: Adrian Reber <[email protected]>
@adrianreber adrianreber force-pushed the 2025-02-04-nftables-make branch from ac55630 to 7d5788c Compare February 6, 2025 10:00
@adrianreber adrianreber force-pushed the 2025-02-04-nftables-make branch from 7d5788c to b9f4785 Compare February 6, 2025 11:10
@adrianreber adrianreber marked this pull request as ready for review February 6, 2025 12:09
@adrianreber
Copy link
Member Author

@mihalicyn That adds a compile time option to easily switch between backends.

@adrianreber
Copy link
Member Author

@danishprakash this is a PR which should help you building RPMs with a different network locking backend.

@danishprakash
Copy link

Thanks for this @adrianreber, this is quite helpful.


I remember we had this discussion in containers/podman#24799 but if we were to make criu "intelligent" and have it figure out the network locking backend on its own, what issues could we have encountered?

* It is, however, possible to change this by defining
* NETWORK_LOCK_DEFAULT to a different value on the command-line.
*/
#ifndef NETWORK_LOCK_DEFAULT
#define NETWORK_LOCK_DEFAULT NETWORK_LOCK_IPTABLES
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 change the default to nftable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. Currently Fedora >= 42 defaults to nft and CentOS Stream >= 10 (RHEL >= 10).

@danishprakash mentioned that Suse is also looking into switching to nft. Not sure what other distributions are planing.

Maintaining the RHEL and Fedora packages it would make my life a bit easier, but I still need to deal with older Fedora and RHEL releases. It feels a bit too early. Maybe if Debian or Ubuntu also switches to nft, the we would have most of the bigger distributions.

@avagin avagin requested a review from mihalicyn February 13, 2025 05:17
@avagin
Copy link
Member

avagin commented Feb 13, 2025

LGTM. Thanks!

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.

4 participants