Closed lcphill closed 1 year ago
Good catch. It is the result of some refactoring, where this corner case was moved to: https://github.com/aristanetworks/ansible-avd/blob/devel/ansible_collections/arista/avd/roles/eos_designs/python_modules/network_services/utils.py#L184.
The lines 273:274 can simply be removed.
Have you seen any functional issues caused by this?
Thanks Claus
Hi, I have ported our inventory from 3.8.X to 4.3.0 and followed the migration guide. I have run eos_designs, but not all the way to the point of deployment through eos_cli_config_gen. In that scenario, I have not observed any issue or side-effect regarding this logic in router_bgp().
Ack on your suggested action (remove lines 273 and 274). That seems correct, but I figured I would throw this over to you, just in case.
Thank you.
Do you want to close this ticket? (and make a separate ticket for a code change), or do you want to keep this ticket open as a basis for making a code change? [ I am ok, either way ]
This ticket is fine. We are not so strict about such things :) Would you be willing to submit a PR to fix this or should I?
Hi, I would prefer if you were to take on the actual code change (I am not a committer, although this has been discussed [ we have a mutual customer ] )
Issue Summary
Hi, This issue is based on code-introspection, concerning a non-executable code path.
See here: https://github.com/aristanetworks/ansible-avd/blob/v4.3.0/ansible_collections/arista/avd/roles/eos_designs/python_modules/mlag/avdstructuredconfig.py#L273
Concerning two methods, namely: router_bgp() and underlay_bgp()
Line 265 (first "if" conditional): if not (self.shared_utils.mlag_l3 is True and self.shared_utils.underlay_bgp)
The method self.shared_utils.underlay_bgp is a compound conditional, as follows: self.bgp AND self.underlay_routing_protocol == "ebgp" AND self.underlay_router AND self.uplink_type == "p2p"
Then, on line 273, there is a second "IF" statement, being: if not self.shared_utils.underlay_bgp
I believe the second conditional will never succeed into the inner return statement on line 274.
There are two intervening lines (line 269 and 270) as follows: peer_group_name = self.shared_utils.bgp_peer_groups["mlag_ipv4_underlay_peer"]["name"] router_bgp = self._router_bgp_mlag_peer_group()
Unless these two statements were to modify underlay_bgp (that is, the compound conditions therein)?, which I would consider to be a side-effect. I do not believe this to be the case (I would consider that as an unwanted side effect?).
Please advise or confirm. Thank you.
Which component(s) of AVD impacted
eos_designs
How do you run AVD ?
None
Steps to reproduce
Relevant log output
Contributing Guide