Working async POC!!! #19

grafuls · 2018-06-24T21:04:48Z

💥 An example implementation of a working POC with async/await implementation.

ILpinto · 2018-06-25T06:17:44Z

disruption_generator/experiments/example.yaml

@@ -2,13 +2,13 @@
 - disrupt_action:
    - name: Kill services on VM start


I see it for testing - change name for "Kill .." to testing ping to file or something like that.

ILpinto · 2018-06-25T06:28:19Z

disruption_generator/listener/alistener.py

+    async def run_client(self, filepath, expression, timeout):
+        async with await asyncssh.connect(self.hostname) as conn:
+            logger.debug("Connected to %s", self.hostname)
+            stdin, stdout, stderr = await conn.open_session("tail -f %s" % filepath)


tail -f will put all the log to the memory, and if we have file rotate it will not work,
maybe try:
tail -F | grep --line-buffered -A 3 -B 5
Or tail -F | grep -C 5

The receive buffers are cleared everytime we read from stdout.
https://groups.google.com/d/topic/asyncssh-users/i1ouhyekxt8/discussion

Change-Id: I34b4aaf2636bfcaee1ee8299bff048ba4376409a

grafuls · 2018-06-25T08:56:39Z

#6

Change-Id: I54f7b01f103627d266dad00dc93faa19d04ad3ef

jan-zmeskal

Unfortunately I have not had enough time to go through it all properly nor actually execute it. I skimmed through the cli.py mostly, se the review is far from complete. But we are still in early development stage, so we might go back and change things easily if need be.

jan-zmeskal · 2018-06-25T12:37:47Z

disruption_generator/cli.py



 @click.command()
