NTP peers

[tommarcoen]

No description provided.

[ktbyers]

@t0mmetje Unit tests are failing.

[mirceaulinic]

Thanks for the addition @t0mmetje - to make sure the tests will pass, you would need to remove some details, as pointe out in my in-line comments.

[mirceaulinic]

@t0mmetje to pass the tests, please remove this and let this peer have an empty dictionary as the other ones.

[mirceaulinic]

Same here.

[coveralls]

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

[mirceaulinic]

@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]

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

[ktbyers]

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]

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]

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]

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]

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

[ktbyers]

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