-
Notifications
You must be signed in to change notification settings - Fork 585
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
utils: suppress errors on missing legacy iptables #1976
base: criu-dev
Are you sure you want to change the base?
Conversation
f1fad20
to
dd1ca2c
Compare
This looks like a correct change. The output says |
0827c6e
to
a0bed14
Compare
criu/util.c
Outdated
int exit_status = WEXITSTATUS(status); | ||
/* Don't print error message for ENOENT (No such file or directory) | ||
* when CRS_CAN_ENOENT flag has been set. */ | ||
if (exit_status != ENOENT || !(flags & CRS_CAN_ENOENT)) |
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 isn't reliable. ENOENT is 2 and the tool that is executed can return 2, it isn't so unusual. I don't like all these jumps with iptable tools, can we use linux API to check whether iptable-s are empty or not? I don't ask to dump iptables, I mean just to check whether we need to dump them or not.
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.
@mihalicyn could you help with this?
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.
@mihalicyn @avagin Do you have any advice on what changes I should make to address this comment, or should I close this pull request?
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.
@rst0git you should not close this pr.
@mihalicyn, pls try to find time to look at this.
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.
yep, friends, I've started looking into this.
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.
@avagin @mihalicyn It seems that reading /proc/net/ip(6)_tables_names
is enough to get info about existance of legacy rules in current netns:
in kernel: https://github.com/torvalds/linux/blob/0326074ff4652329f2a1a9c8685104576bd8d131/net/netfilter/x_tables.c#L1832
in iptables: https://git.netfilter.org/iptables/tree/iptables/nft-shared.c#n1460
@rst0git So we basically need to go over all netns-es of container and read these procfs files and check they are empty, to be sure that we don't need legacy iptables binary to save them.
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.
Good catch @Snorch! It works as a detector if tables are present or not, but unfortunately, it doesn't allow us to check if these tables are empty or not. Example:
root# cat /proc/net/ip_tables_names
root# iptables-legacy -L
Chain INPUT (policy ACCEPT)
target prot opt source destination
Chain FORWARD (policy ACCEPT)
target prot opt source destination
Chain OUTPUT (policy ACCEPT)
target prot opt source destination
root# cat /proc/net/ip_tables_names
filter
So, iptables-legacy
even with the -L
flag created the default struct xt_table
with the name filter
.
BTW, we can use getsockopt(fd, SOL_IP, {IPT,IP6T,EBT}_SO_GET_ENTRIES, ...)
to check if these tables are empty or not.
I'm not sure if we want to be so precise here or if we just want to fail migration if the file /proc/net/ip(6)_tables_names
is not empty and the iptables-legacy
binary isn't present. What do you think?
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've discussed this with Pavel and agreed that it seems sufficient just to check that the {ip,ip6}_tables_names
files are empty in all net namespaces for now.
f392ea1
to
4d137b8
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## criu-dev #1976 +/- ##
============================================
- Coverage 70.51% 70.48% -0.03%
============================================
Files 133 133
Lines 33534 33546 +12
============================================
Hits 23646 23646
- Misses 9888 9900 +12 ☔ View full report in Codecov by Sentry. |
When the legacy iptables backend is not installed, iptables-legacy-save and ip6tables-legacy-save binary files are missing and this results in the following error messages: (00.062021) iptables has nft backend: iptables-save v1.8.8 (nf_tables) Error (criu/util.c:626): execvp("iptables-legacy-save", ...) failed: No such file or directory (00.062793) Error (criu/util.c:641): exited, status=1 (00.062800) Error (criu/util.c:1566): iptables-legacy-save -V failed (00.069758) iptables has nft backend: ip6tables-save v1.8.8 (nf_tables) Error (criu/util.c:626): execvp("ip6tables-legacy-save", ...) failed: No such file or directory (00.070615) Error (criu/util.c:641): exited, status=1 (00.070624) Error (criu/util.c:1566): ip6tables-legacy-save -V failed (00.070632) skipping iptables dump - no legacy version present (00.070635) skipping ip6tables dump - no legacy version present The error messages "No such file or directory" can be ignored. This patch updates the get_legacy_iptables_bin() to check if the /proc/net/ip(6)_tables_names file is empty before trying to run iptables-legacy. Signed-off-by: Radostin Stoyanov <[email protected]>
When the legacy iptables backend is not installed, iptables-legacy-save and ip6tables-legacy-save binaries are missing. This results in the following error messages:
However, these messages should not be errors and can be ignored. With the changes in this pull request, the following messages will be included in the logs instead.