[Feature] Added more logs when pushing flows, force option and retries
#29
Conversation
|
Side note: we might also need to revisit later on how the |
Parametrized retry for 500
viniarck
changed the title
force option and retries
There was a problem hiding this comment.
LGTM. Thanks for the new approach introduced, @viniarck! This will helps get a more robust napp and tolerant to temporary failures.
| flow = self._build_lldp_flow(of_version) | ||
| if flow: | ||
| destination = switch.id | ||
| endpoint = f'{settings.FLOW_MANAGER_URL}/flows/{destination}' | ||
| data = {'flows': [flow]} | ||
| if event.name == 'kytos/topology.switch.enabled': | ||
| requests.post(endpoint, json=data) | ||
| res = requests.post(endpoint, json=data) |
There was a problem hiding this comment.
@viniarck another enhancement opportunity is: we should only request a flow_mod add when it is a new switch (or it was previously disabled before). The reason basically related to the fact the flow_manager should take care of reinstalling the flows once the switch reconnects. Currently, once a switch reconnects a new kytos/topology.switch.enabled event is generated (https://github.com/kytos-ng/topology/blob/master/main.py#L517-L530) and LLDP flows are requested again (even though they probably exists at the switch, if the switch only reconnected).
For the kytos/topology.switch.disabled event (the else branch of the conditional above), then it makes sense to remove the flows.
Let me know what you think and we can create an issue to handle this specific enhancement.
There was a problem hiding this comment.
@italovalcy for sure, agreed. It also minimizes the number of events. Let's go for it and map this. The only corner case that I can see would be if an operator is running flow_manager without the consistency check enabled (do you know if we'll ever has this case in prod?), and then their switch has a physical outage losing the flows installed and then reconnects, but that in practice might not ever happen if operators always have the consistency check enabled, since the pros outweigh tremendously any potential drawback (if any, unless there's a bug).
There was a problem hiding this comment.
I've captured it on issue #31, let's keep refining and evolving it there. Thanks, Italo.
Description of the change
It turns out that
of_lldpwasn't using the force option yet and retries, which facilitates for error handling if potential connections errors take place, and also I added more logs just so operators and developers can better understand any other errors that might happen.Release Notes
flow_managerforceoption as a fallback