search

napalm-automation / napalm-ios

Apache License 2.0
32 stars 40 forks source link

NTP peers #186

Open tommarcoen opened 6 years ago

ktbyers commented 6 years ago

@t0mmetje Unit tests are failing.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.7%) to 70.009% when pulling 1a4640c352f12020276e308e26ee866d603e4e08 on t0mmetje:ntp-peers into 83673534da1dcd89a13ae06d956bb40c63505b11 on napalm-automation:develop.

mirceaulinic commented 6 years ago

@t0mmetje we are preparing to release napalm-ios 0.8.0 and we'd like to include your additions, so I pushed the changes I suggested above to pass the tests.

@ktbyers does this look good to you?

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.7%) to 70.009% when pulling 1a4640c352f12020276e308e26ee866d603e4e08 on t0mmetje:ntp-peers into 83673534da1dcd89a13ae06d956bb40c63505b11 on napalm-automation:develop.

ktbyers commented 6 years ago

That doesn't look like the proper behavior for VRFs (looking at the test data). I would have thought since doesn't support VRFs, that we were only parsing the global/default VRF.

This is a general problem we have i.e. in contexts where VRFs are applicable, but where we are not supporting VRFs--what does NAPALM do? My assumption is we are only parsing the global/default VRFs and ignoring everything else.

mirceaulinic commented 6 years ago

I presume VRF is only necessary to determine what source IP address to use for NTP. Otherwise I can't see any other usability (i.e. I would find very strange to have a different time stamp in VRF A than in VRF B... what would be the reasoning?).

I just checked and Junos, for example, doesn't have any option like that, but directly specify what source IP address to use. IOS, on the other hand requires you to set an interface name and eventually VRF.

dbarrosop commented 6 years ago

Specifying the VRF on management protocols makes complete sense. You might have your own ntp servers that are only accessible via that network and not publicly available. Regarding junos not supporting it... I had some discussions in the past about that with them and their suggestion/workaround was... well, I’ll leave the adjective I had in mind out of here.

mirceaulinic commented 6 years ago

Yes, it certainly makes sense.

For this method specifically (and for the get_ntp_servers counterpart) it probably makes sense to only return the list of peers from the default VRF as @ktbyers said.

@t0mmetje could you please apply the necessary changes to select only the peers that use the global VRF?

mirceaulinic commented 6 years ago

Checking in here - @t0mmetje did you see the conversation above?

ktbyers commented 6 years ago

I will reimplement post-reunification and fix the VRF issue.