[ovs-dev] [PATCH v2] ovs-tcpdump: Support vlan option.
When I try filter geneve protocol with a vlan, the warning message
occurs that tell me the kernel can't support this combination.
$ ovs-tcpdump -i eth2 -nne vlan 10 and geneve
Warning: Kernel filter failed: Invalid argument
The fix is to make a convenience argument that sets the mirror port's
select_vlan column, and also marks the port as native tagged with VLAN.
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 35 +++
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index eada803bb..f2fb2f04b 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -142,6 +142,8 @@ The following options are available:
--mirror-toThe name for the mirror port to use (optional)
Default 'miINTERFACE'
--span If specified, mirror all ports (optional)
+ --vlan If specified, mirror vlan traffic and pop
+ its tag (optional)
""" % {'prog': sys.argv[0]})
sys.exit(0)
@@ -319,7 +321,7 @@ class OVSDB(object):
(mirror_name, txn.get_error()))
self._txn = None
-def make_port(self, port_name, bridge_name):
+def make_port(self, port_name, bridge_name, vlan=None):
iface_row = self.make_interface(port_name, False)
txn = self._txn
@@ -330,6 +332,10 @@ class OVSDB(object):
port = txn.insert(self.get_table('Port'))
port.name = port_name
+if vlan is not None:
+port.verify('tag')
+port.tag = [vlan]
+
br.verify('ports')
ports = getattr(br, 'ports', [])
ports.append(port)
@@ -354,7 +360,7 @@ class OVSDB(object):
return result
def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
- mirror_select_all=False):
+ mirror_select_all=False, mirrored_vlan=None):
txn = self._start_txn()
mirror = txn.insert(self.get_table('Mirror'))
@@ -374,6 +380,12 @@ class OVSDB(object):
src_port.append(mirrored_port)
mirror.select_src_port = src_port
+if mirrored_vlan:
+mirror.verify('select_vlan')
+select_vlan = getattr(mirror, 'select_vlan', [])
+select_vlan.append(mirrored_vlan)
+mirror.select_vlan = select_vlan
+
output_port = self._find_row_by_name('Port', mirror_intf_name)
mirror.verify('output_port')
@@ -440,6 +452,7 @@ def main():
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
interface = None
tcpdargs = []
+vlan = None
skip_next = False
mirror_interface = None
@@ -474,12 +487,25 @@ def main():
elif cur in ['--span']:
mirror_select_all = True
continue
+elif cur in ['--vlan']:
+vlan = nxt
+skip_next = True
+continue
tcpdargs.append(cur)
if interface is None:
print("Error: must at least specify an interface with '-i' option")
sys.exit(1)
+if vlan:
+try:
+vlan = int(vlan)
+if vlan < 0 or vlan > 4095:
+raise ValueError("out of range")
+except ValueError:
+print("Error: vlan must be within <0-4095>")
+sys.exit(1)
+
if not py_which(dump_cmd):
print("Error: unable to execute '%s' (check PATH)" % dump_cmd)
sys.exit(1)
@@ -523,10 +549,11 @@ def main():
teardown(db_sock, interface, mirror_interface, tap_created)
try:
-ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
+ovsdb.make_port(mirror_interface,
+ovsdb.port_bridge(interface), vlan)
ovsdb.bridge_mirror(interface, mirror_interface,
ovsdb.port_bridge(interface),
-mirror_select_all)
+mirror_select_all, vlan)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
sys.exit(1)
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs-tcpdump: Support vlan option.
> 2024年3月30日 上午2:43，Aaron Conole 写道：
>
> Daniel Ding mailto:danieldin...@gmail.com>> writes:
>
>> When I try filter geneve protocol with a vlan, the warning message
>> occurs that tell me the kernel cann't support this combination.
> ^ can't
>>
>> $ ovs-tcpdump -i eth2 -nne vlan 10 and geneve
>> Warning: Kernel filter failed: Invalid argument
>>
>> So I fix it by the following:
>> 1. the mirror-to interface was added with a vlan tag, which let
>> datapath to pop its tag.
>> 2. the traffic will be mirrored with mirror's select_vlan, and that
>> don't care about will not be received on the mirror-to interface.
>
> Maybe rephrase:
>
> "The fix is to make a convenience argument that sets the mirror port's
> select_vlan column, and also marks the port as native tagged with VLAN."
Thanks you, Aaron.
I have already updated this description in the following patch.
>
>> Signed-off-by: Daniel Ding
>> ---
>> utilities/ovs-tcpdump.in | 37 +
>> 1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
>> index eada803bb..b2b69d3c4 100755
>> --- a/utilities/ovs-tcpdump.in
>> +++ b/utilities/ovs-tcpdump.in
>> @@ -142,6 +142,8 @@ The following options are available:
>>--mirror-toThe name for the mirror port to use (optional)
>> Default 'miINTERFACE'
>>--span If specified, mirror all ports (optional)
>> + --vlan If specified, mirror a vlan traffic and pop
> s/ a//
>
>> + its tag (optional)
>> """ % {'prog': sys.argv[0]})
>> sys.exit(0)
>>
>> @@ -319,7 +321,7 @@ class OVSDB(object):
>> (mirror_name, txn.get_error()))
>> self._txn = None
>>
>> -def make_port(self, port_name, bridge_name):
>> +def make_port(self, port_name, bridge_name, vlan=None):
>> iface_row = self.make_interface(port_name, False)
>> txn = self._txn
>>
>> @@ -330,6 +332,12 @@ class OVSDB(object):
>> port = txn.insert(self.get_table('Port'))
>> port.name = port_name
>>
>> +if vlan is not None:
>> +port.verify('tag')
>> +tag = getattr(port, 'tag', [])
>> +tag.append(vlan)
>> +port.tag = tag
>> +
>
> Can a port have multiple tags set for vlan? I was under the impression
> that only one native tag was allowed per-port, but maybe I'm wrong.
>
Agree and updated the patch to ensure one native tag.
>> br.verify('ports')
>> ports = getattr(br, 'ports', [])
>> ports.append(port)
>> @@ -354,7 +362,7 @@ class OVSDB(object):
>> return result
>>
>> def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
>> - mirror_select_all=False):
>> + mirror_select_all=False, mirrored_vlan=None):
>>
>> txn = self._start_txn()
>> mirror = txn.insert(self.get_table('Mirror'))
>> @@ -374,6 +382,12 @@ class OVSDB(object):
>> src_port.append(mirrored_port)
>> mirror.select_src_port = src_port
>>
>> +if mirrored_vlan:
>> +mirror.verify('select_vlan')
>> +select_vlan = getattr(mirror, 'select_vlan', [])
>> +select_vlan.append(mirrored_vlan)
>> +mirror.select_vlan = select_vlan
>> +
>> output_port = self._find_row_by_name('Port', mirror_intf_name)
>>
>> mirror.verify('output_port')
>> @@ -440,6 +454,7 @@ def main():
>> db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
>> interface = None
>> tcpdargs = []
>> +vlan = None
>>
>> skip_next = False
>> mirror_interface = None
>> @@ -474,12 +489,25 @@ def main():
>> elif cur in ['--span']:
>> mirror_select_all = True
>> continue
>> +elif cur in ['--vlan']:
>> +vlan = nxt
>> +skip_next = True
>> +continue
>> tcpdargs.append(cur)
>>
>> if inter
[ovs-dev] [PATCH] ovs-tcpdump: Support vlan option.
When I try filter geneve protocol with a vlan, the warning message
occurs that tell me the kernel cann't support this combination.
$ ovs-tcpdump -i eth2 -nne vlan 10 and geneve
Warning: Kernel filter failed: Invalid argument
So I fix it by the following:
1. the mirror-to interface was added with a vlan tag, which let
datapath to pop its tag.
2. the traffic will be mirrored with mirror's select_vlan, and that
don't care about will not be received on the mirror-to interface.
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 37 +
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index eada803bb..b2b69d3c4 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -142,6 +142,8 @@ The following options are available:
--mirror-toThe name for the mirror port to use (optional)
Default 'miINTERFACE'
--span If specified, mirror all ports (optional)
+ --vlan If specified, mirror a vlan traffic and pop
+ its tag (optional)
""" % {'prog': sys.argv[0]})
sys.exit(0)
@@ -319,7 +321,7 @@ class OVSDB(object):
(mirror_name, txn.get_error()))
self._txn = None
-def make_port(self, port_name, bridge_name):
+def make_port(self, port_name, bridge_name, vlan=None):
iface_row = self.make_interface(port_name, False)
txn = self._txn
@@ -330,6 +332,12 @@ class OVSDB(object):
port = txn.insert(self.get_table('Port'))
port.name = port_name
+if vlan is not None:
+port.verify('tag')
+tag = getattr(port, 'tag', [])
+tag.append(vlan)
+port.tag = tag
+
br.verify('ports')
ports = getattr(br, 'ports', [])
ports.append(port)
@@ -354,7 +362,7 @@ class OVSDB(object):
return result
def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
- mirror_select_all=False):
+ mirror_select_all=False, mirrored_vlan=None):
txn = self._start_txn()
mirror = txn.insert(self.get_table('Mirror'))
@@ -374,6 +382,12 @@ class OVSDB(object):
src_port.append(mirrored_port)
mirror.select_src_port = src_port
+if mirrored_vlan:
+mirror.verify('select_vlan')
+select_vlan = getattr(mirror, 'select_vlan', [])
+select_vlan.append(mirrored_vlan)
+mirror.select_vlan = select_vlan
+
output_port = self._find_row_by_name('Port', mirror_intf_name)
mirror.verify('output_port')
@@ -440,6 +454,7 @@ def main():
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
interface = None
tcpdargs = []
+vlan = None
skip_next = False
mirror_interface = None
@@ -474,12 +489,25 @@ def main():
elif cur in ['--span']:
mirror_select_all = True
continue
+elif cur in ['--vlan']:
+vlan = nxt
+skip_next = True
+continue
tcpdargs.append(cur)
if interface is None:
print("Error: must at least specify an interface with '-i' option")
sys.exit(1)
+if vlan:
+try:
+vlan = int(vlan)
+if vlan < 0 or vlan > 4095:
+raise ValueError("out of range")
+except ValueError:
+print("Error: vlan muse be within <0-4095>")
+sys.exit(1)
+
if not py_which(dump_cmd):
print("Error: unable to execute '%s' (check PATH)" % dump_cmd)
sys.exit(1)
@@ -523,10 +551,11 @@ def main():
teardown(db_sock, interface, mirror_interface, tap_created)
try:
-ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
+ovsdb.make_port(mirror_interface,
+ovsdb.port_bridge(interface), vlan)
ovsdb.bridge_mirror(interface, mirror_interface,
ovsdb.port_bridge(interface),
-mirror_select_all)
+mirror_select_all, vlan)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
sys.exit(1)
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] ovs-tcpdump: Fix cleanup mirror failed with twice fatal signals.
After running ovs-tcpdump and inputs multiple CTRL+C, the program will
raise the following exception.
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror
ovsdb = OVSDB(db_sock)
File "/usr/bin/ovs-tcpdump", line 168, in __init__
OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB
File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change
while idl.change_seqno == seq and not idl.run():
The default handler of SIGINT is default_int_handler, so it was not
registered to the signal handler. When received CTRL+C again, the program
was broken, and calling hook could not be executed completely.
Signed-off-by: Daniel Ding
---
python/ovs/fatal_signal.py | 24 +---
utilities/ovs-tcpdump.in | 32 +++-
2 files changed, 24 insertions(+), 32 deletions(-)
diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py
index cb2e99e87..16a7e78a0 100644
--- a/python/ovs/fatal_signal.py
+++ b/python/ovs/fatal_signal.py
@@ -16,6 +16,7 @@ import atexit
import os
import signal
import sys
+import threading
import ovs.vlog
@@ -112,29 +113,29 @@ def _unlink(file_):
def _signal_handler(signr, _):
_call_hooks(signr)
-# Re-raise the signal with the default handling so that the program
-# termination status reflects that we were killed by this signal.
-signal.signal(signr, signal.SIG_DFL)
-os.kill(os.getpid(), signr)
-
def _atexit_handler():
_call_hooks(0)
-recurse = False
+mutex = threading.Lock()
def _call_hooks(signr):
-global recurse
-if recurse:
+global mutex
+if not mutex.acquire(blocking=False):
return
-recurse = True
for hook, cancel, run_at_exit in _hooks:
if signr != 0 or run_at_exit:
hook()
+if signr != 0:
+# Re-raise the signal with the default handling so that the program
+# termination status reflects that we were killed by this signal.
+signal.signal(signr, signal.SIG_DFL)
+os.kill(os.getpid(), signr)
+
_inited = False
@@ -150,7 +151,9 @@ def _init():
signal.SIGALRM]
for signr in signals:
-if signal.getsignal(signr) == signal.SIG_DFL:
+handler = signal.getsignal(signr)
+if (handler == signal.SIG_DFL or
+handler == signal.default_int_handler):
signal.signal(signr, _signal_handler)
atexit.register(_atexit_handler)
@@ -165,7 +168,6 @@ def signal_alarm(timeout):
if sys.platform == "win32":
import time
-import threading
class Alarm (threading.Thread):
def __init__(self, timeout):
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 4cbd9a5d3..eada803bb 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -534,29 +534,19 @@ def main():
ovsdb.close_idl()
pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs))
-try:
-while pipes.poll() is None:
-data = pipes.stdout.readline().strip(b'\n')
-if len(data) == 0:
-raise KeyboardInterrupt
-print(data.decode('utf-8'))
-raise KeyboardInterrupt
-except KeyboardInterrupt:
-# If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump
-# -i eth0 | grep "192.168.1.1"), the pipe is no longer available
-# after received Ctrl+C.
-# If we write data to an unavailable pipe, a pipe error will be
-# reported, so we turn off stdout to avoid subsequent flushing
-# of data into the pipe.
-try:
-sys.stdout.close()
-except IOError:
-pass
+while pipes.poll() is None:
+data = pipes.stdout.readline().strip(b'\n')
+if len(data) == 0:
+break
+print(data.decode('utf-8'))
-if pipes.poll() is None:
-pipes.terminate()
+try:
+sys.stdout.close()
+except IOError:
+pass
-sys.exit(0)
+if pipes.poll() is None:
+pipes.terminate()
if __name__ == '__main__':
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs-tcpdump: Fix cleanup mirror failed with twice fatal signals.
> 2024年3月16日 上午9:17，Ilya Maximets 写道： > > On 2/23/24 04:37, Daniel Ding wrote: >> After running ovs-tcpdump and inputs multiple CTRL+C, the program will >> raise the following exception. >> >> Error in atexit._run_exitfuncs: >> Traceback (most recent call last): >> File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror >>ovsdb = OVSDB(db_sock) >> File "/usr/bin/ovs-tcpdump", line 168, in __init__ >>OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB >> File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change >>while idl.change_seqno == seq and not idl.run(): >> >> The default handler of SIGINT is default_int_handler, so it not >> register the signal handler successfully. When received CTRL+C again, >> the program be break, and calling hook can't execute completely. >> >> Signed-off-by: Daniel Ding >> --- >> python/ovs/fatal_signal.py | 24 +--- >> utilities/ovs-tcpdump.in | 38 +- >> 2 files changed, 30 insertions(+), 32 deletions(-) > > Sorry for the late response. Thanks for v2! > > I did some testing and it seem to fix the problem, though in case the ovsdb > connection is no longer available for some reason, we would block forever in > the handler waiting for the server to reply while the signals are blocked. > So, it's getting harder to kill the ovs-tcpdump in this situation. But I > don't really have a solution for this. We will have either this problem or > lingering ports and mirrors as long as we're trying to communicate with the > external process in the signal handler. > > I think, it's an OK trade-off to make. Double Ctrl+C seems like a more > frequent event than paused or stuck ovsdb-server, so I think it's OK to have > this patch. > > See a few minor comments below. > Please help me review the following patch again, Thanks! > Best regards, Ilya Maximets. > >> >> diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py >> index cb2e99e87..077f50dd5 100644 >> --- a/python/ovs/fatal_signal.py >> +++ b/python/ovs/fatal_signal.py >> @@ -16,6 +16,7 @@ import atexit >> import os >> import signal >> import sys >> +import threading >> >> import ovs.vlog >> >> @@ -112,29 +113,29 @@ def _unlink(file_): >> def _signal_handler(signr, _): >> _call_hooks(signr) >> >> -# Re-raise the signal with the default handling so that the program >> -# termination status reflects that we were killed by this signal. >> -signal.signal(signr, signal.SIG_DFL) >> -os.kill(os.getpid(), signr) >> - >> >> def _atexit_handler(): >> _call_hooks(0) >> >> >> -recurse = False >> +mutex = threading.Lock() >> >> >> def _call_hooks(signr): >> -global recurse >> -if recurse: >> +global mutex >> +if not mutex.acquire(blocking=False): >> return >> -recurse = True >> >> for hook, cancel, run_at_exit in _hooks: >> if signr != 0 or run_at_exit: >> hook() >> >> +if signr != 0: >> + # Re-raise the signal with the default handling so that the program >> + # termination status reflects that we were killed by this signal. >> + signal.signal(signr, signal.SIG_DFL) >> + os.kill(os.getpid(), signr) > > The indentation here is incorrect: > > python/ovs/fatal_signal.py:134:8: E114 indentation is not a multiple of 4 > (comment) > python/ovs/fatal_signal.py:135:8: E114 indentation is not a multiple of 4 > (comment) > python/ovs/fatal_signal.py:136:8: E111 indentation is not a multiple of 4 > python/ovs/fatal_signal.py:137:8: E111 indentation is not a multiple of 4 Done > >> + >> >> _inited = False >> >> @@ -150,7 +151,9 @@ def _init(): >>signal.SIGALRM] >> >> for signr in signals: >> -if signal.getsignal(signr) == signal.SIG_DFL: >> +handler = signal.getsignal(signr) >> +if (handler == signal.SIG_DFL or >> +handler == signal.default_int_handler): >> signal.signal(signr, _signal_handler) >> atexit.register(_atexit_handler) >> >> @@ -165,7 +168,6 @@ def signal_alarm(timeout): >> >> if sys.platform == "win32": >> import time >> -import threading >> >> class Alarm (threading.
[ovs-dev] [PATCH v2] ovs-tcpdump: Fix cleanup mirror failed with twice fatal signals.
After running ovs-tcpdump and inputs multiple CTRL+C, the program will
raise the following exception.
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror
ovsdb = OVSDB(db_sock)
File "/usr/bin/ovs-tcpdump", line 168, in __init__
OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB
File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change
while idl.change_seqno == seq and not idl.run():
The default handler of SIGINT is default_int_handler, so it not
register the signal handler successfully. When received CTRL+C again,
the program be break, and calling hook can't execute completely.
Signed-off-by: Daniel Ding
---
python/ovs/fatal_signal.py | 24 +---
utilities/ovs-tcpdump.in | 38 +-
2 files changed, 30 insertions(+), 32 deletions(-)
diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py
index cb2e99e87..077f50dd5 100644
--- a/python/ovs/fatal_signal.py
+++ b/python/ovs/fatal_signal.py
@@ -16,6 +16,7 @@ import atexit
import os
import signal
import sys
+import threading
import ovs.vlog
@@ -112,29 +113,29 @@ def _unlink(file_):
def _signal_handler(signr, _):
_call_hooks(signr)
-# Re-raise the signal with the default handling so that the program
-# termination status reflects that we were killed by this signal.
-signal.signal(signr, signal.SIG_DFL)
-os.kill(os.getpid(), signr)
-
def _atexit_handler():
_call_hooks(0)
-recurse = False
+mutex = threading.Lock()
def _call_hooks(signr):
-global recurse
-if recurse:
+global mutex
+if not mutex.acquire(blocking=False):
return
-recurse = True
for hook, cancel, run_at_exit in _hooks:
if signr != 0 or run_at_exit:
hook()
+if signr != 0:
+ # Re-raise the signal with the default handling so that the program
+ # termination status reflects that we were killed by this signal.
+ signal.signal(signr, signal.SIG_DFL)
+ os.kill(os.getpid(), signr)
+
_inited = False
@@ -150,7 +151,9 @@ def _init():
signal.SIGALRM]
for signr in signals:
-if signal.getsignal(signr) == signal.SIG_DFL:
+handler = signal.getsignal(signr)
+if (handler == signal.SIG_DFL or
+handler == signal.default_int_handler):
signal.signal(signr, _signal_handler)
atexit.register(_atexit_handler)
@@ -165,7 +168,6 @@ def signal_alarm(timeout):
if sys.platform == "win32":
import time
-import threading
class Alarm (threading.Thread):
def __init__(self, timeout):
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 4cbd9a5d3..0731b4ac8 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -534,29 +534,25 @@ def main():
ovsdb.close_idl()
pipes = _doexec(*([dump_cmd, '-i', mirror_interface] + tcpdargs))
+while pipes.poll() is None:
+data = pipes.stdout.readline().strip(b'\n')
+if len(data) == 0:
+break
+print(data.decode('utf-8'))
+
+# If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump
+# -i eth0 | grep "192.168.1.1"), the pipe is no longer available
+# after received Ctrl+C.
+# If we write data to an unavailable pipe, a pipe error will be
+# reported, so we turn off stdout to avoid subsequent flushing
+# of data into the pipe.
try:
-while pipes.poll() is None:
-data = pipes.stdout.readline().strip(b'\n')
-if len(data) == 0:
-raise KeyboardInterrupt
-print(data.decode('utf-8'))
-raise KeyboardInterrupt
-except KeyboardInterrupt:
-# If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump
-# -i eth0 | grep "192.168.1.1"), the pipe is no longer available
-# after received Ctrl+C.
-# If we write data to an unavailable pipe, a pipe error will be
-# reported, so we turn off stdout to avoid subsequent flushing
-# of data into the pipe.
-try:
-sys.stdout.close()
-except IOError:
-pass
-
-if pipes.poll() is None:
-pipes.terminate()
+ sys.stdout.close()
+except IOError:
+ pass
-sys.exit(0)
+if pipes.poll() is None:
+pipes.terminate()
if __name__ == '__main__':
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1] ovs-tcpdump: Cleanup mirror failed with twice fatal signals
> 2024年2月22日 下午7:20，Ilya Maximets 写道： > > On 2/22/24 11:53, Ilya Maximets wrote: >> On 2/22/24 11:02, Daniel Ding wrote: >>> >>> >>>> 2024年2月22日 上午1:55，Ilya Maximets >>> <mailto:i.maxim...@ovn.org>> 写道： >>>> >>>> On 2/21/24 15:27, Aaron Conole wrote: >>>>> Daniel Ding mailto:danieldin...@gmail.com>> >>>>> writes: >>>>> >>>>>> After running ovs-tcpdump and inputs multiple CTRL+C, the program will >>>>>> raise the following exception. >>>>>> >>>>>> Error in atexit._run_exitfuncs: >>>>>> Traceback (most recent call last): >>>>>> File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror >>>>>>ovsdb = OVSDB(db_sock) >>>>>> File "/usr/bin/ovs-tcpdump", line 168, in __init__ >>>>>>OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB >>>>>> File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change >>>>>>while idl.change_seqno == seq and not idl.run(): >>>>>> >>>>>> Signed-off-by: Daniel Ding >>>>> <mailto:zhihui.d...@easystack.cn>> >>>>>> --- >>>>> >>>>> LGTM for the linux side - maybe Alin might check the windows side. >>>> >>>> The code is copied from the fatal-signla library, so it is likely fine. >>>> However, I don;t think this issue should be fixed on the application >>>> level (ovs-tcpdump). There seems to be adifference in how C and python >>>> fatal-signla libraries behave. The C version calls the hooks in the run() >>>> function and the signal handler only updates the signal number variable. >>>> So, if the same signal arrives multiple times it will be handled only once >>>> and the run() function will not exit until all the hooks are done, unless >>>> it is a segfault. >>>> >>>> With python implementation we're calling hooks right from the signal >>>> handler (it's not a real signal handler, so we're allowed to use not >>>> really singal-safe functions there). So, if another signal arrives while >>>> we're executing the hooks, the second hook execution will be skipped >>>> due to 'recursion' check, but the signal will be re-raised immediately, >>>> killing the process. >>> >>> As your suggestions, I rewrite recurse as a threading.Lock. So that protect >>> calling hooks >>> applications registered. >>> >>>> >>>> There is likley a way to fix that by using a mutex instead of recursion >>>> check and blocking it while executing the hooks. The signal number >>>> will need to be stored in the signal handler and the signal will need >>>> to be re-raised in the call_hooks() instead of signal handler. >>>> We can use mutex.acquire(blocking=False) as a way to prevent recursion. >>>> >>> >>> To avoid breaking calling hooks, I set the signal restored in the signal >>> handler to SIG_IGN in >>> call_hooks. >> >> I'm not sure this is necessary, see below. The default handler of SIGINT is default_int_handler, so it not register the signal handler successfully. When received CTRL+C again, the program be break, and calling hook can't execute completely. >> >>> And move re-raised the signal in the call_hooks with locked. >>> >>>> Does that make sense? >>>> >>>> Best regards, Ilya Maximets. >>> >>> diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py >>> index cb2e99e87..3aa56a166 100644 >>> --- a/python/ovs/fatal_signal.py >>> +++ b/python/ovs/fatal_signal.py >>> @@ -16,6 +16,7 @@ import atexit >>> import os >>> import signal >>> import sys >>> +import threading >>> >>> import ovs.vlog >>> >>> @@ -112,28 +113,34 @@ def _unlink(file_): >>> def _signal_handler(signr, _): >>> _call_hooks(signr) >>> >>> -# Re-raise the signal with the default handling so that the program >>> -# termination status reflects that we were killed by this signal. >>> -signal.signal(signr, signal.SIG_DFL) >>> -os.kill(os.getpid(), signr) >>> - >>> >>> def _atexit_handler(): >>> _call_h
Re: [ovs-dev] [PATCH v1] ovs-tcpdump: Cleanup mirror failed with twice fatal signals
> 2024年2月22日 上午1:55，Ilya Maximets 写道： > > On 2/21/24 15:27, Aaron Conole wrote: >> Daniel Ding writes: >> >>> After running ovs-tcpdump and inputs multiple CTRL+C, the program will >>> raise the following exception. >>> >>> Error in atexit._run_exitfuncs: >>> Traceback (most recent call last): >>> File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror >>>ovsdb = OVSDB(db_sock) >>> File "/usr/bin/ovs-tcpdump", line 168, in __init__ >>>OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB >>> File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change >>>while idl.change_seqno == seq and not idl.run(): >>> >>> Signed-off-by: Daniel Ding >>> --- >> >> LGTM for the linux side - maybe Alin might check the windows side. > > The code is copied from the fatal-signla library, so it is likely fine. > However, I don;t think this issue should be fixed on the application > level (ovs-tcpdump). There seems to be adifference in how C and python > fatal-signla libraries behave. The C version calls the hooks in the run() > function and the signal handler only updates the signal number variable. > So, if the same signal arrives multiple times it will be handled only once > and the run() function will not exit until all the hooks are done, unless > it is a segfault. > > With python implementation we're calling hooks right from the signal > handler (it's not a real signal handler, so we're allowed to use not > really singal-safe functions there). So, if another signal arrives while > we're executing the hooks, the second hook execution will be skipped > due to 'recursion' check, but the signal will be re-raised immediately, > killing the process. As your suggestions, I rewrite recurse as a threading.Lock. So that protect calling hooks applications registered. > > There is likley a way to fix that by using a mutex instead of recursion > check and blocking it while executing the hooks. The signal number > will need to be stored in the signal handler and the signal will need > to be re-raised in the call_hooks() instead of signal handler. > We can use mutex.acquire(blocking=False) as a way to prevent recursion. > To avoid breaking calling hooks, I set the signal restored in the signal handler to SIG_IGN in call_hooks. And move re-raised the signal in the call_hooks with locked. > Does that make sense? > > Best regards, Ilya Maximets. diff --git a/python/ovs/fatal_signal.py b/python/ovs/fatal_signal.py index cb2e99e87..3aa56a166 100644 --- a/python/ovs/fatal_signal.py +++ b/python/ovs/fatal_signal.py @@ -16,6 +16,7 @@ import atexit import os import signal import sys +import threading import ovs.vlog @@ -112,28 +113,34 @@ def _unlink(file_): def _signal_handler(signr, _): _call_hooks(signr) -# Re-raise the signal with the default handling so that the program -# termination status reflects that we were killed by this signal. -signal.signal(signr, signal.SIG_DFL) -os.kill(os.getpid(), signr) - def _atexit_handler(): _call_hooks(0) -recurse = False +recurse = threading.Lock() def _call_hooks(signr): global recurse -if recurse: +if recurse.locked(): return -recurse = True -for hook, cancel, run_at_exit in _hooks: -if signr != 0 or run_at_exit: -hook() +with recurse: +if signr != 0: +signal.signal(signr, signal.SIG_IGN) +else: +signal.signal(signal.SIGINT, signal.SIG_IGN) + +for hook, cancel, run_at_exit in _hooks: +if signr != 0 or run_at_exit: +hook() + +if signr != 0: + # Re-raise the signal with the default handling so that the program + # termination status reflects that we were killed by this signal. + signal.signal(signr, signal.SIG_DFL) + os.kill(os.getpid(), signr) _inited = False @@ -165,7 +172,6 @@ def signal_alarm(timeout): if sys.platform == "win32": import time -import threading class Alarm (threading.Thread): def __init__(self, timeout): -- 2.43.0 Thanks Ilya Maximets, please help me review again. Best regards, Daniel Ding. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] ovs-tcpdump: Cleanup mirror failed with twice fatal signals
After running ovs-tcpdump and inputs multiple CTRL+C, the program will raise the following exception. Error in atexit._run_exitfuncs: Traceback (most recent call last): File "/usr/bin/ovs-tcpdump", line 421, in cleanup_mirror ovsdb = OVSDB(db_sock) File "/usr/bin/ovs-tcpdump", line 168, in __init__ OVSDB.wait_for_db_change(self._idl_conn) # Initial Sync with DB File "/usr/bin/ovs-tcpdump", line 155, in wait_for_db_change while idl.change_seqno == seq and not idl.run(): Signed-off-by: Daniel Ding --- utilities/ovs-tcpdump.in | 15 +++ 1 file changed, 15 insertions(+) diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in index 4cbd9a5d3..eec2262d6 100755 --- a/utilities/ovs-tcpdump.in +++ b/utilities/ovs-tcpdump.in @@ -17,6 +17,7 @@ import os import pwd from random import randint +import signal import subprocess import sys import time @@ -417,8 +418,22 @@ def py_which(executable): for path in os.environ["PATH"].split(os.pathsep)) +def ignore_fatal_signals(): +if sys.platform == "win32": +signals = [signal.SIGTERM, signal.SIGINT] +else: +signals = [signal.SIGTERM, signal.SIGINT, signal.SIGHUP, + signal.SIGALRM] + +for signr in signals: +signal.signal(signr, signal.SIG_IGN) + + def teardown(db_sock, interface, mirror_interface, tap_created): def cleanup_mirror(): +# Ignore fatal signals, avoid it to break OVSDB operations. +# So that cleanup mirror and ports be finished. +ignore_fatal_signals() try: ovsdb = OVSDB(db_sock) ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface)) -- 2.43.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn v4] northd: forward arp request to lrp snat on.
If the router has a snat rule and it's external ip isn't lrp address,
when the arp request from other router for this external ip, will
be drop, because of this external ip use same mac address as lrp, so
can not forward to MC_FLOOD.
Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
possible.")
Reported-at: https://github.com/ovn-org/ovn/issues/209
Signed-off-by: Daniel Ding
---
northd/northd.c | 31 +++
tests/ovn-northd.at | 12
2 files changed, 43 insertions(+)
diff --git a/northd/northd.c b/northd/northd.c
index e9cb906e2..2e6764d7b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -9004,6 +9004,37 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
}
}
+struct shash_node *snat_snode;
+SHASH_FOR_EACH (snat_snode, &op->od->snat_ips) {
+struct ovn_snat_ip *snat_ip = snat_snode->data;
+
+if (ovs_list_is_empty(&snat_ip->snat_entries)) {
+continue;
+}
+
+struct ovn_nat *nat_entry =
+CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries),
+ struct ovn_nat, ext_addr_list_node);
+const struct nbrec_nat *nat = nat_entry->nb;
+
+/* Check if the ovn port has a network configured on which we could
+ * expect ARP requests/NS for the DNAT external_ip.
+ */
+if (nat_entry_is_v6(nat_entry)) {
+if (!sset_contains(&op->od->lb_ips->ips_v6, nat->external_ip)) {
+build_lswitch_rport_arp_req_flow(
+nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows,
+stage_hint);
+}
+} else {
+if (!sset_contains(&op->od->lb_ips->ips_v4, nat->external_ip)) {
+build_lswitch_rport_arp_req_flow(
+nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows,
+stage_hint);
+}
+}
+}
+
for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
build_lswitch_rport_arp_req_flow(
op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, 80,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5c9da811f..953e0d829 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast),
action=(outport = "_MC_flood"; output;)
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport
= "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.1.1), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
])
@@ -5098,6 +5099,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:02:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport
= "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.2.1), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.100), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.200), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:201), action=(clone {outport =
"ls2-r
Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.
On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara wrote:
> On 12/6/23 02:56, Daniel Ding wrote:
> > Hi Dumitru!
> >
> > On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara wrote:
> >
> >> On 12/5/23 13:58, Daniel Ding wrote:
> >>>
> >>>
> >>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara >>> <mailto:dce...@redhat.com>> wrote:
> >>>
> >>> Hi Daniel,
> >>>
> >>> Thanks for this new revision but why is it v3? I don't think I saw
> >> v2
> >>> posted anywhere but maybe I missed it.
> >>>
> >>>
> >>> Sorry, that is my mistake.
> >>>
> >>
> >> No problem.
> >>
> >>>
> >>> On 12/5/23 08:33, Daniel Ding wrote:
> >>> > If the router has a snat rule and it's external ip isn't lrp
> >> address,
> >>> > when the arp request from other router for this external ip, will
> >>> > be drop, because of this external ip use same mac address as lrp,
> >> so
> >>> > can not forward to MC_FLOOD.
> >>> >
> >>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> >>> whenever possible.")
> >>> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >>> <https://github.com/ovn-org/ovn/issues/209>
> >>> >
> >>> > Signed-off-by: Daniel Ding >>> <mailto:danieldin...@gmail.com>>
> >>> > Acked-by: Dumitru Ceara >> dce...@redhat.com>>
> >>>
> >>> Please don't add an "Acked-by: ... " if the person never explicitly
> >>> replied with "Acked-by: ... " on the previous version of the patch
> or
> >>> if you didn't have explicit agreement from that person to do so.
> >>>
> >>> Quoting from my previous reply to your v1, I said:
> >>>
> >>> "I think it makes sense to do what you're suggesting."
> >>>
> >>>
> >>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >> <
> >>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >>>
> >>>
> >>> That doesn't mean I acked the patch.
> >>>
> >>>
> >>> Got it. Thx!
> >>>
> >>
> >> No worries.
> >>
> >>>
> >>> > ---
> >>> > northd/northd.c | 18 +-
> >>> > tests/ovn-northd.at <http://ovn-northd.at> | 12
> >>> > 2 files changed, 29 insertions(+), 1 deletion(-)
> >>> >
> >>> > diff --git a/northd/northd.c b/northd/northd.c
> >>> > index e9cb906e2..99fb30f46 100644
> >>> > --- a/northd/northd.c
> >>> > +++ b/northd/northd.c
> >>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>> > }
> >>> > }
> >>> >
> >>> > +struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
> >>> > +struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
> >>> > +
> >>> > for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> >>> > struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> >>> > const struct nbrec_nat *nat = nat_entry->nb;
> >>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> >>> ovn_port *op,
> >>> > }
> >>> >
> >>> > if (!strcmp(nat->type, "snat")) {
> >>> > -continue;
> >>> > +if (nat_entry_is_v6(nat_entry)) {
> >>> > +if (sset_contains(&snat_ips_v6,
> >> nat->external_ip)) {
> >>> > +continue;
> >>> > +}
> >>> > +sset_add(&snat_ips_v6, nat->external_ip);
> >>> > +} else {
> >>>
Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.
Hi Dumitru!
On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara wrote:
> On 12/5/23 13:58, Daniel Ding wrote:
> >
> >
> > On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara > <mailto:dce...@redhat.com>> wrote:
> >
> > Hi Daniel,
> >
> > Thanks for this new revision but why is it v3? I don't think I saw
> v2
> > posted anywhere but maybe I missed it.
> >
> >
> > Sorry, that is my mistake.
> >
>
> No problem.
>
> >
> > On 12/5/23 08:33, Daniel Ding wrote:
> > > If the router has a snat rule and it's external ip isn't lrp
> address,
> > > when the arp request from other router for this external ip, will
> > > be drop, because of this external ip use same mac address as lrp,
> so
> > > can not forward to MC_FLOOD.
> > >
> > > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain
> > whenever possible.")
> > > Reported-at: https://github.com/ovn-org/ovn/issues/209
> > <https://github.com/ovn-org/ovn/issues/209>
> > >
> > > Signed-off-by: Daniel Ding > <mailto:danieldin...@gmail.com>>
> > > Acked-by: Dumitru Ceara dce...@redhat.com>>
> >
> > Please don't add an "Acked-by: ... " if the person never explicitly
> > replied with "Acked-by: ... " on the previous version of the patch or
> > if you didn't have explicit agreement from that person to do so.
> >
> > Quoting from my previous reply to your v1, I said:
> >
> > "I think it makes sense to do what you're suggesting."
> >
> >
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> <
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
> >
> >
> > That doesn't mean I acked the patch.
> >
> >
> > Got it. Thx!
> >
>
> No worries.
>
> >
> > > ---
> > > northd/northd.c | 18 +-
> > > tests/ovn-northd.at <http://ovn-northd.at> | 12
> > > 2 files changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index e9cb906e2..99fb30f46 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > > }
> > > }
> > >
> > > +struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
> > > +struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
> > > +
> > > for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > > struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> > > const struct nbrec_nat *nat = nat_entry->nb;
> > > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct
> > ovn_port *op,
> > > }
> > >
> > > if (!strcmp(nat->type, "snat")) {
> > > -continue;
> > > +if (nat_entry_is_v6(nat_entry)) {
> > > +if (sset_contains(&snat_ips_v6,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(&snat_ips_v6, nat->external_ip);
> > > +} else {
> > > +if (sset_contains(&snat_ips_v4,
> nat->external_ip)) {
> > > +continue;
> > > +}
> > > +sset_add(&snat_ips_v4, nat->external_ip);
> > > +}
> >
> > Essentially this just makes sure we don't skip NAT entries and that
> we
> > consider unique external_ips. I'm fine with relaxing the second
> part of
> > the condition in which case, as mentioned on v1, I think we can just
> > remove the whole "if (!strcmp(nat->type, "snat")) {" block.
> >
> > With the following incremental change applied (it removes the block)
> > your test still passes:
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index df7d2d60a5..20efd3b74c 100644
> > ---
Re: [ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.
On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara wrote:
> Hi Daniel,
>
> Thanks for this new revision but why is it v3? I don't think I saw v2
> posted anywhere but maybe I missed it.
>
>
Sorry, that is my mistake.
> On 12/5/23 08:33, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >
> > Signed-off-by: Daniel Ding
> > Acked-by: Dumitru Ceara
>
> Please don't add an "Acked-by: ... " if the person never explicitly
> replied with "Acked-by: ... " on the previous version of the patch or
> if you didn't have explicit agreement from that person to do so.
>
> Quoting from my previous reply to your v1, I said:
>
> "I think it makes sense to do what you're suggesting."
>
>
> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934
>
> That doesn't mean I acked the patch.
>
>
Got it. Thx!
> > ---
> > northd/northd.c | 18 +-
> > tests/ovn-northd.at | 12
> > 2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> > }
> > }
> >
> > +struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
> > +
> > for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> > const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> > }
> >
> > if (!strcmp(nat->type, "snat")) {
> > -continue;
> > +if (nat_entry_is_v6(nat_entry)) {
> > +if (sset_contains(&snat_ips_v6, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(&snat_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(&snat_ips_v4, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(&snat_ips_v4, nat->external_ip);
> > +}
>
> Essentially this just makes sure we don't skip NAT entries and that we
> consider unique external_ips. I'm fine with relaxing the second part of
> the condition in which case, as mentioned on v1, I think we can just
> remove the whole "if (!strcmp(nat->type, "snat")) {" block.
>
> With the following incremental change applied (it removes the block)
> your test still passes:
>
> diff --git a/northd/northd.c b/northd/northd.c
> index df7d2d60a5..20efd3b74c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> }
> }
>
> -struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
> -struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
> -
> for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> const struct nbrec_nat *nat = nat_entry->nb;
> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> continue;
> }
>
> -if (!strcmp(nat->type, "snat")) {
> -if (nat_entry_is_v6(nat_entry)) {
> -if (sset_contains(&snat_ips_v6, nat->external_ip)) {
> -continue;
> -}
> -sset_add(&snat_ips_v6, nat->external_ip);
> -} else {
> -if (sset_contains(&snat_ips_v4, nat->external_ip)) {
> -continue;
> -}
> -sset_add(&snat_ips_v4, nat->external_ip);
> -}
> -}
> -
> /* Check if the ovn port has a netwo
[ovs-dev] [PATCH ovn v3] northd: forward arp request to lrp snat on.
If the router has a snat rule and it's external ip isn't lrp address,
when the arp request from other router for this external ip, will
be drop, because of this external ip use same mac address as lrp, so
can not forward to MC_FLOOD.
Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
possible.")
Reported-at: https://github.com/ovn-org/ovn/issues/209
Signed-off-by: Daniel Ding
Acked-by: Dumitru Ceara
---
northd/northd.c | 18 +-
tests/ovn-northd.at | 12
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index e9cb906e2..99fb30f46 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
}
}
+struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
+struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
+
for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
struct ovn_nat *nat_entry = &op->od->nat_entries[i];
const struct nbrec_nat *nat = nat_entry->nb;
@@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
}
if (!strcmp(nat->type, "snat")) {
-continue;
+if (nat_entry_is_v6(nat_entry)) {
+if (sset_contains(&snat_ips_v6, nat->external_ip)) {
+continue;
+}
+sset_add(&snat_ips_v6, nat->external_ip);
+} else {
+if (sset_contains(&snat_ips_v4, nat->external_ip)) {
+continue;
+}
+sset_add(&snat_ips_v4, nat->external_ip);
+}
}
/* Check if the ovn port has a network configured on which we could
@@ -9025,6 +9038,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
}
+
+sset_destroy(&snat_ips_v4);
+sset_destroy(&snat_ips_v6);
}
static void
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5c9da811f..953e0d829 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast),
action=(outport = "_MC_flood"; output;)
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport
= "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.1.1), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
])
@@ -5098,6 +5099,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:02:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)), action=(outport
= "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.2.1), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.100), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.200), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:201), action=(clone {outport =
"ls2-ro2"; output; }; outport = "_MC_flood_l2"; output;)
Re: [ovs-dev] [PATCH ovn] northd: forward arp request to lrp snat on.
On Fri, 10 Nov 2023 at 22:51, Dumitru Ceara wrote:
> On 10/24/23 09:39, Daniel Ding wrote:
> > If the router has a snat rule and it's external ip isn't lrp address,
> > when the arp request from other router for this external ip, will
> > be drop, because of this external ip use same mac address as lrp, so
> > can not forward to MC_FLOOD.
> >
> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> > possible.")
> > Reported-at: https://github.com/ovn-org/ovn/issues/209
> >
> > Signed-off-by: Daniel Ding
> > ---
>
> Hi Daniel,
>
> Thanks for the patch! I think it makes sense to do what you're
> suggesting. The patch itself, however, is malformed.
>
> Could you please use git format-patch and git send-email?
>
>
> https://github.com/ovn-org/ovn/blob/3bf67405faed9fc5c2d07024b0775e0d363651a6/Documentation/internals/contributing/submitting-patches.rst?plain=1#L90
>
> I fixed the patch formatting so I can apply it locally for review; it
> was mostly lines being truncated to shorten their lengths.
>
Thanks! I will resubmit it with v3.
> > northd/northd.c | 18 +-
> > tests/ovn-northd.at | 12
> > 2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2..99fb30f46 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> > }
> > }
> >
> > +struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
> > +struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
> > +
> > for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
> > struct ovn_nat *nat_entry = &op->od->nat_entries[i];
> > const struct nbrec_nat *nat = nat_entry->nb;
> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> > *op,
> > }
> >
> > if (!strcmp(nat->type, "snat")) {
> > -continue;
> > +if (nat_entry_is_v6(nat_entry)) {
> > +if (sset_contains(&snat_ips_v6, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(&snat_ips_v6, nat->external_ip);
> > +} else {
> > +if (sset_contains(&snat_ips_v4, nat->external_ip)) {
> > +continue;
> > +}
> > +sset_add(&snat_ips_v4, nat->external_ip);
> > +}
> > }
>
> Can't we just remove the whole if? And not skip all snats?
>
Can't remove the whole if. The address of snats is not added to op->od->
lb_ips. and we don't consider lb_ips to snat_ips.
>
> Regards,
> Dumitru
>
> >
> > /* Check if the ovn port has a network configured on which we
> could
> > @@ -9025,6 +9038,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> *op,
> > if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
> > build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od,
> lflows);
> > }
> > +
> > +sset_destroy(&snat_ips_v4);
> > +sset_destroy(&snat_ips_v6);
> > }
> >
> > static void
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 5c9da811f..953e0d829 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed
> > 's/table=../table=??/' | sort],
> >table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast),
> > action=(outport = "_MC_flood"; output;)
> >table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
> > {00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)),
> > action=(outport = "_MC_flood_l2"; output;)
> >table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0
> &&
> > arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport =
> "ls1-ro1";
> > output; }; outport = "_MC_flood_l2"; output;)
> > + table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0
> &&
> > arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport =
> "ls1-ro1";
> > output; }; outport = "_MC_flood_l2"; output;)
> >table=??(ls_in_l2_lkup ), priority=80 , match=
[ovs-dev] [PATCH ovn] northd: forward arp request to lrp snat on.
If the router has a snat rule and it's external ip isn't lrp address,
when the arp request from other router for this external ip, will
be drop, because of this external ip use same mac address as lrp, so
can not forward to MC_FLOOD.
Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
possible.")
Reported-at: https://github.com/ovn-org/ovn/issues/209
Signed-off-by: Daniel Ding
---
northd/northd.c | 18 +-
tests/ovn-northd.at | 12
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index e9cb906e2..99fb30f46 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
}
}
+struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4);
+struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6);
+
for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
struct ovn_nat *nat_entry = &op->od->nat_entries[i];
const struct nbrec_nat *nat = nat_entry->nb;
@@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
*op,
}
if (!strcmp(nat->type, "snat")) {
-continue;
+if (nat_entry_is_v6(nat_entry)) {
+if (sset_contains(&snat_ips_v6, nat->external_ip)) {
+continue;
+}
+sset_add(&snat_ips_v6, nat->external_ip);
+} else {
+if (sset_contains(&snat_ips_v4, nat->external_ip)) {
+continue;
+}
+sset_add(&snat_ips_v4, nat->external_ip);
+}
}
/* Check if the ovn port has a network configured on which we could
@@ -9025,6 +9038,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
if (sw_od->n_router_ports != sw_od->nbs->n_ports) {
build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows);
}
+
+sset_destroy(&snat_ips_v4);
+sset_destroy(&snat_ips_v6);
}
static void
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5c9da811f..953e0d829 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -5084,6 +5084,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls1_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=70 , match=(eth.mcast),
action=(outport = "_MC_flood"; output;)
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:01:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)),
action=(outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.100), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 10.0.0.200), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.1.1), action=(clone {outport = "ls1-ro1";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:101), action=(clone {outport =
"ls1-ro1"; output; }; outport = "_MC_flood_l2"; output;)
])
@@ -5098,6 +5099,7 @@ AT_CHECK([grep "ls_in_l2_lkup" ls2_lflows | sed
's/table=../table=??/' | sort],
table=??(ls_in_l2_lkup ), priority=75 , match=(eth.src ==
{00:00:00:00:02:01} && (arp.op == 1 || rarp.op == 3 || nd_ns)),
action=(outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 192.168.2.1), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.100), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
+ table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
arp.op == 1 && arp.tpa == 20.0.0.200), action=(clone {outport = "ls2-ro2";
output; }; outport = "_MC_flood_l2"; output;)
table=??(ls_in_l2_lkup ), priority=80 , match=(flags[[1]] == 0 &&
nd_ns && nd.target == fe80::200:ff:fe00:201), action=(clone {outport =
"ls2-ro2"; output; }; outport = "_MC_flood_l2"; output;)
])
@@ -5118,8 +5120,10 @@ AT_CHECK([grep "ls_in_
[ovs-dev] [PATCH v2] ovsschema: Set bfd_status to ephemeral.
When restart openvswitch, the bfd status will be kept
before ovs-vswitchd running. And if the ovs-vswitchd
has high workload, which will defer updating bfd status,
which not we excepted.
Signed-off-by: Daniel Ding
---
vswitchd/vswitch.ovsschema | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 4873cfde7..1a49cdffe 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
{"name": "Open_vSwitch",
- "version": "8.3.0",
- "cksum": "3781850481 26690",
+ "version": "8.3.1",
+ "cksum": "3012963480 26720",
"tables": {
"Open_vSwitch": {
"columns": {
@@ -280,7 +280,8 @@
"min": 0, "max": "unlimited"}},
"bfd_status": {
"type": {"key": "string", "value": "string",
- "min": 0, "max": "unlimited"}},
+ "min": 0, "max": "unlimited"},
+ "ephemeral": true},
"cfm_mpid": {
"type": {
"key": {"type": "integer"},
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] ovsschema: Set bfd_status to ephemeral.
When restart openvswitch, the bfd status will be kept
before ovs-vswitchd running. And if the ovs-vswitchd
has high workload, which will defer updating bfd status,
which not we excepted.
---
vswitchd/vswitch.ovsschema | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 4873cfde7..1a49cdffe 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
{"name": "Open_vSwitch",
- "version": "8.3.0",
- "cksum": "3781850481 26690",
+ "version": "8.3.1",
+ "cksum": "3012963480 26720",
"tables": {
"Open_vSwitch": {
"columns": {
@@ -280,7 +280,8 @@
"min": 0, "max": "unlimited"}},
"bfd_status": {
"type": {"key": "string", "value": "string",
- "min": 0, "max": "unlimited"}},
+ "min": 0, "max": "unlimited"},
+ "ephemeral": true},
"cfm_mpid": {
"type": {
"key": {"type": "integer"},
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Acked-by: Mike Pattrick
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..92cff6d2c 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,24 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+try:
+ovsdb = OVSDB(db_sock)
+ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+if tap_created is True:
+_del_taps[sys.platform](mirror_interface)
+except Exception:
+print("Unable to tear down the ports and mirrors.")
+print("Please use ovs-vsctl to remove the ports and mirrors"
+ " created.")
+print(" ex: ovs-vsctl --db=%s del-port %s" % (db_sock,
+ mirror_interface))
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +508,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +518,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -518,18 +534,6 @@ def main():
if pipes.poll() is None:
pipes.terminate()
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-print("Unable to tear down the ports and mirrors.")
-print("Please use ovs-vsctl to remove the ports and mirrors created.")
-print(" ex: ovs-vsctl --db=%s del-port %s" % (db_sock,
- mirror_interface))
-sys.exit(1)
-
sys.exit(0)
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
On 2022/9/10 1:43, Ilya Maximets wrote:
On 9/8/22 09:37, Daniel Ding wrote:
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Acked-by: Mike Pattrick
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..b3f764c87 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,17 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+ovsdb = OVSDB(db_sock)
+ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+if tap_created is True:
+_del_taps[sys.platform](mirror_interface)
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +501,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +511,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -517,12 +526,6 @@ def main():
except KeyboardInterrupt:
if pipes.poll() is None:
pipes.terminate()
-
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors created.")
The above 'except' section, should be remove it, if I understand correctlly.
I think there was some misunderstanding here. Sorry about that.
What I meant is that cleanup_mirror() function should have a
try-except block and have the 'print's above in the 'except' section.
i.e.
def teardown(...):
def cleanup_mirror():
try:
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors
created.")
print(" ex: ovs-vsctl --db=...
add_hook(cleanup_mirror, None, True)
Okay, I will add try-except block in cleanup_mirror() function again,
and prints
warning.
And for the pipes.poll() loop we only need to catch the KeyboardInterrupt.
No need to catch generic exceptions, IIUC. The hook will be executed
anyway, even on the unexpected exception, right?
Right. The third parameter of add_hook indicats the hook function need
be run
at exiting even on the unexpeted exception.
Best regards, Ilya Maximets.
Thanks for your review, @Ilya Maximets. And the following is newer patch
according to your suggestions.
---
utilities/ovs-tcpdump.in | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..92cff6d2c 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+ from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -
[ovs-dev] [PATCH v4] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Acked-by: Mike Pattrick
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..b3f764c87 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,17 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+ovsdb = OVSDB(db_sock)
+ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+if tap_created is True:
+_del_taps[sys.platform](mirror_interface)
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +501,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +511,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -517,12 +526,6 @@ def main():
except KeyboardInterrupt:
if pipes.poll() is None:
pipes.terminate()
-
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors created.")
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
Excuse me,
This patch is invalid because of indentation.
On 2022/9/8 15:07, Daniel Ding wrote:
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Acked-by: Mike Pattrick
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..7d5abba76 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,17 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+ ovsdb = OVSDB(db_sock)
+ ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+ if tap_created is True:
+ _del_taps[sys.platform](mirror_interface)
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +501,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +511,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -517,12 +526,6 @@ def main():
except KeyboardInterrupt:
if pipes.poll() is None:
pipes.terminate()
-
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors created.")
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Acked-by: Mike Pattrick
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..7d5abba76 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,17 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+ ovsdb = OVSDB(db_sock)
+ ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+ if tap_created is True:
+ _del_taps[sys.platform](mirror_interface)
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +501,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +511,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -517,12 +526,6 @@ def main():
except KeyboardInterrupt:
if pipes.poll() is None:
pipes.terminate()
-
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors created.")
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ovs-tcpdump: Cleanup mirror port on SIGHUP/SIGTERM
If ovs-tcpdump received HUP or TERM signal, mirror and mirror
interface should be destroyed. This often happens, when
controlling terminal is closed, like ssh session closed, and
other users use kill to terminate it.
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..1d88a4666 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -44,6 +44,7 @@ try:
from ovs import jsonrpc
from ovs.poller import Poller
from ovs.stream import Stream
+from ovs.fatal_signal import add_hook
except Exception:
print("ERROR: Please install the correct Open vSwitch python support")
print(" libraries (version @VERSION@).")
@@ -405,6 +406,20 @@ def py_which(executable):
for path in os.environ["PATH"].split(os.pathsep))
+def teardown(db_sock, interface, mirror_interface, tap_created):
+def cleanup_mirror():
+try:
+ovsdb = OVSDB(db_sock)
+ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+if tap_created is True:
+_del_taps[sys.platform](mirror_interface)
+except Exception as e:
+print("ERROR: Unable to clean mirror: %s" % str(e))
+
+add_hook(cleanup_mirror, None, True)
+
+
def main():
rundir = os.environ.get('OVS_RUNDIR', '@RUNDIR@')
db_sock = 'unix:%s' % os.path.join(rundir, "db.sock")
@@ -489,6 +504,9 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+teardown(db_sock, interface, mirror_interface, tap_created)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
@@ -496,12 +514,6 @@ def main():
mirror_select_all)
except OVSDBException as oe:
print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
-try:
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
-except Exception:
-pass
sys.exit(1)
ovsdb.close_idl()
@@ -517,12 +529,6 @@ def main():
except KeyboardInterrupt:
if pipes.poll() is None:
pipes.terminate()
-
-ovsdb = OVSDB(db_sock)
-ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
-ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
-if tap_created is True:
-_del_taps[sys.platform](mirror_interface)
except Exception:
print("Unable to tear down the ports and mirrors.")
print("Please use ovs-vsctl to remove the ports and mirrors created.")
--
2.27.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] utilities/ovs-tcpdump.in: Fix not destroy mirror (Daniel Ding)
If ovs-tcpdump received HUP or TERM signal, mirror and mirror interface should
be destroyed. This often happens, when controlling terminal is closed, like
ssh session closed, and other users use kill to terminate it.
Signed-off-by: Daniel Ding
---
utilities/ovs-tcpdump.in | 12
1 file changed, 12 insertions(+)
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 7fd26e405..211d50fc5 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -17,6 +17,7 @@
import os
import pwd
from random import randint
+import signal
import subprocess
import sys
import time
@@ -489,6 +490,17 @@ def main():
print("ERROR: Mirror port (%s) exists for port %s." %
(mirror_interface, interface))
sys.exit(1)
+
+def signal_handler(_signo, _stack_frame):
+ovsdb.destroy_mirror(interface, ovsdb.port_bridge(interface))
+ovsdb.destroy_port(mirror_interface, ovsdb.port_bridge(interface))
+
+sys.exit(0)
+
+if sys.platform in ['linux', 'linux2']:
+signal.signal(signal.SIGHUP, signal_handler)
+signal.signal(signal.SIGTERM, signal_handler)
+
try:
ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
ovsdb.bridge_mirror(interface, mirror_interface,
---
Best regards, Daniel Ding
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
