T75: migrate from pmacct to ipt_NETFLOW by hedrok · Pull Request #4688 · vyos/vyos-1x

hedrok · 2025-08-31T17:34:44Z

Migrate from pmacct to ipt_NETFLOW

Change nft to iptables in system_flow-accounting.py as ipt_NETFLOW is iptales plugin
Remove specific and non-relevant pmacct options
Add ipt_NETFLOW options
Move 'interfaces' to 'netflow' tree
Support more flexible 'source-address' and 'source-interface' for each server instead of one source
Add migration script
Update op mode command 'show flow-accounting'

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

Related PR(s)

T75: ipt-netflow enable sampler, mac and vlan vyos-build#1022

Update of ipt_NETFLOW module to provide sampler, MAC address and VLAN recording.
This PR should work without that one too.

How to test / Smoketest result

I've update smoke test smoketest/scripts/cli/test_system_flow-accounting.py accordingly, output:

vyos@vyos-router:~$ /usr/libexec/vyos/tests/smoke/cli/test_system_flow-accounting.py TestSystemFlowAccounting
test_basic (__main__.TestSystemFlowAccounting.test_basic) ...
ok
test_iptables (__main__.TestSystemFlowAccounting.test_iptables) ...
ok
test_netflow (__main__.TestSystemFlowAccounting.test_netflow) ...
ok
test_sampler (__main__.TestSystemFlowAccounting.test_sampler) ...
ok

----------------------------------------------------------------------
Ran 4 tests in 16.765s

OK

