Refactor base driver

[dbarrosop]

This is something that I have been thinking for some time and now #272 gave me a reason to actually try to sell it.

The idea would be to do, instead of what we do today do, something like this:

class NetworkDriver(object):

    def open(self):
         logger.info("connecting to device with parameters blah")
         try:
              self._open()
         except Exception as e:
              logger.error("couldn't connect to device:\n{}".format(e))
              raise
   
     def _open(self):
           raise NotImplemented

class OurDriver(NetworkDriver):
      def _open(self):
           # here goes the code that we had before in the open() method

Used the logging example from #272 but this could help us implement workflows more consistently across platforms. Something like:

def a_method(self):
      try:
            r = self._do_something()
      except Exception as e:
            logger.error("Something horrible happened:\n{}".format(e))
            self._cleanup()
            raise
      if r == condition_a:
           return self._do_something_else()
      return r

Completely made up in case it wasn't obvious but the idea is that the base driver could implement the workflow, try error recoveries, etc. and the drivers could just implement small functions that do very specific things.

This depends on us agreeing to reunify all the drivers. Otherwise this is probably going to be too hard to implement, or rather it's going to be too work.

[dbarrosop]

ping @ktbyers @mirceaulinic

[ktbyers]

Yes, I don't really like either pattern.

We are starting to get a lot of indirection in our code _wrapper, _open, et cetera. Which makes reading the code/understanding the code overly hard (for sometimes minimal reductions in number of lines of code).

I wonder if there is some other pattern we could start to use more of like decorators (or some other pattern that is better).

I probably need a bigger example of your proposal (on the face of it, I am not seeing why we would want to do it). Either that or I need more coffee which is entire possible.

[mirceaulinic]

At a glance, I don't really like it either.

I'm echoing Kirk's argument from above:

We are starting to get a lot of indirection in our code _wrapper, _open, et cetera. Which makes reading the code/understanding the code overly hard (for sometimes minimal reductions in number of lines of code).

David:
I'm not sure this can be really applicable (everywhere)... meaning that logging might be very driver-specific. And I'm thinking also as a way to log not only errors but also simple information at DEBUG/INFO level.

[dbarrosop]

It's just another way of implementing the "adapter pattern". The advantage over decorators is that decorators need to be defined explicitly while this is controlled from the parent class and the adapters only have to implement the "napalm protocol/interface". It's no different from how other protocols work so you can make a class behave like a dictionary, provide a context manager, etc... but instead of implementing __getitem__ or __enter__ you have to implement _napalm_open.

meaning that logging might be very driver-specific

I think we need a minimum set of log messages common to every driver. For example, if I am running get_bgp_neighbors I want to see three common debug messages:

1. That I am in the method so I know at least i got there.
2. That I managed to retrieve the information from the device so I know at least communication from the device is fine and I can retrieve native data.
3. That data was parsed correctly.

With only these three logging messages I can easily know whose fault is if something doesn't break and this is easily implementable with something like this:

def get_bgp_neighbors(self):
     logger.debug("calling get_bgp_neighbors")
     logger.debug("gathering data for get_bgp_neighbors")
     try:
         data = self._get_bgp_neighbors_data(self)
     except Expception as e:
         logger.error("problem gathering data from the device")
         raise e

    logger.debug("parsing data for get_bgp_neighbors")
    try:
        result = self._get_bgp_neighbors_parse(self)
     except Expception as e:
         logger.error("problem parsing")
         raise e
    logger.debug("data for get_bgp_neighbors parsed correctly")
    return result

We can still provide more detailed logging if the driver requires it or if it's useful inside _get_bgp_neighbors_data and _get_bgp_neighbors_parse but with this at least we get a common framework for all the drivers.

Also note that for the getters it'd be fairly trivial to build those methods dynamically as all of them would have the same pattern.

for sometimes minimal reductions in number of lines of code

it's not about reducing code, it's about ensuring consistency and having control. If tomorrow we want to change that previous workflow and retry N times we can do it in the parent class without modifying the drivers. Or if we want to do some cleanup or recovery attempt after failure we just define the "protocol" (i.e. we need _a_recovery_method) and use it if available and skip recovery attempt if not.

I wonder if there is some other pattern we could start to use more of like decorators (or some other pattern that is better)

Check the adapter pattern. Right now we implement what other languages call interfaces which does the job but I really think the adapter pattern would be better as it would give us more control. If you check the adapter pattern, the base driver would be what the pattern calls the client.

[mirceaulinic]

I think we need a minimum set of log messages common to every driver. For example, if I am running get_bgp_neighbors I want to see three common debug messages:

Not sure a constraint like that would be a great idea.

Anyway, I see what you mean, but overall I tend to believe this aims way too far just for logging. I think it adds an unnecessary level of indirection. I was thinking about something very simple:

I will take for example a random not-very-long method from napalm-ios.

Currently implemented like:

@staticmethod
def _create_tmp_file(config):
    """Write temp file and for use with inline config and SCP."""
    tmp_dir = tempfile.gettempdir()
    rand_fname = py23_compat.text_type(uuid.uuid4())
    filename = os.path.join(tmp_dir, rand_fname)
    with open(filename, 'wt') as fobj:
        fobj.write(config)
    return filename

I would add just a couple of debug logs:

@staticmethod
def _create_tmp_file(config):
    """Write temp file and for use with inline config and SCP."""
    log.debug('Creating temporary file, under:')
    tmp_dir = tempfile.gettempdir()
    log.debug(tmp_dir)
    rand_fname = py23_compat.text_type(uuid.uuid4())
    filename = os.path.join(tmp_dir, rand_fname)
    log.debug('Temporary file absolute path:')
    log.debug(rand_fname)
    log.debug('Writing contents to the temp file')
    with open(filename, 'wt') as fobj:
        fobj.write(config)
    return filename

Maybe not that verbose, but I think you see what you mean. Looking into the commit_config method under the same driver, I identify plenty of debug logs to insert there, while under napalm-junos there's no enough reasoning.

If we were to try catching all of these under napalm-base, I would say it might be overkill & the implementation would take extremely long, while a simple PR adding the necessary logs per driver would be just straight forward.

[dbarrosop]

overall I tend to believe this aims way too far just for logging

It's not just for logging. It's about controlling the workflow and only requiring the drivers to implement smaller primitives. Primitives that do only one single thing so the flow can be controlled elsewhere. We are talking about logging now because it was the trigger of the conversation but imagine for a second we were using the get_bgp_neighbors method of my previous message and now we wanted to verify the connection is open before trying anything. It would take a single PR with 3 lines in the base class and all the drivers would immediately get the feature:

def get_bgp_neighbors(self):
     logger.debug("calling get_bgp_neighbors")
+     if not self._is_open(): # this would be the primitive that self.is_alive() implements and would return always true if not implemented
+         logger.error("connection with the device is not open")
+        raise NapalmConnectionClosed("connection with the device is not open")
     logger.debug("gathering data for get_bgp_neighbors")
     try:
         data = self._get_bgp_neighbors_data(self)
     ... (rest as my previous comment)

Voila! All drivers now verify the connection to the device is alive before actually trying anything and return a meaningful error and a consistent exception.

I will take for example a random not-very-long method from napalm-ios.

Ok, you have solved 0.1% of the problem. What about the other 99.9%? How do we make sense of the logs if every driver implements logs wherever they want and with their own format?

I am fine scrapping this idea but I want to make sure the idea and patterns are understood and this is just not scrapped because people didn't really understand it.