Inventory rework #243

dbarrosop · 2018-09-04T12:43:45Z

Needs to cleanup some bits but wanted to start with the review already :D

This PR builds on another one that needs even more cleaning but basically this PR does the following:

Separates the serializer objects from the "core" objects. So now we have a nornir.core.deserializer.inventory file with classes based on pydantic that are used to serialize/deserialize the inventory. Those objects then map into regular classes that contains all the nornir logic.
It creates proper structure to the inventory file having a proper schema. This means two things; there is clear separation between constructs and the schema can validate the entered data is correct, failing if a key is out of place, missing or misspelled.
InitNornir has been moved to nornir.init_nornir (can be imported as from nornir import InitNornir. The motivation behind the move is that as mypy starts to get enabled cyclic imports start to show up clearly suggesting code smell. For instance, in the core we were importing the connection plugins in order to register them, but then the plugins also needed to import the core as the connection superclass lives there. The solution? Well, InitNornir is meant to be initialization code that ties all the different parts together so it's clearly not part of the "core" itself. Moving it outside the core solves the cyclic imports.

You can see the schema by doing:

import json
from nornir.core.deserializer.inventory import Inventory

print(json.dumps(Inventory.schema(), indent=4))

I updated the relevant tutorials:

Also, note that the well-known attributes are now attributes as opposed to items (host.username, host.password...) while items are user-defined and free (host["my_custom_var_nornir_doesnt_care"])

coveralls · 2018-09-04T12:49:02Z

Pull Request Test Coverage Report for Build 987

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.1%) to 92.297%

Totals
Change from base Build 985:	-1.1%
Covered Lines:	1390
Relevant Lines:	1506

💛 - Coveralls

ktbyers · 2018-09-04T23:30:09Z

Note, I removed some earlier comments as I had errors in them.

dbarrosop · 2018-09-09T12:58:37Z

This one is ready for review.

ktbyers · 2018-09-09T15:44:14Z

Okay, starting to look at this. I looked at earlier version quite a bit early last week.

nornir/__init__.py

ktbyers · 2018-09-13T00:15:56Z

nornir/core/inventory.py

+        v = object.__getattribute__(self, name)
+        if v is None:
+            for g in self.groups.refs:
+                r = object.__getattribute__(self, name)


This behavior looks wrong here?

In my inventory I have a port defined as follows in groups.yaml:

nxos: port: 8443 connection_options: napalm: platform: nxos extras: timeout: 300 optional_args: port: 8443 data: foo: whatever

But 'port' won't resolve if I try to access it via a host that belongs to the nxos group. Digging into this a bunch more with Pdb.

The current variables are as follows:

(Pdb) p v None (Pdb) p g Group: nxos (Pdb) p self.groups.refs [Group: nxos] (Pdb) p self Host: nxos1 (Pdb)

Shouldn't that line 181 be:

r = object.__getattribute__(g, name)

The code for r = object.__getattribute__(self, name) is the same as the code for v = ... right above it (which we already know is None in this context).

Mmmm, so I may have changed the behavior and now you either have to override all the attributes or none. I don't have strong opinions about either way so I can revert it.

I didn't understand that last statement, it looks like a bug and not a misdesign (i.e. it looks like you intended to operate on the group (g) and not on self, but you coded self)?

Just ping me on Slack if you get a chance...probably quicker to resolve that way.

Yeah, you are right. I misinterpreted the problem you were describing, sorry for the confusion. Will take a look and fix.

ktbyers · 2018-09-13T00:21:11Z

I am just going to submit some items for review that I see (as opposed to trying to review the entire PR at once).

ktbyers · 2018-09-13T01:25:47Z

What was the main reason for changing this (i.e. for changing the groups to be a list of group names)? Was this mainly to have contains behave better or some other reasons?

(Pdb) p self.groups
['nxos']
(Pdb) p self.groups.refs
[Group: nxos]

Does refs name have some meaning?

dbarrosop · 2018-09-13T11:26:12Z

Yeah, it feels cleaner overall but it can certainly be reverted to the previous behavior. I don't recall changing that because of mypy surfacing some code smell.

ktbyers · 2018-09-29T16:56:13Z

Okay, that all makes sense and I tested the Host object behavior and the logging behavior.

dbarrosop · 2018-09-30T09:51:22Z

Let me know when you think this one is good to go

ktbyers · 2018-09-30T20:41:26Z

I think we should go ahead and merge it and move forward with this as the base for 2.0 So +1

On Sun, Sep 30, 2018 at 2:51 AM David Barroso ***@***.***> wrote: Let me know when you think this one is good to go — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA26hFKjlR9uWgbCv2P7BlGcdcaai1uRks5ugJQbgaJpZM4WY6L3> .

-- *Kirk Byers* *ktbyers@twb-tech.com <ktbyers@twb-tech.com>* *Simplify through Automation*

ktbyers · 2018-10-02T16:01:13Z

+1 ... merge away.

dbarrosop · 2018-10-03T09:01:21Z

