New junos messages for bgp, ospf, and configuration #168

lampwins · 2017-09-29T18:35:30Z

This covers bgp neighbor state changes, ospf state changes, and configuration commit and rollback life cycle.

coveralls · 2017-09-29T18:47:25Z

Coverage decreased (-0.2%) to 51.072% when pulling 2a3d0fc on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

mirceaulinic

That's awesome @lampwins!

mirceaulinic · 2017-09-29T19:15:30Z

napalm_logs/config/junos/ESWD_BPDU_BLOCK_ERROR_DISABLED.yml

+messages:
+  # 'error' should be unique and vendor agnostic. Currently we are using the JUNOS syslog message name as the canonical name.
+  # This may change if we are able to find a more well defined naming system.
+  - error: BPDU_BLOCK_INTERFACE_DISSABLED


Small typo here: BPDU_BLOCK_INTERFACE_DISABLED

mirceaulinic · 2017-09-29T19:30:50Z

tests/config/junos/BGP_NEIGHBOR_STATE_CHANGED/active_to_connect/syslog.msg

@@ -0,0 +1 @@
+<149>Jun 21 14:03:12  vmx01 rpd[2902]: RPD_BGP_NEIGHBOR_STATE_CHANGED: BGP peer 1.1.1.1 (External AS 2222) changed state from Active to Connect (event AnEvent) (instance master)


It would be good if you could check the actual value the device sends for this PRI: this will be reflected in the expected output from yang.json. Now, the documentation is auto-generated from these mock files, as examples, e.g., http://napalm-logs.readthedocs.io/en/latest/messages/NTP_SERVER_UNREACHABLE.html, so it would be good to have the facility and severity in the examples as close as possible to the real values, so the users know what to expect.

mirceaulinic · 2017-09-29T19:31:45Z

napalm_logs/config/junos/ESWD_BPDU_BLOCK_ERROR_DISABLED.yml

@@ -0,0 +1,15 @@
+# This error tag corresponds to syslog messages notifying that the configured
+# interface has been dissabled due to a bpdu block


disabled here again

mirceaulinic · 2017-09-29T19:33:39Z

napalm_logs/config/junos/ESWD_BPDU_BLOCK_ERROR_DISABLED.yml

@@ -0,0 +1,15 @@
+# This error tag corresponds to syslog messages notifying that the configured


In general, the filename of the profile is the value of the error, so it's easier to check where is a feature defined; however, that's not mandatory.

mirceaulinic · 2017-09-29T19:38:14Z

Solid work here @lampwins, thank you! Besides my suggestions above, I'd recommend you to double check your git settings and amend the commit author (the email address in particular), so it will link to your github profile.

lampwins · 2017-09-29T20:48:02Z

I have changed the filenames in the profile and fixed the PRI numbers.

coveralls · 2017-09-29T20:57:23Z

Coverage decreased (-0.2%) to 51.072% when pulling 560090a on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

mirceaulinic

Few more minor changes please, and some food for thoughts :-)

mirceaulinic · 2017-09-29T21:34:46Z

napalm_logs/config/junos/OSPF_NEIGHBOR_DOWN.yml

+      area: (\d+\.\d+\.\d+\.\d+)
+      reason_message: ([\w ]+)
+    line: 'OSPF neighbor {neighbor} (realm ospf-v2 {interface} area {area}) state changed from Full to Down due to KillNbr (event reason: {reason_message})'
+    model: openconfig-network-instance


I missed this previously: I think we should reference the openconfig-opsf, as it's a standalone model (although it's just based on the network instance one). Thoughts?

Makes sense, except for why openconfig does this :)

mirceaulinic · 2017-09-29T21:35:53Z

tests/config/junos/BGP_NEIGHBOR_STATE_CHANGED/active_to_connect/yang.json

