Added recorder and an example (to be removed) #296

dbarrosop · 2017-08-07T22:39:21Z

This PR introduces a class that can sit between the driver and the underlying library (pyeapi, netmiko, pyez, etc. see napalm-automation/napalm-eos/pull/173).

The "middleware" has three modes:

pass. Does nothing. The Recorder will just proxy the calls to the underlying library.
record. The interactions between the driver and the underlying library are proxied and recorded.
replay. The interactions between the driver and the underlying library are interrupted. Instead, the Recorder will pickle a previous recording and replay it.

Example, in the following snippet we are running the same code three times testing all modes. Note that in the "replay" mode at the end I set wrong data to prove that there is no connection with the real device.

(napalm) ➜  napalm-base git:(recorder) ✗ python test.py
{u'fqdn': u'localhost',
 u'hostname': u'localhost',
 u'interface_list': [u'Ethernet1', u'Ethernet2', u'Management1'],
 u'model': u'vEOS',
 u'os_version': u'4.15.2.1F-2759627.41521F',
 u'serial_number': u'',
 u'uptime': 4147,
 u'vendor': u'Arista'}
{u'Ethernet1': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0919724,
                u'mac_address': u'08:00:27:34:90:5B',
                u'speed': 0},
 u'Ethernet2': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0921075,
                u'mac_address': u'08:00:27:07:C3:F0',
                u'speed': 0},
 u'Management1': {u'description': u'',
                  u'is_enabled': True,
                  u'is_up': True,
                  u'last_flapped': 1502141358.069374,
                  u'mac_address': u'08:00:27:7D:44:C1',
                  u'speed': 1000}}
napalm-base - DEBUG - Recording run_commands
napalm-base - DEBUG - Recording run_commands
{u'fqdn': u'localhost',
 u'hostname': u'localhost',
 u'interface_list': [u'Ethernet1', u'Ethernet2', u'Management1'],
 u'model': u'vEOS',
 u'os_version': u'4.15.2.1F-2759627.41521F',
 u'serial_number': u'',
 u'uptime': 4147,
 u'vendor': u'Arista'}
napalm-base - DEBUG - Recording run_commands
{u'Ethernet1': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0919635,
                u'mac_address': u'08:00:27:34:90:5B',
                u'speed': 0},
 u'Ethernet2': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0921032,
                u'mac_address': u'08:00:27:07:C3:F0',
                u'speed': 0},
 u'Management1': {u'description': u'',
                  u'is_enabled': True,
                  u'is_up': True,
                  u'last_flapped': 1502141358.0693803,
                  u'mac_address': u'08:00:27:7D:44:C1',
                  u'speed': 1000}}
napalm-base - DEBUG - Replaying run_commands
napalm-base - DEBUG - Replaying run_commands
{u'fqdn': u'localhost',
 u'hostname': u'localhost',
 u'interface_list': [u'Ethernet1', u'Ethernet2', u'Management1'],
 u'model': u'vEOS',
 u'os_version': u'4.15.2.1F-2759627.41521F',
 u'serial_number': u'',
 u'uptime': 4147,
 u'vendor': u'Arista'}
napalm-base - DEBUG - Replaying run_commands
{u'Ethernet1': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0919635,
                u'mac_address': u'08:00:27:34:90:5B',
                u'speed': 0},
 u'Ethernet2': {u'description': u'',
                u'is_enabled': True,
                u'is_up': True,
                u'last_flapped': 1502141344.0921032,
                u'mac_address': u'08:00:27:07:C3:F0',
                u'speed': 0},
 u'Management1': {u'description': u'',
                  u'is_enabled': True,
                  u'is_up': True,
                  u'last_flapped': 1502141358.0693803,
                  u'mac_address': u'08:00:27:7D:44:C1',
                  u'speed': 1000}}

TODO:

Make sure we can capture exceptions as well
Decide if we want to use pickle or serialize/deserialize objects. 99% of the interactions are going to be via dicts, json or xml so it should be doable. Exceptions might be trickier with doable as well.

dbarrosop · 2017-08-07T22:50:04Z

@ubaumann @mirceaulinic @ktbyers thoughts?

ubaumann · 2017-08-08T06:39:39Z