I will probably send a PR on top of this one doing some fixes to the configuration and its documentation before mering. Otherwise it's going to be confusing for people using the 2.0 branch.

ktbyers · 2018-10-04T00:05:59Z

@dbarrosop What if we made a 2.1 branch (i.e. branch 2.1 from 2.0) and then merge this change into 2.1.

We could then notify people in the Slack channel that 2.0 will never be released and that they need to migrate their code over to 2.1 format (and that there will be a set of things that are going to break in this process).

Then all new PRs against 2.X would need to go into the 2.1 branch.

dbarrosop · 2018-10-07T11:06:19Z

We could just create a 2.0rc1 branch and deprecate 2.0 instead. I don't think there is any need to actually skip a release version. Let me work first on the mentioned PR as I mostly want to avoid changing the structure of the configuration without documenting it.

ktbyers · 2018-10-07T15:29:51Z

Sounds good.

dmfigol

Great work, David.
Just a couple of minor things I think we can improve.
Things I like the most about this PR:

separate section for host data
strict schema and this example:

from nornir.core.deserializer.inventory import InventoryElement
import json
print(json.dumps(InventoryElement.schema(), indent=4))

As a user, you can clearly see what is expected from you and it is in the tutorial!

dmfigol · 2018-10-18T17:58:22Z

docs/howto/inventory.ipynb

+       "<span class=\"lineno\"> 4 </span>        <span class=\"l l-Scalar l-Scalar-Plain\">domain</span><span class=\"p p-Indicator\">:</span> <span class=\"l l-Scalar l-Scalar-Plain\">global.local</span>\n",
+       "<span class=\"lineno\"> 5 </span>        <span class=\"l l-Scalar l-Scalar-Plain\">asn</span><span class=\"p p-Indicator\">:</span> <span class=\"l l-Scalar l-Scalar-Plain\">1</span>\n",
+       "<span class=\"lineno\"> 6 </span>\n",
+       "<span class=\"lineno\"> 7 </span><span c


This section is empty.
Update, link to the line does not seem to work correctly.
This was referring to:
"You can also grab the children of a group:"

Not sure I understand but I am going to merge nonetheless. We need a broader effort to fix docs.

dmfigol · 2018-10-18T17:59:20Z

docs/howto/inventory.ipynb

+       "<span class=\"lineno\"> 4 </span>        <span class=\"l l-Scalar l-Scalar-Plain\">domain</span><span class=\"p p-Indicator\">:</span> <span class=\"l l-Scalar l-Scalar-Plain\">global.local</span>\n",
+       "<span class=\"lineno\"> 5 </span>        <span class=\"l l-Scalar l-Scalar-Plain\">asn</span><span class=\"p p-Indicator\">:</span> <span class=\"l l-Scalar l-Scalar-Plain\">1</span>\n",
+       "<span class=\"lineno\"> 6 </span>\n",
+       "<span class=\"lineno\"> 7 </span><span c


typo complext

shouldn't this section directly lead to the advanced filtering notebook?

fixing, I hope :)

dmfigol · 2018-10-18T18:15:17Z

docs/upgrading/1_to_2.rst

+
+to::
+
+    from nornir import InitNornir


Can we add also a mention about data going to the specific section and link to an example with the new inventory?

I am going to leave this out for now. We certainly need to work on the upgrade notes and the docs before the official release.

dmfigol · 2018-10-18T18:18:30Z