+          "1.1.1.1": {
+            "state": {
+              "peer_as": "2222",
+              "session-state-change-event": "AnEvent",


Is this a genuine event message (AnEvent)? If not, could you please reference a real example (same documentation reason).

Again, just me being lazy, will fix. Also this relates to my comments on the ospf events.

mirceaulinic · 2017-09-29T21:38:33Z

tests/config/junos/CONFIGURATION_ERROR/default/yang.json

@@ -0,0 +1,33 @@
+{


We might revisit this structure later (I can't think of a better structure currently though), but I fear it won't be compatible with other platforms. That's just a FYI.

I agree this will not last in the long run. Ideally I think it should map to actual openconfig models depending on the error. I can't even begin to imagine what that would look like in a mapping definition though (outside of some complex python version).

Discussion to follow under #175

mirceaulinic · 2017-09-29T21:40:55Z

tests/config/junos/OSPF_NEIGHBOR_DOWN/inactive_timer/yang.json

+                                "neighbor": {
+                                  "1.1.1.1": {
+                                    "state": {
+                                      "adjacency-state-change-reason-message": "dead timer",


I'm wondering how diverse is this message and if we could define a typeref as with the others, e.g., DEAD_TIMER (so it will be standard across other platforms). We can leave this as an open discussion for later, but it would be nice if you could build up a list of the messages you've seen.

This was just me being lazy with the test and I should have fixed it in the last commit. The reason message is tough is because there are many different messages that relate to the INACTIVE_TIMER event type. For example, whether of not BFD is in play.

This is the same problem in the BGP state changes. As you can see there are already a number mapping required to cover all state transitions in the BGP state machine. IMO it would be very unwieldy to then throw in all the human event messages because there are cases where several messages map to one state change.

I think it would be more feasible down the road if we could define these messages separate from the log mappings. These would allow us to map several different vender messages for an event to some model value while not having to keep up with tens/hundreds of log mappings for a single log type.

Good points. My opinion is that we should try to map them as much as possible (if they are not completely random and there's a decent amount of mappings needed).

I think it would be more feasible down the road if we could define these messages separate from the log mappings.

Good suggestion - after this is merged, I will open an issues so we can discuss further and see in time what's the best way to approach this, after we'll have more examples and feedback from the users / developers.

Let's discuss this further and build up a list of possible messages: #176

coveralls · 2017-09-30T03:03:59Z

Coverage decreased (-0.2%) to 51.072% when pulling ad7a3da on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

coveralls · 2017-09-30T03:03:59Z

Coverage decreased (-0.2%) to 51.072% when pulling ad7a3da on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

lampwins · 2017-09-30T03:11:31Z

Any idea what's up with the py2.7 travis build? Just curious as I have also been having to exclude it locally.

mirceaulinic

This looks good to me.
Would you have any feedback @loverend?

mirceaulinic · 2017-10-02T16:37:42Z

@lampwins I don't know either why's py27 failing - I will have a closer somewhere next week, certainly worth investigating. I suspect there was a recent change in one of the underlying dependencies; tox sometimes behaves very weird too, so there can be several possibilities.
But for sure it is not related at all to your changes here (checked locally).

luke-orden

This is really great, just a couple of suggestions.

luke-orden · 2017-10-03T08:17:03Z

napalm_logs/config/junos/OSPF_NEIGHBOR_DOWN.yml

+      neighbor: ([\w\d:\.]+)
+      interface: ([\w\-\/\.\d]+)
+      area: (\d+\.\d+\.\d+\.\d+)
+      reason_message: ([\w ]+)


You should not use special characters in the key name as we escape all special characters. This will cause a key error when parsing.

@loverend Could you elaborate on this? Do you mean in the regex's or reason_message itself?

I mean reason_message, it should be reasonMessage

luke-orden · 2017-10-03T08:46:18Z

napalm_logs/config/junos/BGP_NEIGHBOR_STATE_CHANGED.yml

+        bgp//neighbors//neighbor//{peer}//state//peer_as: asn
+        bgp//neighbors//neighbor//{peer}//state//session-state-change-event: event
+      static:
+        bgp//neighbors//neighbor//{peer}//state//session-state: IDLE


The repeat entries can be condensed into one entry by matching the old and new states and then converting the values into the appropriate openconfig names. You can do this by casting to a function. See https://github.com/napalm-automation/napalm-logs/blob/master/napalm_logs/config/junos/SYSTEM_ALARM.yml#L13 for an example.

In this case you could add a function to convert any changes like OpenSent to OPEN_SENT and others you could just return as uppercase, i.e:

def bgp_state_convert(state): state_dict = {'OpenSent': 'OPEN_SENT', 'OpenConfirm': 'OPEN_CONFIRM'} return state_dict.get(state, state.upper())

I think this is better than my current approach but IMO this is going to get unwieldy fast once you start to throw other vendors. For example, Juniper calls it OpenSent while cisco calls it OPEN SENT. Obviously these mappings have to be defined somewhere but I don't think hiding them in a function inside the utils package is the best idea in the long run.

Like I said, this is better than what I have for now, but I think it is worth further discussion, along with the vendor event message mappings.

We should probably have a functionality directly into the YAML profiles and help with this kind of 1:1 mappings, e.g.,

mapping: replace: state: OpenSent: OPEN_SENT OpenConfirm: OPEN_CONFIRM variables: bgp//neighbors//neighbor//{peer}//state//peer_as: asn bgp//neighbors//neighbor//{peer}//state//session-state-change-event: event bgp//neighbors//neighbor//{peer}//state//session-state: state

This way we also avoid any function utility, but the mapping is specific to a particular platform only.
Any thoughts on this @loverend @lampwins?

Anyway, for the moment I would say to get this PR in, and investigate this sort of optimisations later (before they'll grow more), but it's very good to have this discussion right now.

@mirceaulinic I think this is headed in the right direction. Ultimately will take some playing around with but I like this.

Or maybe even something like:

preprocess: state: - replace: OpenSent: OPEN_SENT OpenConfirm: OPEN_CONFIRM - upper - lower - upper mapping: variables: bgp//neighbors//neighbor//{peer}//state//peer_as: asn bgp//neighbors//neighbor//{peer}//state//session-state-change-event: event bgp//neighbors//neighbor//{peer}//state//session-state: state

Just in case we'd even need more extensive preprocessing to some fields (that's maybe just not the best example, but I think you see what I mean: to a certain field you can apply several consecutive operations).

coveralls · 2017-10-03T14:32:29Z

Coverage decreased (-0.03%) to 51.207% when pulling 89764db on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

coveralls · 2017-10-03T14:32:29Z

Coverage decreased (-0.03%) to 51.207% when pulling 89764db on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

coveralls · 2017-10-03T14:46:15Z

Coverage increased (+0.08%) to 51.317% when pulling 3d119e4 on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

coveralls · 2017-10-03T14:46:15Z

Coverage increased (+0.08%) to 51.317% when pulling 3d119e4 on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

coveralls · 2017-10-03T14:46:15Z

Coverage increased (+0.08%) to 51.317% when pulling 3d119e4 on lampwins:feature/new-junos-logs into 8967650 on napalm-automation:master.

mirceaulinic requested review from luke-orden and mirceaulinic September 29, 2017 19:19

mirceaulinic added enhancement profile labels Sep 29, 2017

mirceaulinic added this to the 0.2.0 milestone Sep 29, 2017

mirceaulinic requested changes Sep 29, 2017

View reviewed changes

mirceaulinic reviewed Sep 29, 2017

View reviewed changes

John Anderson and others added 2 commits September 29, 2017 16:44

new junos messages for bgp, ospf, and configuration

d3625ea

fixed PRI numbers and added new rollback case

560090a

lampwins force-pushed the feature/new-junos-logs branch from 2a3d0fc to 560090a Compare September 29, 2017 20:46

mirceaulinic requested changes Sep 29, 2017

View reviewed changes

added better test cases for ospf and bgp state change events

ad7a3da

mirceaulinic approved these changes Oct 2, 2017

View reviewed changes

luke-orden requested changes Oct 3, 2017

View reviewed changes

fixed key name in ospf and added bgp state mapping function

89764db

added tests for bgp_state_convert_function

3d119e4

mirceaulinic mentioned this pull request Oct 4, 2017

Version 0.2.0 #170

Merged

mirceaulinic merged commit 5d6740f into napalm-automation:master Oct 4, 2017

mirceaulinic mentioned this pull request Oct 10, 2017

Revisit the structure of the CONFIGURATION_ERROR #175

Open

		@@ -0,0 +1 @@
		<149>Jun 21 14:03:12 vmx01 rpd[2902]: RPD_BGP_NEIGHBOR_STATE_CHANGED: BGP peer 1.1.1.1 (External AS 2222) changed state from Active to Connect (event AnEvent) (instance master)

		@@ -0,0 +1,15 @@
		# This error tag corresponds to syslog messages notifying that the configured
		# interface has been dissabled due to a bpdu block

New junos messages for bgp, ospf, and configuration #168

New junos messages for bgp, ospf, and configuration #168

Conversation

lampwins commented Sep 29, 2017

coveralls commented Sep 29, 2017 • edited Loading

mirceaulinic 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

mirceaulinic commented Sep 29, 2017

lampwins commented Sep 29, 2017

coveralls commented Sep 29, 2017 • edited Loading

mirceaulinic 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

coveralls commented Sep 30, 2017

coveralls commented Sep 30, 2017

lampwins commented Sep 30, 2017

mirceaulinic left a comment

Choose a reason for hiding this comment

mirceaulinic commented Oct 2, 2017

luke-orden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lampwins Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-orden Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirceaulinic Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 3, 2017

coveralls commented Oct 3, 2017 • edited Loading

coveralls commented Oct 3, 2017

coveralls commented Oct 3, 2017

coveralls commented Oct 3, 2017

coveralls commented Sep 29, 2017 •

edited

Loading

coveralls commented Sep 29, 2017 •

edited

Loading

lampwins Oct 3, 2017 •

edited

Loading

luke-orden Oct 3, 2017 •

edited

Loading

mirceaulinic Oct 3, 2017 •

edited

Loading

coveralls commented Oct 3, 2017 •

edited

Loading