feat(anta): Verify cvx status #930
Conversation
CodSpeed Performance ReportMerging #930 will not alter performanceComparing Summary
|
|
| class Peer(TypedDict): | ||
| """Class for holding CVX Peer information.""" | ||
|
|
||
| peer_name: str | ||
| registration_state: str |
There was a problem hiding this comment.
You probably don't need this as you have it from input models
| cluster_mode: bool | ||
| role: Literal["Master", "Standby", "Disconnected"] = "Master" | ||
| peer_status: list[CVXPeers] | ||
| CVXPeers: ClassVar[type[CVXPeers]] = CVXPeers |
There was a problem hiding this comment.
same this is not needed for new tests (we do it on refactored ones because SemVer). @carl-baillargeon to confirm
There was a problem hiding this comment.
Correct, not needed for new tests.
| cluster_mode: true | ||
| role: Master |
There was a problem hiding this comment.
we have more params here than what is present in the docstring
| role: Master | ||
| peer_status: | ||
| - peer_name : cvx-red-2 | ||
| registration_state: Registration complete |
There was a problem hiding this comment.
If I understand correctly this state is the default right? so could just be
peer_status:
- peer_name : cvx-red-2
- peer_name : cvx-red-3
?
There was a problem hiding this comment.
Sometimes, some switches can intentionally be left at a non-"registration complete" state in case the system is between upgrades. In these cases, I would like to provide an option to input that as part of test.
There was a problem hiding this comment.
you can keep the input but my point was that this input is the default so not needed.
Also according to the docstring you wrote I feel like if left on purpose in non-"registration complete" would imply failure:
* Failure: The test will fail if any of the following conditions are met:
- If the CVX Status is disabled
- If the peers are not in "Registration ok" state
Maybe it should be rewritten
- If the peers are not in the expected state
| cluster_status = command_output.get("clusterStatus") | ||
| if not (command_output.get("clusterMode") and cluster_status): | ||
| self.result.is_failure("CVX Server is not a cluster") | ||
| return False | ||
| return True |
There was a problem hiding this comment.
feels like this could be checked in _validate_cluster_status instead
| peer_cluster = cluster_status.get("peerStatus") | ||
|
|
||
| # Check if peer_cluster is None or not sized | ||
| if not peer_cluster or not isinstance(peer_cluster, dict): |
There was a problem hiding this comment.
is it possible that peer_cluster is not a dict if it is not None ?
I would probably change the logic here
# get a default dict if not present and assumes valid formatted dict otherwise
peer_cluster = cluster_status.get("peerStatus", {})
| # Check cluster role | ||
| cluster_role = cluster_status.get("role") | ||
| if cluster_role != self.inputs.role: | ||
| self.result.is_failure(f"CVX Role is not valid: {cluster_role}") | ||
| return False |
There was a problem hiding this comment.
so here you are checking the role :)
I would move the check in a different place, add in the test docstring you are doing so and
| # Check cluster role | |
| cluster_role = cluster_status.get("role") | |
| if cluster_role != self.inputs.role: | |
| self.result.is_failure(f"CVX Role is not valid: {cluster_role}") | |
| return False | |
| if (cluster_role := cluster_status.get("role")) != self.inputs.role: | |
| self.result.is_failure(f"CVX Role is not valid: {cluster_role}") | |
| return False |
| for peer in self.inputs.peer_status: | ||
| if not self._validate_individual_peer(peer, peer_cluster): | ||
| continue | ||
| return True |
There was a problem hiding this comment.
I think this is more correct
here the if..continue is useless as there is nothing else in the loop
and the last return True is possibly wrong if any peer validation was False
| for peer in self.inputs.peer_status: | |
| if not self._validate_individual_peer(peer, peer_cluster): | |
| continue | |
| return True | |
| return all(self._validate_individual_peer(peer, peer_cluster) for peer in self.inputs.peer_status) |
| Args: | ||
| peer (Peer): The peer object containing expected peer details. | ||
| peer_cluster (dict[str, Any]): The dictionary representing the peer statuses in the cluster. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool: True if the peer is valid, False otherwise. |
There was a problem hiding this comment.
| Args: | |
| peer (Peer): The peer object containing expected peer details. | |
| peer_cluster (dict[str, Any]): The dictionary representing the peer statuses in the cluster. | |
| Returns | |
| ------- | |
| bool: True if the peer is valid, False otherwise. | |
| Parameters | |
| ------------ | |
| peer | |
| The peer object containing expected peer details. | |
| peer_cluster: | |
| The dictionary representing the peer statuses in the cluster. | |
| Returns | |
| ------- | |
| bool | |
| True if the peer is valid, False otherwise. |
| role: Master | ||
| peer_status: | ||
| - peer_name : cvx-red-2 | ||
| registration_state: Registration complete |
There was a problem hiding this comment.
is the state needed? if not then lets remove it from here
Description
Verify the CVX server status and its corresponding peer registration status
Checklist:
pre-commit run)tox -e testenv)