| Message ID | 20241030135043.3139987-7-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ipsec: Resiliency to Libreswan failures. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 30 Oct 2024, at 14:50, Ilya Maximets wrote: > Add a new command line option --command-timeout that controls the > command timeout. It is important to have this configurable, because > the retransmit-timeout is configurable in Libreswan. Also, users > may prefer the monitor to be more responsive. > > ovs-monitor-ipsec options are not documented anywhere, so not > trying to address that here. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> See comment below. > --- > ipsec/ovs-monitor-ipsec.in | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in > index dba855af5..bf7b9c6ad 100755 > --- a/ipsec/ovs-monitor-ipsec.in > +++ b/ipsec/ovs-monitor-ipsec.in > @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") > exiting = False > monitor = None > xfrm = None > +command_timeout = None > RECONCILIATION_INTERVAL = 15 # seconds > TIEMOUT_EXPIRED = 37 > > @@ -98,7 +99,7 @@ def run_command(args, description=None): > proc = subprocess.Popen(args, stdout=subprocess.PIPE, > stderr=subprocess.PIPE) > try: > - pout, perr = proc.communicate(timeout=120) > + pout, perr = proc.communicate(timeout=command_timeout) Using a global variable inside a function looks wrong :) Probably still wrong, but moving it in the definition might look better ¯\_(ツ)_/¯ def run_command(args, description=None, timeout=command_timeout): > ret = proc.returncode > except subprocess.TimeoutExpired: > vlog.warn("Command timed out trying to %s." % description) > @@ -1387,6 +1388,10 @@ def main(): > parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", > help="Use DIR/IPSEC-CTL as location for " > " pluto ctl socket (libreswan only).") > + parser.add_argument("--command-timeout", metavar="TIMEOUT", > + type=int, default=120, > + help="Timeout for external commands called by the " > + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") > > ovs.vlog.add_args(parser) > ovs.daemon.add_args(parser) > @@ -1396,11 +1401,13 @@ def main(): > > global monitor > global xfrm > + global command_timeout > > root_prefix = args.root_prefix if args.root_prefix else "" > xfrm = XFRM(root_prefix) > monitor = IPsecMonitor(root_prefix, args.ike_daemon, > not args.no_restart_ike_daemon, args) > + command_timeout = args.command_timeout > > remote = args.database > schema_helper = ovs.db.idl.SchemaHelper() > -- > 2.46.0
On 10/31/24 12:03, Eelco Chaudron wrote: > > > On 30 Oct 2024, at 14:50, Ilya Maximets wrote: > >> Add a new command line option --command-timeout that controls the >> command timeout. It is important to have this configurable, because >> the retransmit-timeout is configurable in Libreswan. Also, users >> may prefer the monitor to be more responsive. >> >> ovs-monitor-ipsec options are not documented anywhere, so not >> trying to address that here. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > See comment below. > >> --- >> ipsec/ovs-monitor-ipsec.in | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in >> index dba855af5..bf7b9c6ad 100755 >> --- a/ipsec/ovs-monitor-ipsec.in >> +++ b/ipsec/ovs-monitor-ipsec.in >> @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") >> exiting = False >> monitor = None >> xfrm = None >> +command_timeout = None >> RECONCILIATION_INTERVAL = 15 # seconds >> TIEMOUT_EXPIRED = 37 >> >> @@ -98,7 +99,7 @@ def run_command(args, description=None): >> proc = subprocess.Popen(args, stdout=subprocess.PIPE, >> stderr=subprocess.PIPE) >> try: >> - pout, perr = proc.communicate(timeout=120) >> + pout, perr = proc.communicate(timeout=command_timeout) > > Using a global variable inside a function looks wrong :) > Probably still wrong, but moving it in the definition might look better ¯\_(ツ)_/¯ > > def run_command(args, description=None, timeout=command_timeout): Yeah, there is no much difference, IMO. But I can change that. > >> ret = proc.returncode >> except subprocess.TimeoutExpired: >> vlog.warn("Command timed out trying to %s." % description) >> @@ -1387,6 +1388,10 @@ def main(): >> parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", >> help="Use DIR/IPSEC-CTL as location for " >> " pluto ctl socket (libreswan only).") >> + parser.add_argument("--command-timeout", metavar="TIMEOUT", >> + type=int, default=120, >> + help="Timeout for external commands called by the " >> + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") >> >> ovs.vlog.add_args(parser) >> ovs.daemon.add_args(parser) >> @@ -1396,11 +1401,13 @@ def main(): >> >> global monitor >> global xfrm >> + global command_timeout >> >> root_prefix = args.root_prefix if args.root_prefix else "" >> xfrm = XFRM(root_prefix) >> monitor = IPsecMonitor(root_prefix, args.ike_daemon, >> not args.no_restart_ike_daemon, args) >> + command_timeout = args.command_timeout >> >> remote = args.database >> schema_helper = ovs.db.idl.SchemaHelper() >> -- >> 2.46.0 >
On 31 Oct 2024, at 16:07, Ilya Maximets wrote: > On 10/31/24 12:03, Eelco Chaudron wrote: >> >> >> On 30 Oct 2024, at 14:50, Ilya Maximets wrote: >> >>> Add a new command line option --command-timeout that controls the >>> command timeout. It is important to have this configurable, because >>> the retransmit-timeout is configurable in Libreswan. Also, users >>> may prefer the monitor to be more responsive. >>> >>> ovs-monitor-ipsec options are not documented anywhere, so not >>> trying to address that here. >>> >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> >> See comment below. >> >>> --- >>> ipsec/ovs-monitor-ipsec.in | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in >>> index dba855af5..bf7b9c6ad 100755 >>> --- a/ipsec/ovs-monitor-ipsec.in >>> +++ b/ipsec/ovs-monitor-ipsec.in >>> @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") >>> exiting = False >>> monitor = None >>> xfrm = None >>> +command_timeout = None >>> RECONCILIATION_INTERVAL = 15 # seconds >>> TIEMOUT_EXPIRED = 37 >>> >>> @@ -98,7 +99,7 @@ def run_command(args, description=None): >>> proc = subprocess.Popen(args, stdout=subprocess.PIPE, >>> stderr=subprocess.PIPE) >>> try: >>> - pout, perr = proc.communicate(timeout=120) >>> + pout, perr = proc.communicate(timeout=command_timeout) >> >> Using a global variable inside a function looks wrong :) >> Probably still wrong, but moving it in the definition might look better ¯\_(ツ)_/¯ >> >> def run_command(args, description=None, timeout=command_timeout): > > > Yeah, there is no much difference, IMO. But I can change that. It both looks ugly, but at least it’s a bit more clear from a function entry point, rather than in function. >> >>> ret = proc.returncode >>> except subprocess.TimeoutExpired: >>> vlog.warn("Command timed out trying to %s." % description) >>> @@ -1387,6 +1388,10 @@ def main(): >>> parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", >>> help="Use DIR/IPSEC-CTL as location for " >>> " pluto ctl socket (libreswan only).") >>> + parser.add_argument("--command-timeout", metavar="TIMEOUT", >>> + type=int, default=120, >>> + help="Timeout for external commands called by the " >>> + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") >>> >>> ovs.vlog.add_args(parser) >>> ovs.daemon.add_args(parser) >>> @@ -1396,11 +1401,13 @@ def main(): >>> >>> global monitor >>> global xfrm >>> + global command_timeout >>> >>> root_prefix = args.root_prefix if args.root_prefix else "" >>> xfrm = XFRM(root_prefix) >>> monitor = IPsecMonitor(root_prefix, args.ike_daemon, >>> not args.no_restart_ike_daemon, args) >>> + command_timeout = args.command_timeout >>> >>> remote = args.database >>> schema_helper = ovs.db.idl.SchemaHelper() >>> -- >>> 2.46.0 >>
On 10/31/24 17:18, Eelco Chaudron wrote: > > > On 31 Oct 2024, at 16:07, Ilya Maximets wrote: > >> On 10/31/24 12:03, Eelco Chaudron wrote: >>> >>> >>> On 30 Oct 2024, at 14:50, Ilya Maximets wrote: >>> >>>> Add a new command line option --command-timeout that controls the >>>> command timeout. It is important to have this configurable, because >>>> the retransmit-timeout is configurable in Libreswan. Also, users >>>> may prefer the monitor to be more responsive. >>>> >>>> ovs-monitor-ipsec options are not documented anywhere, so not >>>> trying to address that here. >>>> >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> >>> See comment below. >>> >>>> --- >>>> ipsec/ovs-monitor-ipsec.in | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in >>>> index dba855af5..bf7b9c6ad 100755 >>>> --- a/ipsec/ovs-monitor-ipsec.in >>>> +++ b/ipsec/ovs-monitor-ipsec.in >>>> @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") >>>> exiting = False >>>> monitor = None >>>> xfrm = None >>>> +command_timeout = None >>>> RECONCILIATION_INTERVAL = 15 # seconds >>>> TIEMOUT_EXPIRED = 37 >>>> >>>> @@ -98,7 +99,7 @@ def run_command(args, description=None): >>>> proc = subprocess.Popen(args, stdout=subprocess.PIPE, >>>> stderr=subprocess.PIPE) >>>> try: >>>> - pout, perr = proc.communicate(timeout=120) >>>> + pout, perr = proc.communicate(timeout=command_timeout) >>> >>> Using a global variable inside a function looks wrong :) >>> Probably still wrong, but moving it in the definition might look better ¯\_(ツ)_/¯ >>> >>> def run_command(args, description=None, timeout=command_timeout): >> >> >> Yeah, there is no much difference, IMO. But I can change that. > > It both looks ugly, but at least it’s a bit more clear from a function entry point, > rather than in function. Ack. Will change. > >>> >>>> ret = proc.returncode >>>> except subprocess.TimeoutExpired: >>>> vlog.warn("Command timed out trying to %s." % description) >>>> @@ -1387,6 +1388,10 @@ def main(): >>>> parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", >>>> help="Use DIR/IPSEC-CTL as location for " >>>> " pluto ctl socket (libreswan only).") >>>> + parser.add_argument("--command-timeout", metavar="TIMEOUT", >>>> + type=int, default=120, >>>> + help="Timeout for external commands called by the " >>>> + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") >>>> >>>> ovs.vlog.add_args(parser) >>>> ovs.daemon.add_args(parser) >>>> @@ -1396,11 +1401,13 @@ def main(): >>>> >>>> global monitor >>>> global xfrm >>>> + global command_timeout >>>> >>>> root_prefix = args.root_prefix if args.root_prefix else "" >>>> xfrm = XFRM(root_prefix) >>>> monitor = IPsecMonitor(root_prefix, args.ike_daemon, >>>> not args.no_restart_ike_daemon, args) >>>> + command_timeout = args.command_timeout >>>> >>>> remote = args.database >>>> schema_helper = ovs.db.idl.SchemaHelper() >>>> -- >>>> 2.46.0 >>> >
On 10/31/24 17:29, Ilya Maximets wrote: > On 10/31/24 17:18, Eelco Chaudron wrote: >> >> >> On 31 Oct 2024, at 16:07, Ilya Maximets wrote: >> >>> On 10/31/24 12:03, Eelco Chaudron wrote: >>>> >>>> >>>> On 30 Oct 2024, at 14:50, Ilya Maximets wrote: >>>> >>>>> Add a new command line option --command-timeout that controls the >>>>> command timeout. It is important to have this configurable, because >>>>> the retransmit-timeout is configurable in Libreswan. Also, users >>>>> may prefer the monitor to be more responsive. >>>>> >>>>> ovs-monitor-ipsec options are not documented anywhere, so not >>>>> trying to address that here. >>>>> >>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> >>>> See comment below. >>>> >>>>> --- >>>>> ipsec/ovs-monitor-ipsec.in | 9 ++++++++- >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in >>>>> index dba855af5..bf7b9c6ad 100755 >>>>> --- a/ipsec/ovs-monitor-ipsec.in >>>>> +++ b/ipsec/ovs-monitor-ipsec.in >>>>> @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") >>>>> exiting = False >>>>> monitor = None >>>>> xfrm = None >>>>> +command_timeout = None >>>>> RECONCILIATION_INTERVAL = 15 # seconds >>>>> TIEMOUT_EXPIRED = 37 >>>>> >>>>> @@ -98,7 +99,7 @@ def run_command(args, description=None): >>>>> proc = subprocess.Popen(args, stdout=subprocess.PIPE, >>>>> stderr=subprocess.PIPE) >>>>> try: >>>>> - pout, perr = proc.communicate(timeout=120) >>>>> + pout, perr = proc.communicate(timeout=command_timeout) >>>> >>>> Using a global variable inside a function looks wrong :) >>>> Probably still wrong, but moving it in the definition might look better ¯\_(ツ)_/¯ >>>> >>>> def run_command(args, description=None, timeout=command_timeout): >>> >>> >>> Yeah, there is no much difference, IMO. But I can change that. >> >> It both looks ugly, but at least it’s a bit more clear from a function entry point, >> rather than in function. > > Ack. Will change. Hrm. Unfortunately, we can't actually do that, because the default value object is created at the moment of function definition, so the value is always the value it was when the script is loaded, i.e. it will be None. I'll keep this part as is for now in this and the previous patches. > >> >>>> >>>>> ret = proc.returncode >>>>> except subprocess.TimeoutExpired: >>>>> vlog.warn("Command timed out trying to %s." % description) >>>>> @@ -1387,6 +1388,10 @@ def main(): >>>>> parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", >>>>> help="Use DIR/IPSEC-CTL as location for " >>>>> " pluto ctl socket (libreswan only).") >>>>> + parser.add_argument("--command-timeout", metavar="TIMEOUT", >>>>> + type=int, default=120, >>>>> + help="Timeout for external commands called by the " >>>>> + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") >>>>> >>>>> ovs.vlog.add_args(parser) >>>>> ovs.daemon.add_args(parser) >>>>> @@ -1396,11 +1401,13 @@ def main(): >>>>> >>>>> global monitor >>>>> global xfrm >>>>> + global command_timeout >>>>> >>>>> root_prefix = args.root_prefix if args.root_prefix else "" >>>>> xfrm = XFRM(root_prefix) >>>>> monitor = IPsecMonitor(root_prefix, args.ike_daemon, >>>>> not args.no_restart_ike_daemon, args) >>>>> + command_timeout = args.command_timeout >>>>> >>>>> remote = args.database >>>>> schema_helper = ovs.db.idl.SchemaHelper() >>>>> -- >>>>> 2.46.0 >>>> >> >
diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in index dba855af5..bf7b9c6ad 100755 --- a/ipsec/ovs-monitor-ipsec.in +++ b/ipsec/ovs-monitor-ipsec.in @@ -83,6 +83,7 @@ vlog = ovs.vlog.Vlog("ovs-monitor-ipsec") exiting = False monitor = None xfrm = None +command_timeout = None RECONCILIATION_INTERVAL = 15 # seconds TIEMOUT_EXPIRED = 37 @@ -98,7 +99,7 @@ def run_command(args, description=None): proc = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) try: - pout, perr = proc.communicate(timeout=120) + pout, perr = proc.communicate(timeout=command_timeout) ret = proc.returncode except subprocess.TimeoutExpired: vlog.warn("Command timed out trying to %s." % description) @@ -1387,6 +1388,10 @@ def main(): parser.add_argument("--ipsec-ctl", metavar="IPSEC-CTL", help="Use DIR/IPSEC-CTL as location for " " pluto ctl socket (libreswan only).") + parser.add_argument("--command-timeout", metavar="TIMEOUT", + type=int, default=120, + help="Timeout for external commands called by the " + "ovs-monitor-ipsec daemon, e.g. ipsec --start.") ovs.vlog.add_args(parser) ovs.daemon.add_args(parser) @@ -1396,11 +1401,13 @@ def main(): global monitor global xfrm + global command_timeout root_prefix = args.root_prefix if args.root_prefix else "" xfrm = XFRM(root_prefix) monitor = IPsecMonitor(root_prefix, args.ike_daemon, not args.no_restart_ike_daemon, args) + command_timeout = args.command_timeout remote = args.database schema_helper = ovs.db.idl.SchemaHelper()
Add a new command line option --command-timeout that controls the command timeout. It is important to have this configurable, because the retransmit-timeout is configurable in Libreswan. Also, users may prefer the monitor to be more responsive. ovs-monitor-ipsec options are not documented anywhere, so not trying to address that here. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- ipsec/ovs-monitor-ipsec.in | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)