With this approach we could address the 'more verbose logging' issue as well: #272 #273

I will look deeper in the code asap

ubaumann · 2017-08-08T08:58:10Z

My findings so far:

debugging will be harder for newbie. (Debug pointer is jumping around thru recoder function)
We should not create the real object in 'replay' mode. Netmiko tries to establish the connection in the constructor and will fail

...
  File "/home/ubaumann/napalm-base/napalm_base/recorder.py", line 57, in __init__
    self.device = cls(*args, **kwargs)
  File "/home/ubaumann/.virtualenvs/napalm_base/lib/python2.7/site-packages/netmiko/ssh_dispatcher.py", line 131, in ConnectHandler
    return ConnectionClass(*args, **kwargs)
  File "/home/ubaumann/.virtualenvs/napalm_base/lib/python2.7/site-packages/netmiko/base_connection.py", line 150, in __init__
    self.establish_connection()
  File "/home/ubaumann/.virtualenvs/napalm_base/lib/python2.7/site-packages/netmiko/base_connection.py", line 506, in establish_connection
    raise NetMikoTimeoutException(msg)
netmiko.ssh_exception.NetMikoTimeoutException: Connection to device timed-out: cisco_ios 127.0.0.1:123

With the files and the count number, the order of commands can't be changed. So, every workflow needs a separate record. I would like to have a more flexible way. For example creating a YAML file with the function name and the arguments, so the the recording can be reused. I know the file could get big.

send_command:
  - 'show version': 'asdfasdfasdfasdf'
  - 'show interface': 'asdfasdfasdfasdf'
enable: ''

Advantages from my point of view:

More readable then with pickle.dumps
Reusable / more flexible
Just one file to handel for each wrapped object

Recordings make it easy to test against different OS versions

mirceaulinic · 2017-08-08T09:03:07Z

napalm_base/recorder.py

+    logger.debug("Recording {}".format(func.__name__))
+    r = func(*args, **kwargs)
+    filename = "{}.{}".format(func.__name__, cls.current_count)
+    with open(os.path.join(cls.path, filename), 'w') as f:


This should scream as a resource leakage.

Also, this is saving a file under the Python sys paths (where the lib is installed) - not good.

This should scream as a resource leakage

Care to elaborate?

Also, this is saving a file under the Python sys paths (where the lib is installed) - not good.

path is specified by the user. Not sure what you mean.

mirceaulinic · 2017-08-08T09:03:50Z

napalm_base/recorder.py

+def record(cls, func, *args, **kwargs):
+    logger.debug("Recording {}".format(func.__name__))
+    r = func(*args, **kwargs)
+    filename = "{}.{}".format(func.__name__, cls.current_count)


I'd probably use the FQDN (the host variable sent during init) rather than a spin number, so you can have a better understanding on what commands have been executed where.

path is specified by the user via optional_arg["recorder_path"], if you want to use hostname, use it. You are not limited by anyone making any decissions for you. The code you are pointing out is just a deterministic way of finding the order of the calls.

mirceaulinic

I kinda like the idea, but if I'd compare the benefits vs. complexity + risks and environment exposure, I find there's not much real value added.

There's also the long-vehiculated argument: this should probably happen in a higher level tool, napalm is just a library :]

mirceaulinic · 2017-08-08T09:26:04Z

With the files and the count number, the order of commands can't be changed. So, every workflow needs a separate record. I would like to have a more flexible way. For example creating a YAML file with the function name and the arguments, so the the recording can be reused. I know the file could get big.

Yes, I believe this is way, way, beyond the goals of NAPALM - as @dbarrosop likes to say everytime, "napalm is a library, it's not a tool.", so this implementation, from any perspective, I believe it doesn't really fit into this definition.

dbarrosop · 2017-08-08T09:55:28Z

We should not create the real object in 'replay' mode

I agree. That's certainly an easy bug to fix.

With the files and the count number, the order of commands can't be changed

That's the point. This is a recorder/replayer.

I kinda like the idea, but if I'd compare the benefits vs. complexity + risks and environment exposure, I find there's not much real value added.

Complexity, less than what we have today. Benefits of having this between each driver and the underlying class (netmiko, pyeapi, etc):

