Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Added more logs when pushing flows, force option and retries #29

Merged
merged 9 commits into from
Dec 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ Security
========


[1.2.0] - 2021-12-13
********************
Changed
=======
- Added support for retries when sending a request to ``flow_manager``
- Parametrized ``force`` option as a fallback
- Added more logs for request errors


[1.1.1] - 2021-04-22
********************
Changed
Expand Down
2 changes: 1 addition & 1 deletion kytos.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"username": "kytos",
"name": "of_lldp",
"description": "Discover network-to-network interfaces (NNIs) using the LLDP protocol.",
"version": "1.1.1",
"version": "1.2.0",
"napp_dependencies": ["kytos/of_core", "kytos/flow_manager", "kytos/topology"],
"license": "MIT",
"url": "https://github.com/kytos/of_lldp.git",
Expand Down
44 changes: 42 additions & 2 deletions main.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""NApp responsible to discover new switches and hosts."""
import struct
import time

import requests
from flask import jsonify, request
Expand Down Expand Up @@ -115,6 +116,15 @@ def handle_lldp_flows(self, event):
event (:class:`~kytos.core.events.KytosEvent`):
Event with new switch information.

"""
self._handle_lldp_flows(event)

def _handle_lldp_flows(self, event):
"""Install or remove flows in a switch.

Install a flow to send LLDP packets to the controller. The proactive
flow is installed whenever a switch is enabled. If the switch is
disabled the flow is removed.
"""
try:
dpid = event.content['dpid']
Expand All @@ -124,15 +134,45 @@ def handle_lldp_flows(self, event):
except AttributeError:
of_version = None

def _retry_if_status_code(response, endpoint, data, status_codes,
viniarck marked this conversation as resolved.
Show resolved Hide resolved
retries=3, wait=2):
"""Retry if the response is in the status_codes."""
if response.status_code not in status_codes:
return
if retries - 1 <= 0:
return
data = dict(data)
data["force"] = True
res = requests.post(endpoint, json=data)
method = res.request.method
if res.status_code != 202:
log.error(f"Failed to retry on {endpoint}, error: {res.text},"
f" status: {res.status_code}, method: {method},"
f" data: {data}")
time.sleep(wait)
return _retry_if_status_code(response, endpoint, data,
status_codes, retries - 1, wait)
log.info(f"Successfully forced {method} flows to {endpoint}")

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@viniarck viniarck Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've captured it on issue #31, let's keep refining and evolving it there. Thanks, Italo.

if res.status_code != 202:
log.error(f"Failed to push flows on {destination},"
f" error: {res.text}, status: {res.status_code},"
f" data: {data}")
_retry_if_status_code(res, endpoint, data, [424, 500])
viniarck marked this conversation as resolved.
Show resolved Hide resolved
else:
requests.delete(endpoint, json=data)
res = requests.delete(endpoint, json=data)
if res.status_code != 202:
log.error(f"Failed to delete flows on {destination},"
f" error: {res.text}, status: {res.status_code}",
f" data: {data}")
_retry_if_status_code(res, endpoint, data, [424, 500])

@listen_to('kytos/of_core.v0x0[14].messages.in.ofpt_packet_in')
def notify_uplink_detected(self, event):
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ exclude = .eggs,ENV,build,docs/conf.py,venv

[yala]
radon mi args = --min C
pylint args = --disable=too-many-locals,too-few-public-methods,too-many-instance-attributes --ignored-modules=napps.kytos.of_lldp
pylint args = --disable=too-many-locals,too-few-public-methods,too-many-instance-attributes,too-many-arguments,inconsistent-return-statements --ignored-modules=napps.kytos.of_lldp

[pydocstyle]
add-ignore = D105
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
BASE_ENV = Path(os.environ.get('VIRTUAL_ENV', '/'))

NAPP_NAME = 'of_lldp'
NAPP_VERSION = '1.1.1'
NAPP_VERSION = '1.2.0'

# Kytos var folder
VAR_PATH = BASE_ENV / 'var' / 'lib' / 'kytos'
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,24 @@ def test_handle_lldp_flows(self, mock_post, mock_delete):
self.napp.handle_lldp_flows(event_del)
mock_delete.assert_called()

@patch("time.sleep")
@patch("requests.post")
def test_handle_lldp_flows_retries(self, mock_post, _):
"""Test handle_lldp_flow method retries."""
dpid = "00:00:00:00:00:00:00:01"
switch = get_switch_mock("00:00:00:00:00:00:00:01", 0x04)
self.napp.controller.switches = {dpid: switch}
event_post = get_kytos_event_mock(name="kytos/topology.switch.enabled",
content={"dpid": dpid})

mock = MagicMock()
mock.request.method = "POST"
mock.status_code = 500
mock.text = "some_err"
mock_post.return_value = mock
self.napp._handle_lldp_flows(event_post)
self.assertTrue(mock_post.call_count, 3)

@patch('kytos.core.buffers.KytosEventBuffer.put')
@patch('napps.kytos.of_lldp.main.KytosEvent')
@patch('kytos.core.controller.Controller.get_switch_by_dpid')
Expand Down