[PATCH 1/4] Machine: fix timeout kill_command for netns commands

[Ondrej Lichtner]

From: Ondrej Lichtner <olichtne(a)redhat.com&gt;

If a command was running in a network namespace and timed out, the
kill_command rpc request was sent to the main lnst-slave process instead
of the netns one. This commit fixes that.

Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
---
 lnst/Controller/Machine.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
index c360e6c..8a40ceb 100644
--- a/lnst/Controller/Machine.py
+++ b/lnst/Controller/Machine.py
@@ -302,10 +302,20 @@ class Machine(object):
             else:
                 cmd_res = self._rpc_call("run_command", command)
         except MachineError as exc:
-            if "proc_id" in command:
-                cmd_res = self._rpc_call("kill_command",
command["proc_id"])
+            if command["netns"] is not None:
+                netns = command["netns"]
+                if "proc_id" in command:
+                    cmd_res = self._rpc_call_to_netns(netns, "kill_command",
+                                                      command["proc_id"])
+                else:
+                    cmd_res = self._rpc_call_to_netns(netns, "kill_command",
+                                                      None)
             else:
-                cmd_res = self._rpc_call("kill_command", None)
+                if "proc_id" in command:
+                    cmd_res = self._rpc_call("kill_command",
command["proc_id"])
+                else:
+                    cmd_res = self._rpc_call("kill_command", None)
+
             cmd_res["passed"] = False
             cmd_res["msg"] = str(exc)
 
-- 
2.5.0

[PATCH 2/4] NetTestCommand: add set_finished method

[Ondrej Lichtner]

From: Ondrej Lichtner <olichtne(a)redhat.com&gt;

