Repository: mesos Updated Branches: refs/heads/master 10908df71 -> 77921200a
Refactored base of Python CLI tests. This patch mostly adds additional hardening to make sure we error out with proper error messages in places where we weren't before. However, as part of this, we also fixed a bug related to referencing counting Master, Agent, and Task objects. Previously, this reference count would increased in '__init__()' and decreased in '__del__()'. However, this caused problems when limiting the number of agents and masters to exactly 1, because we could never guarantee when the python garbage collector would kick in to delete objects from previous test runs. This manifested itself as flaky test failures. To fix this, we now reference count only on explicit counts to 'launch()' and 'kill()' instead of in '__init__()' and '__del__()'. Review: https://reviews.apache.org/r/67842/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/77921200 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/77921200 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/77921200 Branch: refs/heads/master Commit: 77921200a69564f966917bfc8e07a3d1e3ada196 Parents: 10908df Author: Armand Grillet <agril...@mesosphere.io> Authored: Fri Jul 6 15:52:28 2018 +0200 Committer: Kevin Klues <klue...@gmail.com> Committed: Fri Jul 6 16:58:43 2018 +0200 ---------------------------------------------------------------------- src/python/cli_new/lib/cli/tests/base.py | 59 +++++++++++++++++---------- 1 file changed, 38 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/77921200/src/python/cli_new/lib/cli/tests/base.py ---------------------------------------------------------------------- diff --git a/src/python/cli_new/lib/cli/tests/base.py b/src/python/cli_new/lib/cli/tests/base.py index 4ffa27c..89360e6 100644 --- a/src/python/cli_new/lib/cli/tests/base.py +++ b/src/python/cli_new/lib/cli/tests/base.py @@ -92,7 +92,7 @@ class Executable(object): self.proc = None def __del__(self): - if hasattr(self, "proc"): + if hasattr(self, "proc") and self.proc is not None: self.kill() def launch(self): @@ -103,6 +103,10 @@ class Executable(object): raise CLIException("{name} already launched" .format(name=self.name.capitalize())) + if not os.path.exists(self.executable): + raise CLIException("{name} executable not found" + .format(name=self.name.capitalize())) + try: flags = ["--{key}={value}".format(key=key, value=value) for key, value in self.flags.iteritems()] @@ -137,6 +141,8 @@ class Executable(object): return try: + self.proc.stdin.close() + self.proc.stdout.close() self.proc.kill() self.proc.wait() self.proc = None @@ -159,8 +165,6 @@ class Master(Executable): raise CLIException("Creating more than one master" " is currently not possible") - Master.count += 1 - if flags is None: flags = {} @@ -183,9 +187,24 @@ class Master(Executable): def __del__(self): super(Master, self).__del__() - if hasattr(self, "flags"): + if hasattr(self, "flags") and hasattr(self.flags, "work_dir"): shutil.rmtree(self.flags["work_dir"]) + # pylint: disable=arguments-differ + def launch(self): + """ + After starting the master, we need to make sure its + reference count is increased. + """ + super(Master, self).launch() + Master.count += 1 + + def kill(self): + """ + After killing the master, we need to make sure its + reference count is decreased. + """ + super(Master, self).kill() Master.count -= 1 @@ -203,8 +222,6 @@ class Agent(Executable): raise CLIException("Creating more than one agent" " is currently not possible") - Agent.count += 1 - if flags is None: flags = {} @@ -236,19 +253,20 @@ class Agent(Executable): def __del__(self): super(Agent, self).__del__() - if hasattr(self, "flags"): + if hasattr(self, "flags") and hasattr(self.flags, "work_dir"): shutil.rmtree(self.flags["work_dir"]) + if hasattr(self, "flags") and hasattr(self.flags, "runtime_dir"): shutil.rmtree(self.flags["runtime_dir"]) - Agent.count -= 1 - # pylint: disable=arguments-differ def launch(self, timeout=TIMEOUT): """ - After starting the agent, we need to make sure it has + After starting the agent, we first need to make sure its + reference count is increased and then check that it has successfully registered with the master before proceeding. """ super(Agent, self).launch() + Agent.count += 1 try: # pylint: disable=missing-docstring @@ -260,7 +278,6 @@ class Agent(Executable): stdout = "" if self.proc.poll(): stdout = "\n{output}".format(output=self.proc.stdout.read()) - self.proc = None raise CLIException("Could not get '/slaves' endpoint as JSON with" " only 1 agent in it: {error}{stdout}" @@ -274,20 +291,21 @@ class Agent(Executable): """ super(Agent, self).kill() - if self.proc is None: - return - try: # pylint: disable=missing-docstring - def no_slaves(data): - return len(data["slaves"]) == 0 + def one_inactive_slave(data): + slaves = data["slaves"] + return len(slaves) == 1 and not slaves[0]["active"] - http.get_json(self.flags["master"], "slaves", no_slaves, timeout) + http.get_json( + self.flags["master"], "slaves", one_inactive_slave, timeout) except Exception as exception: raise CLIException("Could not get '/slaves' endpoint as" " JSON with 0 agents in it: {error}" .format(error=exception)) + Agent.count -= 1 + class Task(Executable): """ @@ -308,7 +326,6 @@ class Task(Executable): port=TEST_MASTER_PORT) if "name" not in flags: flags["name"] = "task-{id}".format(id=Task.count) - Task.count += 1 if "command" not in flags: raise CLIException("No command supplied when creating task") @@ -361,6 +378,7 @@ class Task(Executable): has actually been added to the agent before proceeding. """ super(Task, self).launch() + Task.count += 1 try: # pylint: disable=missing-docstring @@ -389,9 +407,6 @@ class Task(Executable): """ super(Task, self).kill() - if self.proc is None: - return - try: # pylint: disable=missing-docstring def container_does_not_exist(data): @@ -405,6 +420,8 @@ class Task(Executable): .format(name=self.flags["name"], error=exception)) + Task.count -= 1 + def capture_output(command, argv, extra_args=None): """