nornir/core/__init__.py

        dry_run(``bool``): Whether if we are testing the changes or not
        config (:obj:`nornir.core.configuration.Config`): Configuration parameters
    """

    def __init__(
-        self, inventory, dry_run, config=None, config_file=None, logger=None, data=None
+        self, inventory, _config=None, config_file=None, logger=None, data=None


_config is weird for public API. Could you please clarify this change?

That's because config is a property and there is some magic with the filter method. However, this can be fixed in #252, I just don't want to overcomplicate this PR even more.

nornir/core/deserializer/inventory.py

dmfigol · 2018-10-19T00:12:56Z

nornir/core/state.py

+    versions of Nornir after running ``filter`` multiple times.
+
+    Attributes:
+        failed_hosts (list): Hosts that have failed to run a task properly


this is a set, but let's remove it and rely on annotation for the set

nornir/core/state.py

nornir/core/deserializer/inventory.py

Refactor Configuration

dbarrosop added 14 commits August 30, 2018 10:51

refactor configuration

e39e614

trying to make mypy happy

f9dfb40

Merge branch '2.0' of github.com:nornir-automation/nornir into pydantic

8252296

progress

5d6c31d

tests passing

333dbd3

final touches to the inventory rework

9cc059d

Data is now GlobalState

63e329f

add a fixture to reset_failed_hosts

809c864

fix defaults

e2fec9c

fix howtos

53978f1

undo GlobalState as a class object

2720a0f

progress

e310735

progress

620d446

fix tests

6105052

dbarrosop added 6 commits September 9, 2018 12:16

progress

88115da

fix mypy

4a3efbe

we need to fix doc references

dfea375

fix circular reference

7127dbb

fix cyclic import

7d8aa5e

fix sphinx

67bab3b

dbarrosop changed the title ~~[WIP] Inventory rework~~ Inventory rework Sep 9, 2018

dbarrosop mentioned this pull request Sep 12, 2018

Add support for multiple connections using the same plugin #246

Closed

ktbyers reviewed Sep 13, 2018

View reviewed changes

dbarrosop added 2 commits October 7, 2018 13:24

stop printing coverage by default

ac82aa0

separate serializing function for the configuration

4d9d839

dbarrosop added 7 commits October 13, 2018 16:11

refactor config to have everything inside a section

46bef2e

document the configuration

067b042

fix docs for exceptions

32c69eb

blackify

a639ce0

pylamafy

60f336c

fix mypy

2686173

add missing directory

b34d09c

dbarrosop mentioned this pull request Oct 14, 2018

Refactor Configuration #254

Merged

dbarrosop added 2 commits October 16, 2018 09:55

fix typos

29a7a9e

fixes #258

78fcec9

dmfigol requested changes Oct 19, 2018

View reviewed changes

dbarrosop added 5 commits October 19, 2018 10:01

change logger names __name__

7c7c85c

fix deserializer types

75820ae

classmethod should follow cls convention

147ef56

various fixes

1924649

Merge pull request #254 from nornir-automation/config_continue

900af24

Refactor Configuration

dbarrosop changed the base branch from pydantic to 2.0.0-beta October 23, 2018 16:13

addressing PR comments

37a72b1

dbarrosop merged commit e29aa1a into 2.0.0-beta Oct 23, 2018

dbarrosop deleted the inventory_rework branch October 27, 2018 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inventory rework #243

Inventory rework #243

dbarrosop commented Sep 4, 2018 •

edited

Loading

coveralls commented Sep 4, 2018 •

edited

Loading

ktbyers commented Sep 4, 2018 •

edited

Loading

dbarrosop commented Sep 9, 2018

ktbyers commented Sep 9, 2018 •

edited

Loading

ktbyers Sep 13, 2018 •

edited

Loading

dbarrosop Sep 13, 2018

ktbyers Sep 13, 2018

ktbyers Sep 13, 2018

dbarrosop Sep 13, 2018

ktbyers commented Sep 13, 2018

ktbyers commented Sep 13, 2018

dbarrosop commented Sep 13, 2018

ktbyers commented Sep 29, 2018

dbarrosop commented Sep 30, 2018

ktbyers commented Sep 30, 2018 via email

ktbyers commented Oct 2, 2018

dbarrosop commented Oct 3, 2018

ktbyers commented Oct 4, 2018

dbarrosop commented Oct 7, 2018 •

edited

Loading

ktbyers commented Oct 7, 2018

dmfigol left a comment •

edited

Loading

dmfigol Oct 18, 2018 •

edited

Loading

dbarrosop Oct 23, 2018

dmfigol Oct 18, 2018 •

edited

Loading

dbarrosop Oct 23, 2018

dmfigol Oct 18, 2018

dbarrosop Oct 23, 2018

dmfigol Oct 18, 2018

dbarrosop Oct 23, 2018

dmfigol Oct 19, 2018

dbarrosop Oct 23, 2018

Inventory rework #243

Inventory rework #243

Conversation

dbarrosop commented Sep 4, 2018 • edited Loading

coveralls commented Sep 4, 2018 • edited Loading

Pull Request Test Coverage Report for Build 987

💛 - Coveralls

ktbyers commented Sep 4, 2018 • edited Loading

dbarrosop commented Sep 9, 2018

ktbyers commented Sep 9, 2018 • edited Loading

ktbyers Sep 13, 2018 • edited Loading

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

ktbyers commented Sep 13, 2018

ktbyers commented Sep 13, 2018

dbarrosop commented Sep 13, 2018

ktbyers commented Sep 29, 2018

dbarrosop commented Sep 30, 2018

ktbyers commented Sep 30, 2018 via email

ktbyers commented Oct 2, 2018

dbarrosop commented Oct 3, 2018

ktbyers commented Oct 4, 2018

dbarrosop commented Oct 7, 2018 • edited Loading

ktbyers commented Oct 7, 2018

dmfigol left a comment • edited Loading

Choose a reason for hiding this comment

dmfigol Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol Oct 18, 2018 • edited Loading

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

dbarrosop commented Sep 4, 2018 •

edited

Loading

coveralls commented Sep 4, 2018 •

edited

Loading

ktbyers commented Sep 4, 2018 •

edited

Loading

ktbyers commented Sep 9, 2018 •

edited

Loading

ktbyers Sep 13, 2018 •

edited

Loading

dbarrosop commented Oct 7, 2018 •

edited

Loading

dmfigol left a comment •

edited

Loading

dmfigol Oct 18, 2018 •

edited

Loading

dmfigol Oct 18, 2018 •

edited

Loading