Test cases are easily recorded. Just run the test against a real device in recording mode and you are done.
With these 60 lines of code, including whitespaces and comments, you can say goodbye to all the mocking code we have today, which is a lot and vendor dependent; MockDriver, FakeEOS, FakeJunos, Fakeblah, all gone.
Debugging, something breaks? Just enable recording mode and check what's happening, the scenario can even be shared on an issue/branch/PR for us to debug and fix.
Compliance, I am sure compliance people would like it.

An alternative to the "pass" behavior would be to call the class directly without the wrapper so the default behavior wouldn't change at all. So on the drivers we would have:

                if self.recorder_mode == "pass":
                    self.device = pyeapi.client.Node(connection, enablepwd=self.enablepwd)
                else:
                      self.device = napalm_base.recorder.Recorder(pyeapi.client.Node,
                                                               recorder_options=self.recorder_options,
                                                               connection=connection,
                                                               enablepwd=self.enablepwd)

Or something like that

itdependsnetworks · 2017-08-09T07:37:28Z

My take:

More debugging is helpful. Have run into issues that are masked
Was looking for the record feature of this. It is helpful to see the raw data and be able to keep it for historical purposes
Can use the cli to backup commands not in data model. e.g. have a requirement to log: sh hardware internal errors module 1 \| diff \| i crc
If we add a way to anonymize confidential data (e.g. IP, descriptions, etc..) potentially make it easier for users to contribute mock data.

IMHO unless this introduced an unnecessary amount of work and future burden I see it as clear net positive.

ktbyers · 2017-08-10T18:47:56Z

@itdependsnetworks Can you elaborate on this statement?

Can use the cli to backup commands not in data model. e.g. have a requirement to log: sh hardware internal errors module 1 \| diff \| i crc

I think we should use probably use JSON or YAML to save data instead of pickle.

I like the general idea.

itdependsnetworks · 2017-08-10T19:35:15Z

I'm simply highlighting he fact that that command will never be part of any standard getters. It would be nice to have a standard way to say the raw response no matter what you were running

dbarrosop · 2017-08-11T07:07:23Z

I think we should use probably use JSON or YAML to save data instead of pickle.

Problem with YAML/JSON is that it becomes a problem when the method returns pythons objects. For example, a method might return None or True, an Exception or even a custom object, and those don't translate well to structured data. But I will do some tests and see what happens.

ktbyers · 2017-08-11T16:31:51Z

If we use Pickle, then we won't be able to accept data from anyone we don't trust...as they could totally mess us over (like embed system calls inside the pickle file) and we wouldn't be able to easily inspect what was sent to us beforehand.

Booleans and None are not really a problem (objects including Exceptions are a problem).

Does it break how you were planning on using it? I was thinking of it as more of capturing data that could be added to unit tests (as opposed to replacing mocked devices).

>>> a = None
>>> import json
>>> json.dumps(a)
'null'
>>> b = True
>>> json.dumps(a)
'null'
>>> json.dumps(b)
'true'
>>> print json.loads(json.dumps(a))
None
>>> print json.loads(json.dumps(b))
True

dbarrosop · 2017-08-16T17:40:13Z

then we won't be able to accept data from anyone we don't trust

As discussed via other channels. I think you are right and I will look into that. The main issue is that not all calls might return structured but some "bare" object like a string or an Exception but that can be treated.

mirceaulinic raised the concern that the recorder might break things if a method changes how things are retrieved

That is very much correct so we will do two things:

Make clear in the documentation that the recorder is mostly for internal purposes and that they are free to use it but without guarantees.
We will add some metadata when recording so if we break something we know how and when it was recorded. For example, we could add the date and all the napalm libraries installed with their version at the time of the recording.

…ed and with which lib versions

coveralls · 2017-08-16T18:03:18Z

Changes Unknown when pulling 507a46e on recorder into ** on develop**.

dbarrosop · 2017-08-17T06:36:26Z

Regarding the metadata, now it adds a metadata.yaml file with the following contents:

