Soft nftables reload support#108
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in "soft reload" mode that, when a previous pbr ruleset is already installed, applies only the dynamic portion of the generated nft script to the live ruleset and rewrites pkg.nft_main_file without invoking fw4 -q reload, avoiding the brief connectivity gap caused by a full fw4 reload. The structural prefix of the generated nft script is now tracked via nft_structural_end, a new pbr_chains cleanup action flushes only the pbr-prefixed chains, and the sets cleanup case is reworked (incidentally fixing a pre-existing split-delimiter bug). A new nft_soft_reload boolean is added to the config schema and the default UCI file (off by default).
Changes:
- Track a structural-section boundary in the generated nft script and add a soft-apply branch in
nft.install()gated bycfg.nft_soft_reloadand presence of a previously installed jump. - Add
pbr_chainscleanup case and rewritesetscleanup to flush+delete using space-split parsing; wire soft-reload cleanup actions inpbr.uc. - Add
nft_soft_reloadboolean toconfig.ucschema and defaultfiles/etc/config/pbr(default0).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| files/lib/pbr/nft.uc | Adds soft-reload install path, structural-end marker, pbr_chains cleanup, reworks sets cleanup; significant duplication and a few correctness concerns. |
| files/lib/pbr/pbr.uc | Branches the reload cleanup action list on cfg.nft_soft_reload. |
| files/lib/pbr/config.uc | Declares the new nft_soft_reload boolean option (default false). |
| files/etc/config/pbr | Adds option nft_soft_reload '0' to the default config. |
Comments suppressed due to low confidence (2)
files/lib/pbr/nft.uc:704
- In soft-reload mode,
cleanup('...', 'sets')still falls through to thecase 'sets':block, which performs both aflushand adeleteon every pbr set. Deleting sets defeats the purpose of soft reload: rules currently loaded that reference a set will either prevent the delete (leaving stale sets that then conflict withadd set ...in the new ruleset) or, if delete succeeds, the references in the still-installed jumps break briefly until the soft-applied ruleset re-adds them. Soft reload should only flush set contents (so they can be repopulated by the new ruleset) and skip the delete step. Consider introducing a separate cleanup action (e.g.sets_flush) used by the soft path, or branching oncfg.nft_soft_reloadinside the case.
case 'sets': {
let nft_sets_str = get_nft_sets();
if (nft_sets_str) {
let sets_list = split(nft_sets_str, ' ');
for (let s in sets_list) {
s = trim(s);
if (s)
nft_call('flush', 'set', 'inet', pkg.nft_table, s);
}
for (let s in sets_list) {
s = trim(s);
if (s)
nft_call('delete', 'set', 'inet', pkg.nft_table, s);
}
}
break;
}
files/lib/pbr/nft.uc:310
- In the soft-reload path, on success only
output.okn()is called —output.info/output.verboseare no longer told this was a soft (live) apply versus a full reload. For an operations feature that explicitly changes behavior in a non-obvious way, please surface the distinction (e.g. log "soft" vs "full" reload) so support diagnostics can tell which code path executed without enabling debug instrumentation.
if (nft_call('-f', pkg.nft_temp_file)) {
writefile(pkg.nft_main_file, full_content);
output.okn();
return true;
} else {
push(state.errors, { code: 'errorNftMainFileInstall', info: pkg.nft_temp_file });
output.failn();
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let nft_sets_str = get_nft_sets(); | ||
| if (nft_sets_str) { | ||
| let sets_list = split(nft_sets_str, '\n'); | ||
| let sets_list = split(nft_sets_str, ' '); |
There was a problem hiding this comment.
I appreciate this fix and I'd be much obliged if you submit this on its own.
| if (cfg.nft_soft_reload && nft_structural_end > 0) { | ||
| let chain_exists = sh.run('nft list chain inet ' + pkg.nft_table + ' mangle_prerouting 2>/dev/null | grep -q ' + sh.quote('jump ' + pkg.nft_prefix + '_prerouting')) == 0; | ||
| if (chain_exists) { | ||
| let soft_lines = slice(nft_lines, nft_structural_end); | ||
| writefile(pkg.nft_temp_file, '#!/usr/sbin/nft -f\n\n' + join('\n', soft_lines) + '\n'); | ||
| if (nft_call('-f', pkg.nft_temp_file)) { | ||
| writefile(pkg.nft_main_file, full_content); | ||
| output.okn(); | ||
| return true; | ||
| } else { | ||
| push(state.errors, { code: 'errorNftMainFileInstall', info: pkg.nft_temp_file }); | ||
| output.failn(); | ||
| return false; | ||
| } | ||
| } else { | ||
| if (sh.run('cp -f ' + sh.quote(pkg.nft_temp_file) + ' ' + sh.quote(pkg.nft_main_file)) == 0) { | ||
| output.okn(); | ||
| sh.run('fw4 -q reload'); | ||
| return true; | ||
| } else { | ||
| push(state.errors, { code: 'errorNftMainFileInstall', info: pkg.nft_temp_file }); | ||
| output.failn(); | ||
| return false; | ||
| } | ||
| } | ||
| } else { | ||
| if (sh.run('cp -f ' + sh.quote(pkg.nft_temp_file) + ' ' + sh.quote(pkg.nft_main_file)) == 0) { | ||
| output.okn(); | ||
| sh.run('fw4 -q reload'); | ||
| return true; | ||
| } else { | ||
| push(state.errors, { code: 'errorNftMainFileInstall', info: pkg.nft_temp_file }); | ||
| output.failn(); | ||
| return false; | ||
| } | ||
| } |
| return false; | ||
| } | ||
| if (cfg.nft_soft_reload && nft_structural_end > 0) { | ||
| let chain_exists = sh.run('nft list chain inet ' + pkg.nft_table + ' mangle_prerouting 2>/dev/null | grep -q ' + sh.quote('jump ' + pkg.nft_prefix + '_prerouting')) == 0; |
| if (nft_call('-f', pkg.nft_temp_file)) { | ||
| writefile(pkg.nft_main_file, full_content); | ||
| output.okn(); | ||
| return true; |
| nft_set_counter: ['bool', false], | ||
| nft_set_flags_timeout: ['bool', false], | ||
| nft_user_set_counter: ['bool', false], | ||
| netifd_enabled: ['bool', false], |
| push(nft_lines, 'add rule inet ' + nft_table + ' mangle_output jump ' + nft_prefix + '_output'); | ||
| push(nft_lines, 'add rule inet ' + nft_table + ' mangle_forward jump ' + nft_prefix + '_forward'); | ||
| push(nft_lines, ''); | ||
| nft_structural_end = length(nft_lines); |
|
What's the use case for soft reload? |
|
Run the “ping 1.1.1.1” command on a device connected to a router with PBR. Then restart the PBR. You will see that packet transmission is interrupted for a moment. The same thing happens when you add or modify any rules. If you add another IP address to the list, the entire table will be regenerated, causing the internet connection to drop again. This is particularly critical for games and servers. |
|
Use pbr in netifd/mwan4 mode and/or disable Please let me know if you'd want to create PR for the nftset.cleanup() function fix separately. |
|
Added nftset.cleanup(new_sets) that only removes sets no longer needed. Unchanged sets are left alone, avoiding the flush/delete/recreate cycle that disrupts connections. Why this is better Instead of switching modes or disabling safety features:
Backward compatible, cleanup('sets') still works the same. |
|
I think your problem might be caused by the |
|
I already have strict enforcement disabled, but that doesn't help. The internet still goes down for a few seconds while the tables are being generated. My |
|
I did a test my self and did not experience any loss of connection when a non related interfaces goes up and down if strict enforcement is disabled. PBR 1.2.3-61 If strict enforcement is enabled and wg interface disconnection is simulated with |
|
I fixed the issue with gateway detection. Now I think my pull request is ready for full review. Earlier, I shared a link to a YouTube video demonstrating how PBR works without "Soft nftables," and you can see there that using While I was writing the code, I also noticed issue #109, which had already been fixed. I've updated my code to match PR #109. |
Right now, PBR reloads all tables from scratch, which kills the internet connection for a few seconds. Added a soft reload feature to keep things running smoothly during restarts.