(I've removed kernel messages from ipt_NETFLOW for clarity and brevity)

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation (https://docs.vyos.io/en/latest/configuration/system/flow-accounting.html)
I have updated the documentation accordingly

github-actions · 2025-08-31T17:34:55Z

👍
No issues in PR Title / Commit Title

github-actions · 2025-08-31T17:35:03Z

✅ No issues found in unused-imports check. Please refer the workflow run

Copilot

Pull Request Overview

This PR migrates VyOS flow accounting from pmacct to ipt_NETFLOW, changing the underlying implementation while maintaining similar functionality. The migration simplifies configuration by removing pmacct-specific options and introduces more flexible NetFlow server configuration with per-server source addressing.

Replaces pmacct daemon with ipt_NETFLOW kernel module for flow accounting
Moves interface configuration under the netflow tree structure
Updates operational commands to work with the new implementation

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/op_mode/flow_accounting_op.py	Replaces pmacct-based flow parsing with ipt_NETFLOW /proc interface parsing
src/migration-scripts/flow-accounting/2-to-3	Migrates configuration from pmacct format to ipt_NETFLOW format
src/conf_mode/system_flow-accounting.py	Replaces nftables/pmacct with iptables/ipt_NETFLOW configuration
smoketest/scripts/cli/test_system_flow-accounting.py	Updates tests for new ipt_NETFLOW implementation
interface-definitions/system_flow-accounting.xml.in	Removes pmacct options and restructures NetFlow configuration
data/templates/ipt-netflow/ipt_NETFLOW.conf.j2	New template for ipt_NETFLOW module configuration

Comments suppressed due to low confidence (1)

src/op_mode/flow_accounting_op.py:1

Using current_rule in the list comprehension should be current_rule_parsed since current_rule is a string but you're trying to access it by index. This should be range(min(len(current_rule_parsed), len(columns))).

#!/usr/bin/env python3

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-31T21:30:20Z

    elif re.match(r'^\d+,.*\d$', arg):
        # Port list
-        ports = re.split(r',+', arg) # This allows duplicate commad like '1,,2,3,4'
+        ports = re.split(r',+', arg) # This allows duplicate commas like '1,,2,3,4'


Fix typo in comment: 'commas' is misspelled as 'commad'.

Copilot · 2025-08-31T21:30:20Z

+                key, subkeys = h.split(':', 1)
+                headers[i] = {'key': key, 'subkeys': subkeys.split(',')}
+
+        headers.remove('type') # Somewhy this field has no value


Fix the comment: 'Somewhy' should be 'For some reason' or 'Somehow'.

Suggested change

headers.remove('type') # Somewhy this field has no value

headers.remove('type') # For some reason this field has no value

Copilot · 2025-08-31T21:30:21Z

+need_reload = True

-# get nftables rule dict for chain in table
-def _nftables_get_nflog(chain, table):
+


The global variable need_reload is a problematic pattern. Consider passing this state through function parameters or using a class-based approach to avoid global state management issues.

Copilot · 2025-08-31T21:30:21Z

+        if iface in configured_ifaces_extended:
            configured_ifaces_extended.remove(iface)


This condition will always be false because iface is from active_ifaces which contains dictionaries, while configured_ifaces_extended contains dictionaries. The comparison should check if iface is in active_ifaces instead, or the logic needs to be restructured.

Suggested change

if iface in configured_ifaces_extended:

configured_ifaces_extended.remove(iface)

# Remove any entry in configured_ifaces_extended whose 'iface' matches iface['iface']

configured_ifaces_extended[:] = [entry for entry in configured_ifaces_extended if entry['iface'] != iface['iface']]

sever-sever · 2025-09-01T09:24:02Z

      </node>
    </children>
  </node>
-  <node name="restart">


What about other op-mode commands?

vyos@r14:~$ show conf com | match flow set system flow-accounting netflow interface 'eth99' set system flow-accounting netflow interface 'eth1' set system flow-accounting netflow interface 'eth0' set system flow-accounting netflow sampling-rate '1' set system flow-accounting netflow server 127.0.0.1 set system flow-accounting netflow version '10' set system flow-accounting vrf 'red' set vrf name red table '1010' vyos@r14:~$ vyos@r14:~$ show flow-accounting Traceback (most recent call last): File "/usr/libexec/vyos/op_mode/flow_accounting_op.py", line 268, in <module> flows_list = _get_flows_list() ^^^^^^^^^^^^^^^^^ File "/usr/libexec/vyos/op_mode/flow_accounting_op.py", line 120, in _get_flows_list headers.remove('type') # Somewhy this field has no value ^^^^^^^^^^^^^^^^^^^^^^ ValueError: list.remove(x): x not in list vyos@r14:~$ vyos@r14:~$ vyos@r14:~$ vyos@r14:~$ show flow-accounting interface eth0 Traceback (most recent call last): File "/usr/libexec/vyos/op_mode/flow_accounting_op.py", line 268, in <module> flows_list = _get_flows_list() ^^^^^^^^^^^^^^^^^ File "/usr/libexec/vyos/op_mode/flow_accounting_op.py", line 120, in _get_flows_list headers.remove('type') # Somewhy this field has no value ^^^^^^^^^^^^^^^^^^^^^^ ValueError: list.remove(x): x not in list vyos@r14:~$

I've fixed the issue with show and tested it with current ipt_NETFLOW module.
I don't see how to implement 'clear' command with ipt_NETFLOW.
I'll implement restart...

I've implemented restart, restarting clears all counters. Should I make clear do exactly what restart does or just remove it?..

c-po · 2025-09-09T13:58:53Z

-    call('systemctl daemon-reload')
+    prev_config = ''
+    if os.path.exists(ipt_netflow_conf_path):
+        with open(ipt_netflow_conf_path) as f:


Please use vyos.utils.file.read_file()

Thank you, nice utility function, updated.

c-po · 2025-09-09T14:01:08Z

-            os.unlink(uacctd_conf_path)
+    new_config = ''
+    if os.path.exists(ipt_netflow_conf_path):
+        with open(ipt_netflow_conf_path) as f:


Please use vyos.utils.file.read_file()

dmbaturin

I left a few suggestions regarding help string wording. The logic seems good at a glance.

dmbaturin · 2025-09-15T10:31:53Z

+              </leafNode>
+              <leafNode name="active-timeout">
+                <properties>
+                  <help>Export flow after it's active for this timeout in seconds</help>


hedrok · 2025-09-15T11:38:08Z

I left a few suggestions regarding help string wording. The logic seems good at a glance.

Thanks, I've updated messages.

sever-sever · 2025-09-15T14:46:56Z

  ocserv,
 # End "vpn openconnect"
-# For "system flow-accounting"
-  pmacct (>= 1.6.0),


Do not forget to remove mentions about pmacct (after merging) in the vyos-build

Thanks you, I missed that!
Here is PR: vyos/vyos-build#1030
I'll make it non-draft when ipt_NETFLOW is merged.

Copilot

Pull Request Overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/op_mode/flow_accounting_op.py:89

There's a typo in the comment: 'dictioanry' should be 'dictionary'.

    # return dictioanry

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

* Change nft to iptables in system_flow-accounting.py as ipt_NETFLOW is iptales plugin * Remove specific and non-relevant pmacct options * Add ipt_NETFLOW options * Move 'interfaces' to 'netflow' tree * Support more flexible 'source-address' and 'source-interface' for each server instead of one source * Add migration script * Update op mode command 'show flow-accounting' * Update op mode command 'restart flow-accounting'

github-actions · 2025-09-16T12:06:46Z

CI integration ❌ failed!

Details

CI logs

CLI Smoketests (no interfaces) ❌ failed
CLI Smoketests VPP 👍 passed
CLI Smoketests (interfaces only) 👍 passed
Config tests 👍 passed
Config tests VPP 👍 passed
RAID1 tests 👍 passed
TPM tests 👍 passed

sever-sever

My internal test looks working fine.
Other could be fixed after merging.

dmbaturin

Let's get it into the rolling release image.

github-actions Bot added the current label Aug 31, 2025

github-actions Bot assigned hedrok Aug 31, 2025

sever-sever requested a review from Copilot August 31, 2025 21:29

Copilot AI reviewed Aug 31, 2025

View reviewed changes

sever-sever requested changes Sep 1, 2025

View reviewed changes

Comment thread smoketest/configs/bgp-azure-ipsec-gateway Outdated

Comment thread smoketest/configs/bgp-azure-ipsec-gateway Outdated

Comment thread smoketest/configs/bgp-big-as-cloud Outdated

Comment thread smoketest/configs/bgp-big-as-cloud

sever-sever requested review from c-po, dmbaturin and zdc September 1, 2025 01:29

hedrok force-pushed the T75-migration-to-ipt-netflow branch 2 times, most recently from ab956e7 to 331408c Compare September 1, 2025 07:44

sever-sever reviewed Sep 1, 2025

View reviewed changes

Comment thread src/conf_mode/system_flow-accounting.py Outdated

hedrok force-pushed the T75-migration-to-ipt-netflow branch from dfbdf42 to b04a0f7 Compare September 1, 2025 12:59

sever-sever self-requested a review September 1, 2025 22:46

hedrok force-pushed the T75-migration-to-ipt-netflow branch 2 times, most recently from 98282ed to cd59586 Compare September 2, 2025 12:47

c-po requested changes Sep 9, 2025

View reviewed changes

c-po reviewed Sep 9, 2025

View reviewed changes

hedrok force-pushed the T75-migration-to-ipt-netflow branch 2 times, most recently from 4cbac1a to 8ec9e2f Compare September 9, 2025 18:52

hedrok requested a review from c-po September 10, 2025 04:12

dmbaturin requested changes Sep 15, 2025

View reviewed changes

github-actions Bot added the rebase label Sep 15, 2025

hedrok force-pushed the T75-migration-to-ipt-netflow branch from f3bcc47 to 102fc58 Compare September 15, 2025 11:37

sever-sever reviewed Sep 15, 2025

View reviewed changes

hedrok mentioned this pull request Sep 15, 2025

T75: remove pmacct vyos/vyos-build#1030

Merged

11 tasks

sever-sever self-requested a review September 15, 2025 16:41

sever-sever requested review from Copilot and dmbaturin September 15, 2025 16:41

Copilot AI reviewed Sep 15, 2025

View reviewed changes

Comment thread src/op_mode/flow_accounting_op.py

Comment thread python/vyos/ipt_netflow.py

Comment thread python/vyos/ipt_netflow.py

hedrok force-pushed the T75-migration-to-ipt-netflow branch from 102fc58 to e992fb4 Compare September 16, 2025 10:01

sever-sever approved these changes Sep 16, 2025

View reviewed changes

dmbaturin approved these changes Sep 16, 2025

View reviewed changes

dmbaturin merged commit 8b0c9b7 into vyos:current Sep 16, 2025
17 of 18 checks passed

dmbaturin added the bp/circinus Create automatic backport for circinus label Sep 16, 2025

vyosbot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Sep 16, 2025

andamasov removed the bp/circinus Create automatic backport for circinus label May 9, 2026

	headers.remove('type') # Somewhy this field has no value
	headers.remove('type') # For some reason this field has no value

		if iface in configured_ifaces_extended:
		configured_ifaces_extended.remove(iface)

Uh oh!

Conversation

hedrok commented Aug 31, 2025

Migrate from pmacct to ipt_NETFLOW

Types of changes

Related Task(s)

Related PR(s)

How to test / Smoketest result

Checklist:

Uh oh!

github-actions Bot commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmbaturin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedrok commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 16, 2025

Details

Uh oh!

sever-sever left a comment

Choose a reason for hiding this comment

Uh oh!

dmbaturin left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 31, 2025 •

edited

Loading

github-actions Bot commented Aug 31, 2025 •

edited

Loading