feat(anta): Added the test case to verify NTP associations functionality #757
Conversation
vitthalmagadum
changed the title
anta/tests/system.py
Outdated
| @AntaTest.anta_test | ||
| def test(self) -> None: | ||
| """Main test function for VerifyNTPAssociations.""" | ||
| failures: dict[str, Any] = {"ntp_servers": {}} |
There was a problem hiding this comment.
this does not need to be a dict with a top level key ntp_servers, the key does not bring any value. you can make it a list of dict
anta/tests/system.py
Outdated
| for ntp_server in self.inputs.ntp_servers: | ||
| address = str(ntp_server.server_address) | ||
| preferred = ntp_server.preferred | ||
| failure: dict[str, dict[str, dict[str, Any]]] = {"ntp_servers": {address: {}}} |
There was a problem hiding this comment.
why build a nested structure on every iteration?
anta/tests/system.py
Outdated
| failure["ntp_servers"][address] = {"status": "Not configured"} | ||
| failures = deep_update(failures, failure) |
There was a problem hiding this comment.
once failures is a dict, you can simply do this
| failure["ntp_servers"][address] = {"status": "Not configured"} | |
| failures = deep_update(failures, failure) | |
| failures[address] = {"status": "Not configured"}}) |
Though it feels weird we have status vs condition would be better to have a single way of rendering things to be easier to manipulate
anta/tests/system.py
Outdated
| condition = get_value(peer_detail, "condition") | ||
| if not preferred and condition != "candidate": | ||
| failure["ntp_servers"][address] = {"condition": "Not candidate"} | ||
| failures = deep_update(failures, failure) |
There was a problem hiding this comment.
please try to avoid to use deep_update, no need to unnecessarily nest structures here
anta/tests/system.py
Outdated
| class NTPServer(BaseModel): | ||
| """Model for a NTP server.""" | ||
|
|
||
| server_address: IPv4Address |
There was a problem hiding this comment.
If a user configuring the pool then they wont aware about the ip address. So instant of checking the condition on basis of peer address can we check the peers.
leaf1-dc1(config)#show ntp associations | json
{
"peers": {
"192.168.66.21": {
"condition": "reject",
"peerIpAddr": "192.168.66.21",
"refid": ".INIT.",
"stratumLevel": 16,
"peerType": "unicast",
"lastReceived": 1721650565.0,
"pollInterval": 1024,
"reachabilityHistory": [
false
],
"delay": 0.0,
"offset": 0.0,
"jitter": 0.0
},
"ntp3.aristanetworks.com": {
"condition": "sys.peer",
"peerIpAddr": "10.41.194.20",
"refid": "17.253.16.125",
"stratumLevel": 2,
"peerType": "unicast",
},
}
In that case server address can be str or IPv4Address.
@carl-baillargeon @gmuloc
There was a problem hiding this comment.
I agree that we can have a model that looks like either
class NTPServer(BaseModel):
server_address: IPv4Address | None
server_name: str | None
preferred: bool = False
and we use whichever is available between address and name (or both to check the peer IP)
or
class NTPServer(BaseModel):
server: str | IPv4Address
preferred: bool = False
and we are more limited in what we check but we can move forward
An additional problem is that even the server address can be wrong as we have seen in the example you shared (e.g. - you add a generic server name ntp.aristanetworks.com and a more specific one appears in your device output ntp3.aristanetworks.com)
And this we cannot really handle in ANTA I think (because it all depends on DNS resolution) - probably this caveat should be documented in the test. @vitthalmagadum can you add some note stating the same?
vitthalmagadum
force-pushed
the
issue_753
branch
2 times, most recently
from
89e775a to
64979b4
Compare
anta/tests/system.py
Outdated
|
|
||
| # Check if NTP server details exists. | ||
| if (peer_detail := get_value(peer_details, server_address, separator="..")) is None: | ||
| failures += f"\nNTP peer {server_address} is not configured." |
There was a problem hiding this comment.
I don't like having unnecessary carriage returns in the result messages. When there is only 1 message:
There was a problem hiding this comment.
Removed the unnecessary \n at start. Thanks!!
| "expected": { | ||
| "result": "failure", | ||
| "messages": [ | ||
| "\nFor NTP peer 1.1.1.1:\nExpected `sys.peer` as the condition, but found `candidate` instead.\n" |
There was a problem hiding this comment.
Here there will be an empty line at the beginning of the message which is unnecessary IMO.
There was a problem hiding this comment.
Same here as well.
| }, | ||
| "expected": { | ||
| "result": "failure", | ||
| "messages": ["NTP peers are not configured."], |
vitthalmagadum
force-pushed
the
issue_753
branch
from
64979b4 to
a8ee02c
Compare
anta/tests/system.py
Outdated
| class NTPServer(BaseModel): | ||
| """Model for a NTP server.""" | ||
|
|
||
| server_address: str | IPv4Address |
There was a problem hiding this comment.
| server_address: str | IPv4Address | |
| server_address: Hostname | IPv4Address |
Import from custom_types.
vitthalmagadum
force-pushed
the
issue_753
branch
from
a8ee02c to
dc54e62
Compare
|
Description
NTP associations for all NTP servers should be up as denoted by the "condition" field below, and the primary NTP server should be preferred.
primary server will have condition "sys.peer", and pri server is denoted using "preferred: true "
other servers will have condition "candidate"
Fixes #753
Checklist:
pre-commit run)tox -e testenv)