-def main(args=None):
+@click.option(
+    "--experiments-path",


Potential technical debt here described in #24. No need to tackle it the moment thogh.

Agree. We could add an additional option like --experiment.

jan-zmeskal · 2018-06-25T12:47:05Z

disruption_generator/cli.py



 @click.command()
-def main(args=None):
+@click.option(


There's probably an argument why experiments-path should be an argument instead of option. The reasoning has two points:

There's no point in running disruption_generator without this. Options usually just modify behaviour of the program, while executing disruption_generator without experiments-path makes no sense.

The help string IMO makes more sense then:
Usage: disruption_generator [OPTIONS] [EXPERIMENTS_PATH]

It's been implemented as an option since not providing this argument during execution will default to ./experiments (in the context of the execution). We can reconsider removing this and requesting the experiment path as mandatory.

jan-zmeskal · 2018-06-25T14:20:08Z

Pipfile

@@ -16,6 +16,7 @@ Sphinx = "==1.7.1"
 twine = "==1.10.0"
 pytest = "==3.4.2"
 pytest-runner = "==2.11.1"
+pytest-asyncio = "*"


Don't want to impose a debugger in case contributors prefer others. 🌎

jan-zmeskal · 2018-06-25T14:24:30Z

disruption_generator/cli.py

+    "-e",
+    type=click.Path(exists=True, file_okay=False, readable=True, resolve_path=True),
+    help="Path to experiments yamls",
+    default="./experiments/",


This implies that you're in specific path when executing disruption_generator, which is not necessarily the case. I am thinking if this default path can't be made relative to the location of this module?

jan-zmeskal · 2018-06-25T14:28:41Z

disruption_generator/cli.py

+    for (dirpath, dirnames, filenames) in walk(experiments_path):
+        for file in filenames:
+            _files.append(path.join(dirpath, file))
+        break  # Get's only root level directory files


jan-zmeskal · 2018-06-25T14:36:33Z

disruption_generator/cli.py

+        break  # Get's only root level directory files
+    _scenarios = []
+    for _file in _files:
+        _parser = ExperimentParser(yaml_path=_file)


Honestly I am not a huge fan of prefixing var names with '_' unless there's very good reason for it. Sometimes it's a good idea because the var name might e.g. mirror built-in name. Or you want to say everybody not to touch that specific piece of code. But I cannot think of why we should use it here.

from [1]:
“Private” instance variables that cannot be accessed except from inside an object don’t exist in Python. However, there is a convention that is followed by most Python code: a name prefixed with an underscore (e.g. _spam) should be treated as a non-public part of the API (whether it is a function, a method or a data member).

[1] https://docs.python.org/2/tutorial/classes.html#private-variables-and-class-local-references

jan-zmeskal · 2018-06-25T14:39:52Z

disruption_generator/cli.py

+async def execute(experiments_path):
+    _files = []
+    for (dirpath, dirnames, filenames) in walk(experiments_path):
+        for file in filenames:


This mirrors python2 built-in: https://docs.python.org/2/library/functions.html#file It's okay since we're using python3, but I would still rename it to f, that's common python convention

Good point, forgot to prefix with _

jan-zmeskal · 2018-06-25T14:45:13Z

disruption_generator/parsers/experiment_parser.py

-            yield disruption
+            _disruptions.append(disruption)
+
+        return _disruptions


I was thinking that maybe we could call here scenario as well since we call it like that in main method. I don't really mind why var name we use and I am not gonna argue about variable names. But labeling the same thing with the same name at different places really makes it easier for contributors to visually parse a program.

Speaking of which, it seems to me that it would me a lot of sense to create Scenario class that would be collection of Disruptions, if that is the intent.

Agree on refactoring to scenario. No need for a Scenario class at the moment since there is no additional metadata related to it. We can consider it in the future if necessary.

jan-zmeskal · 2018-06-25T15:17:08Z

disruption_generator/cli.py

+        _scenarios.extend(scenario)
+    for scenario in _scenarios:
+        click.echo("Scenario: %s" % scenario)
+        alistener = Alistener(scenario.listener.target)


I have to say that is's a bit confusing that scenario variable (which holds Disruption object in this case) has listener attribute which is something completely different than Alistener. Listener class just holds some text data so I suppose it could be called differently.

jan-zmeskal · 2018-06-25T15:19:42Z

disruption_generator/cli.py

+        alistener = Alistener(scenario.listener.target)
+
+        for action in scenario.actions:
+            result = await alistener.tail(


Please let's generally use this format: result = await alistener.tail(filepath=scenario.listener.log, expression=scenario.listener.re, timeout=action.timeout). Once again it really helps to quickly parse it visually.

Change-Id: I10a21489604f4702ce83f90bcf37d7c6106ce47b

grafuls · 2018-06-28T08:55:18Z

disruption_generator/parsers/utils.py

@@ -7,6 +7,7 @@ class Action(object):
    """
    A trigger object with the requested disruptive action.
    """
+
    name = attr.ib(validator=attr.validators.instance_of(six.text_type))


consider removing six since we are not using python2

grafuls added 6 commits June 23, 2018 20:10

Added async listener

e5e7839

Additional changes to alistener and cli

29a2b8f

Added logic for glue method

05e0cd8

Additional changes plus fade to black

c49b251

Fixed tests

16fff51

Removed 3.4 references in favor of async/await syntax

80561a6

grafuls added this to the POC milestone Jun 24, 2018

grafuls requested review from jan-zmeskal and ILpinto June 24, 2018 21:04

grafuls changed the title ~~Working async POC!!!~~ 💥 Working async POC!!! Jun 24, 2018

grafuls changed the title ~~💥 Working async POC!!!~~ Working async POC!!! Jun 24, 2018

grafuls added 2 commits June 24, 2018 23:46

Fixes to tests

cb6da6b

Fixes to long polling cmds

0542c1e

ILpinto reviewed Jun 25, 2018

View reviewed changes

Fixing PR comments

4a172cc

Change-Id: I34b4aaf2636bfcaee1ee8299bff048ba4376409a

Fixed timeout handling on alistener

e2be317

Change-Id: I54f7b01f103627d266dad00dc93faa19d04ad3ef

jan-zmeskal reviewed Jun 25, 2018

View reviewed changes

grafuls added 2 commits June 26, 2018 15:49

Fixes to setup and cli

66a1fbd

Change-Id: I10a21489604f4702ce83f90bcf37d7c6106ce47b

Comments from PR

35f087c

grafuls commented Jun 28, 2018

View reviewed changes

grafuls requested review from KwisatzHaderach and petr-balogh June 28, 2018 10:48

grafuls self-assigned this Jun 28, 2018

grafuls added the enhancement New feature or request label Jun 28, 2018

ILpinto approved these changes Jun 28, 2018

View reviewed changes

grafuls merged commit 039d7d7 into master Jun 28, 2018

grafuls deleted the alistener branch June 28, 2018 13:30

grafuls mentioned this pull request Jun 29, 2018

[Bug] watch_for_local_changes needs refactoring #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Working async POC!!! #19

Working async POC!!! #19

grafuls commented Jun 24, 2018 •

edited

Loading

ILpinto Jun 25, 2018

ILpinto Jun 25, 2018

grafuls Jun 25, 2018

grafuls commented Jun 25, 2018

jan-zmeskal left a comment

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 27, 2018

jan-zmeskal Jun 25, 2018

jan-zmeskal Jun 25, 2018

grafuls Jun 28, 2018

		@@ -2,13 +2,13 @@
		- disrupt_action:
		- name: Kill services on VM start

Working async POC!!! #19

Working async POC!!! #19

Conversation

grafuls commented Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafuls commented Jun 25, 2018

jan-zmeskal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafuls commented Jun 24, 2018 •

edited

Loading