(napalm) ➜  napalm-base git:(recorder) ✗ cat test_recorder/metadata.yaml
date: 2017-08-16 19:57:11.194889
napalm_version:
- napalm-ansible==0.7.0
- napalm-base==0.24.3
- napalm-eos==0.6.0
- napalm-fortios==0.4.0
- napalm-ios==0.7.0
- napalm-iosxr==0.5.4
- napalm-junos==0.12.0
- napalm-nxos==0.6.0
- napalm-panos==0.4.0
- napalm-pluribus==0.5.1
- napalm-ros==0.2.2
- napalm-vyos==0.1.3
- napalm==1.2.0

coveralls · 2017-08-17T11:02:48Z

Changes Unknown when pulling b8d1ccb on recorder into ** on develop**.

mirceaulinic

I have a comment regarding the dependency. Please let's discuss before pushing this further.

mirceaulinic · 2017-08-17T11:04:53Z

requirements.txt

@@ -2,3 +2,4 @@ jtextfsm
 jinja2
 netaddr
 pyYAML
+camel


If we are anyway adding another dependency for serialization, why not use something widely adopted like msgpack?

Whatever would be the extra package, I dislike very much the idea of having a dependency for a feature I know for sure I'll never use.

"MessagePack is an efficient binary serialization format. It's like JSON. but fast and small."

So a few reasons:

We don't want binary, we want something secure that humans can look at.

Camel is is a tiny library that builds on top of pyyaml, which we already have.

msgpack is huge and include even more C extensions.

dbarrosop · 2017-08-17T11:10:29Z

@ktbyers @mirceaulinic latest update replaces pickle with Camel, which builds on top of pyyaml to provide a safe mechanism to serialize/deserialize custom objects.

I tried pyyaml pickle-like behavior but it turns out is as unsafe as pickle itself and also not very reliable as it fails to understand how the constructor of the classes work.

I recommend reading this if you have the time/energy:

https://eev.ee/blog/2015/10/15/dont-use-pickle-use-camel/

If you are happy with this I will remove all the testing stuff I added and merge. Then I will quickly integrate it with the napalm cli tool so users can start recording with it.

mirceaulinic · 2017-08-17T11:16:36Z

@dbarrosop - you may have missed my review.