This commit adds the set_finished method to the NetTestCommand class. By
default it set's the _finished attribute to True, but takes an optional
parameter if you want to clear it (doesn't have a use right now).

Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
---
 lnst/Common/NetTestCommand.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lnst/Common/NetTestCommand.py b/lnst/Common/NetTestCommand.py
index 0ce8998..c24110e 100644
--- a/lnst/Common/NetTestCommand.py
+++ b/lnst/Common/NetTestCommand.py
@@ -111,6 +111,9 @@ class NetTestCommand:
     def forked(self):
         return self._process != None
 
+    def set_finished(self, value=True):
+        self._finished = value
+
     def finished(self):
         return self._finished
 
-- 
2.5.0

[PATCH 3/4] NetTestCommand: get_cmd return None if no command found

[Ondrej Lichtner]

From: Ondrej Lichtner <olichtne(a)redhat.com&gt;

The get_cmd method of the NetTestCommandContext class now returns a None
object instead of throwing an exception when no command with the
specified id is found.

Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
---
 lnst/Common/NetTestCommand.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lnst/Common/NetTestCommand.py b/lnst/Common/NetTestCommand.py
index c24110e..6944051 100644
--- a/lnst/Common/NetTestCommand.py
+++ b/lnst/Common/NetTestCommand.py
@@ -234,7 +234,10 @@ class NetTestCommandContext:
         del self._dict[cmd.get_id()]
 
     def get_cmd(self, id):
-        return self._dict[id]
+        if id in self._dict:
+            return self._dict[id]
+        else:
+            return None
 
     def _kill_all_cmds(self):
         for id in self._dict:
-- 
2.5.0

[PATCH 4/4] NetTestSlave: check if command finished before killing

[Ondrej Lichtner]

From: Ondrej Lichtner <olichtne(a)redhat.com&gt;

This commit makes shure that when the rpc method kill_command is called,
it first checks if the command didn't finish first. If it did, it will
only be removed from the command context. This situation can occur when
the Controller detects a command timeout and sends a kill_command
request, but on the Slave the command is already finished and the result
was sent in the mean time.

This commit should fix this issue and the command will be seen as
finished on time. If the command was killed we now also add a killed
flag to the result message to indicate this action.

Related to #120

Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
---
 lnst/Common/NetTestCommand.py |  1 +
 lnst/Controller/Machine.py    |  5 +++--
 lnst/Slave/NetTestSlave.py    | 25 ++++++++++++++++++++-----
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/lnst/Common/NetTestCommand.py b/lnst/Common/NetTestCommand.py
index 6944051..1489794 100644
--- a/lnst/Common/NetTestCommand.py
+++ b/lnst/Common/NetTestCommand.py
@@ -208,6 +208,7 @@ class NetTestCommand:
             result = self._cmd_cls.get_result()
             result["passed"] = True
             result["msg"] = "Command killed."
+            result["killed"] = True
             self._result = result
 
         return self._result
diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
index 8a40ceb..e60e1ea 100644
--- a/lnst/Controller/Machine.py
+++ b/lnst/Controller/Machine.py
@@ -316,8 +316,9 @@ class Machine(object):
                 else:
                     cmd_res = self._rpc_call("kill_command", None)
 
-            cmd_res["passed"] = False
-            cmd_res["msg"] = str(exc)
+            if "killed" in cmd_res and cmd_res["killed"]:
+                cmd_res["passed"] = False
+                cmd_res["msg"] = str(exc)
 
         signal.alarm(0)
         signal.signal(signal.SIGALRM, prev_handler)
diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py
index a725a62..502abbf 100644
--- a/lnst/Slave/NetTestSlave.py
+++ b/lnst/Slave/NetTestSlave.py
@@ -366,7 +366,18 @@ class SlaveMethods:
     def run_command(self, command):
         cmd = NetTestCommand(self._command_context, command,
                                     self._resource_table, self._log_ctl)
-        self._command_context.add_cmd(cmd)
+
+        if self._command_context.get_cmd(cmd.get_id()) != None:
+            prev_cmd = self._command_context.get_cmd(cmd.get_id())
+            if not prev_cmd.finished():
+                if cmd.get_id() is None:
+                    raise Exception("Previous foreground command still "\
+                                    "running!")
+                else:
+                    raise Exception("Different command with id '%s' "\
+                                    "still running!" % cmd.get_id())
+        else:
+            self._command_context.add_cmd(cmd)
 
         res = cmd.run()
         if not cmd.forked():
@@ -385,9 +396,13 @@ class SlaveMethods:
 
     def kill_command(self, id):
         cmd = self._command_context.get_cmd(id)
-        cmd.kill(None)
-        self._command_context.del_cmd(cmd)
-        return cmd.get_result()
+        if cmd is not None:
+            self._command_context.del_cmd(cmd)
+            if not cmd.finished():
+                cmd.kill(None)
+                return cmd.get_result()
+        else:
+            raise Exception("No command with id '%s'." % id)
 
     def machine_cleanup(self):
         logging.info("Performing machine cleanup.")
@@ -852,7 +867,7 @@ class NetTestSlave:
                 self._server_handler.send_data_to_ctl(msg)
                 cmd = self._cmd_context.get_cmd(None)
                 cmd.join()
-                self._cmd_context.del_cmd(cmd)
+                cmd.set_finished()
             else:
                 cmd = self._cmd_context.get_cmd(msg["cmd_id"])
                 cmd.join()
-- 
2.5.0

[PATCH 4/4] NetTestSlave: check if command finished before killing

[Ondrej Lichtner]

On Thu, Aug 06, 2015 at 09:00:35AM +0200, olichtne(a)redhat.com wrote:
>  From: Ondrej Lichtner <olichtne(a)redhat.com&gt;
>  
>  This commit makes shure that when the rpc method kill_command is called,
>  it first checks if the command didn't finish first. If it did, it will
>  only be removed from the command context. This situation can occur when
>  the Controller detects a command timeout and sends a kill_command
>  request, but on the Slave the command is already finished and the result
>  was sent in the mean time.
>  
>  This commit should fix this issue and the command will be seen as
>  finished on time. If the command was killed we now also add a killed
>  flag to the result message to indicate this action.
>  
>  Related to #120
>  
>  Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
>  ---
>   lnst/Common/NetTestCommand.py |  1 +
>   lnst/Controller/Machine.py    |  5 +++--
>   lnst/Slave/NetTestSlave.py    | 25 ++++++++++++++++++++-----
>   3 files changed, 24 insertions(+), 7 deletions(-)
>  
>  diff --git a/lnst/Common/NetTestCommand.py b/lnst/Common/NetTestCommand.py
>  index 6944051..1489794 100644
>  --- a/lnst/Common/NetTestCommand.py
>  +++ b/lnst/Common/NetTestCommand.py
>  @@ -208,6 +208,7 @@ class NetTestCommand:
>               result = self._cmd_cls.get_result()
>               result["passed"] = True
>               result["msg"] = "Command killed."
>  +            result["killed"] = True
>               self._result = result
>   
>           return self._result
>  diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
>  index 8a40ceb..e60e1ea 100644
>  --- a/lnst/Controller/Machine.py
>  +++ b/lnst/Controller/Machine.py
>  @@ -316,8 +316,9 @@ class Machine(object):
>                   else:
>                       cmd_res = self._rpc_call("kill_command", None)
>   
>  -            cmd_res["passed"] = False
>  -            cmd_res["msg"] = str(exc)
>  +            if "killed" in cmd_res and cmd_res["killed"]:
>  +                cmd_res["passed"] = False
>  +                cmd_res["msg"] = str(exc)
>   
>           signal.alarm(0)
>           signal.signal(signal.SIGALRM, prev_handler)
>  diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py
>  index a725a62..502abbf 100644
>  --- a/lnst/Slave/NetTestSlave.py
>  +++ b/lnst/Slave/NetTestSlave.py
>  @@ -366,7 +366,18 @@ class SlaveMethods:
>       def run_command(self, command):
>           cmd = NetTestCommand(self._command_context, command,
>                                       self._resource_table, self._log_ctl)
>  -        self._command_context.add_cmd(cmd)
>  +
>  +        if self._command_context.get_cmd(cmd.get_id()) != None:
>  +            prev_cmd = self._command_context.get_cmd(cmd.get_id())
>  +            if not prev_cmd.finished():
>  +                if cmd.get_id() is None:
>  +                    raise Exception("Previous foreground command still "\
>  +                                    "running!")
>  +                else:
>  +                    raise Exception("Different command with id '%s'
> "\
>  +                                    "still running!" % cmd.get_id())
>  +        else:
>  +            self._command_context.add_cmd(cmd)
>   
>           res = cmd.run()
>           if not cmd.forked():
>  @@ -385,9 +396,13 @@ class SlaveMethods:
>   
>       def kill_command(self, id):
>           cmd = self._command_context.get_cmd(id)
>  -        cmd.kill(None)
>  -        self._command_context.del_cmd(cmd)
>  -        return cmd.get_result()
>  +        if cmd is not None:
>  +            self._command_context.del_cmd(cmd)
>  +            if not cmd.finished():
>  +                cmd.kill(None)
>  +                return cmd.get_result()
>  +        else:
>  +            raise Exception("No command with id '%s'." % id)
>   
>       def machine_cleanup(self):
>           logging.info("Performing machine cleanup.")
>  @@ -852,7 +867,7 @@ class NetTestSlave:
>                   self._server_handler.send_data_to_ctl(msg)
>                   cmd = self._cmd_context.get_cmd(None)
>                   cmd.join()
>  -                self._cmd_context.del_cmd(cmd)
>  +                cmd.set_finished()
>               else:
>                   cmd = self._cmd_context.get_cmd(msg["cmd_id"])
>                   cmd.join()
>  -- 
>  2.5.0
>   
This patch causes regressions that somehow didn't show up when I first
ran the regression tests so ignore this patch set for now...

[Jan Tluka]

Thu, Aug 06, 2015 at 09:00:32AM CEST, olichtne(a)redhat.com wrote:
> From: Ondrej Lichtner <olichtne(a)redhat.com&gt;
>
> If a command was running in a network namespace and timed out, the
> kill_command rpc request was sent to the main lnst-slave process instead
> of the netns one. This commit fixes that.
>
> Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
> ---
>  lnst/Controller/Machine.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
> index c360e6c..8a40ceb 100644
> --- a/lnst/Controller/Machine.py
> +++ b/lnst/Controller/Machine.py
> @@ -302,10 +302,20 @@ class Machine(object):
>              else:
>                  cmd_res = self._rpc_call("run_command", command)
>          except MachineError as exc:
> -            if "proc_id" in command:
> -                cmd_res = self._rpc_call("kill_command",
> command["proc_id"])
> +            if command["netns"] is not None:
> +                netns = command["netns"]
> +                if "proc_id" in command:
> +                    cmd_res = self._rpc_call_to_netns(netns, "kill_command",
> +                                                      command["proc_id"])
> +                else:
> +                    cmd_res = self._rpc_call_to_netns(netns, "kill_command",
> +                                                      None) 
Why we need the None version? What would be killed if None is passed as
parameter?

>              else:
> -                cmd_res = self._rpc_call("kill_command", None)
> +                if "proc_id" in command:
> +                    cmd_res = self._rpc_call("kill_command",
> command["proc_id"])
> +                else:
> +                    cmd_res = self._rpc_call("kill_command", None)
> +
>              cmd_res["passed"] = False
>              cmd_res["msg"] = str(exc)
>  
> -- 
> 2.5.0
>
> _______________________________________________
> LNST-developers mailing list
> LNST-developers(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers

[Jan Tluka]

Thu, Aug 06, 2015 at 11:44:48AM CEST, jtluka(a)redhat.com wrote:
> Thu, Aug 06, 2015 at 09:00:32AM CEST, olichtne(a)redhat.com wrote:
> >From: Ondrej Lichtner <olichtne(a)redhat.com&gt;
> >
> >If a command was running in a network namespace and timed out, the
> >kill_command rpc request was sent to the main lnst-slave process instead
> >of the netns one. This commit fixes that.
> >
> >Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com&gt;
> >---
> > lnst/Controller/Machine.py | 16 +++++++++++++---
> > 1 file changed, 13 insertions(+), 3 deletions(-)
> >
> >diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
> >index c360e6c..8a40ceb 100644
> >--- a/lnst/Controller/Machine.py
> >+++ b/lnst/Controller/Machine.py
> >@@ -302,10 +302,20 @@ class Machine(object):
> >             else:
> >                 cmd_res = self._rpc_call("run_command", command)
> >         except MachineError as exc:
> >-            if "proc_id" in command:
> >-                cmd_res = self._rpc_call("kill_command",
> command["proc_id"])
> >+            if command["netns"] is not None:
> >+                netns = command["netns"]
> >+                if "proc_id" in command:
> >+                    cmd_res = self._rpc_call_to_netns(netns,
> "kill_command",
> >+                                                      command["proc_id"])
> >+                else:
> >+                    cmd_res = self._rpc_call_to_netns(netns,
> "kill_command",
> >+                                                      None)
>
> Why we need the None version? What would be killed if None is passed as
> parameter?
>  
Oh, so there are subsequent patches in the series that explains this.
Looks ok.

> >             else:
> >-                cmd_res = self._rpc_call("kill_command", None)
> >+                if "proc_id" in command:
> >+                    cmd_res = self._rpc_call("kill_command",
> command["proc_id"])
> >+                else:
> >+                    cmd_res = self._rpc_call("kill_command", None)
> >+
> >             cmd_res["passed"] = False
> >             cmd_res["msg"] = str(exc)
> > 
> >-- 
> >2.5.0
> >
> >_______________________________________________
> >LNST-developers mailing list
> &gt;LNST-developers(a)lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/lnst-developers