I disagree on using an obscure library that gained less interest than naplam (i.e. https://github.com/eevee/camel has exactly 7 followers), rather than a very well known and widely adopted msgpack (also very well know for its speed).

Also, it's very bizzare that the sole resource I was able to find about this camel is the blog post you pointed out... written by its maintainer and one of the 4 contributors.

dbarrosop · 2017-08-17T11:24:02Z

I disagree on using an obscure library that gained less interest than naplam (i.e. https://github.com/eevee/camel has exactly 7 followers)

Are we measuring now usefulness by number of followers? Doesn't make much sense IMHO.

rather than a very well known and widely adopted msgpack (also very well know for its speed

I can write myself what camel does as it's just a framework on top of pyyaml, which seems unnecessary to write as someone else did it already, but as I mentioned in the other comment, msgpack is way overkill and doesn't do exactly what we want.

dbarrosop · 2017-08-17T12:19:01Z

As discussed in slack, will rewrite without dependencies.

Added recorded and an example (to be removed)

8c9c3a0

dbarrosop mentioned this pull request Aug 7, 2017

Added integration with the recorder napalm-automation/napalm-eos#173

Open

Set wrong data in the last test

13e1e7d

dbarrosop changed the title ~~Added recorded and an example (to be removed)~~ Added recorder and an example (to be removed) Aug 7, 2017

dbarrosop mentioned this pull request Aug 8, 2017

Mock return text and increase_count_on_error option #295

Open

mirceaulinic reviewed Aug 8, 2017

View reviewed changes

mirceaulinic added this to the DISCUSSION milestone Aug 8, 2017

mirceaulinic added the enhancement label Aug 8, 2017

dbarrosop self-assigned this Aug 13, 2017

Add metadata information when recording so we know when it was record…

507a46e

…ed and with which lib versions

Use Camel instead of pickle

b8d1ccb

mirceaulinic suggested changes Aug 17, 2017

View reviewed changes

napalm-automation deleted a comment from mirceaulinic Aug 17, 2017

This was referenced Oct 14, 2017

kcelenza ideas for hackathon napalm-automation/hackathon-2017#1

Closed

Idea for Hackathon - Writing Output of Commands/Offline mode napalm-automation/hackathon-2017#4

Open

dbarrosop mentioned this pull request Nov 19, 2017

Pre decorator commit napalm-automation/napalm#540

Closed

dbarrosop closed this Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added recorder and an example (to be removed) #296

Added recorder and an example (to be removed) #296

dbarrosop commented Aug 7, 2017 •

edited

Loading

dbarrosop commented Aug 7, 2017

ubaumann commented Aug 8, 2017

ubaumann commented Aug 8, 2017 •

edited

Loading

mirceaulinic Aug 8, 2017

mirceaulinic Aug 8, 2017 •

edited

Loading

dbarrosop Aug 8, 2017

mirceaulinic Aug 8, 2017 •

edited

Loading

dbarrosop Aug 8, 2017

mirceaulinic left a comment •

edited

Loading

mirceaulinic commented Aug 8, 2017

dbarrosop commented Aug 8, 2017 •

edited

Loading

itdependsnetworks commented Aug 9, 2017

ktbyers commented Aug 10, 2017

itdependsnetworks commented Aug 10, 2017

dbarrosop commented Aug 11, 2017

ktbyers commented Aug 11, 2017 •

edited

Loading

dbarrosop commented Aug 16, 2017

coveralls commented Aug 16, 2017 •

edited

Loading

dbarrosop commented Aug 17, 2017

coveralls commented Aug 17, 2017 •

edited

Loading

mirceaulinic left a comment

mirceaulinic Aug 17, 2017

dbarrosop Aug 17, 2017

dbarrosop commented Aug 17, 2017

mirceaulinic commented Aug 17, 2017 •

edited

Loading

dbarrosop commented Aug 17, 2017

dbarrosop commented Aug 17, 2017

@@ @@ -2,3 +2,4 @@ jtextfsm @@
               jinja2
               netaddr
               pyYAML
+              camel

Added recorder and an example (to be removed) #296

Added recorder and an example (to be removed) #296

Conversation

dbarrosop commented Aug 7, 2017 • edited Loading

dbarrosop commented Aug 7, 2017

ubaumann commented Aug 8, 2017

ubaumann commented Aug 8, 2017 • edited Loading

mirceaulinic Aug 8, 2017

Choose a reason for hiding this comment

mirceaulinic Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

dbarrosop Aug 8, 2017

Choose a reason for hiding this comment

mirceaulinic Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

dbarrosop Aug 8, 2017

Choose a reason for hiding this comment

mirceaulinic left a comment • edited Loading

Choose a reason for hiding this comment

mirceaulinic commented Aug 8, 2017

dbarrosop commented Aug 8, 2017 • edited Loading

itdependsnetworks commented Aug 9, 2017

ktbyers commented Aug 10, 2017

itdependsnetworks commented Aug 10, 2017

dbarrosop commented Aug 11, 2017

ktbyers commented Aug 11, 2017 • edited Loading

dbarrosop commented Aug 16, 2017

coveralls commented Aug 16, 2017 • edited Loading

dbarrosop commented Aug 17, 2017

coveralls commented Aug 17, 2017 • edited Loading

mirceaulinic left a comment

Choose a reason for hiding this comment

mirceaulinic Aug 17, 2017

Choose a reason for hiding this comment

dbarrosop Aug 17, 2017

Choose a reason for hiding this comment

dbarrosop commented Aug 17, 2017

mirceaulinic commented Aug 17, 2017 • edited Loading

dbarrosop commented Aug 17, 2017

dbarrosop commented Aug 17, 2017

dbarrosop commented Aug 7, 2017 •

edited

Loading

ubaumann commented Aug 8, 2017 •

edited

Loading

mirceaulinic Aug 8, 2017 •

edited

Loading

mirceaulinic Aug 8, 2017 •

edited

Loading

mirceaulinic left a comment •

edited

Loading

dbarrosop commented Aug 8, 2017 •

edited

Loading

ktbyers commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 16, 2017 •

edited

Loading

coveralls commented Aug 17, 2017 •

edited

Loading

mirceaulinic commented Aug 17, 2017 •

edited

Loading