kytos-ng / of_core

Kytos Main OpenFlow Network Application (NApp)
MIT License
0 stars 5 forks source link

[Fix] Handler kytos/core.openflow.raw.in crashed when trying to log an OFPT_ERROR #62

Closed viniarck closed 2 years ago

viniarck commented 2 years ago

Fixes #61

Now it correctly logs and then publishes the OFPT_ERROR message, I removed the hex f-string format, it was there primarily for convenience.

This branch is on top of #60 to facilitate local development and managing in-flight branches.

Release notes

Running it locally

2022-05-19 16:01:02,889 - ERROR [kytos.napps.kytos/of_core] (thread_pool_sb_7) OFPT_ERROR: type ErrorType.OFPET_BAD_MATCH, error code 7, from switch 00:00:00:00:00:00:00:01, xid 130177
2539
2022-05-19 16:01:02,894 - WARNING [kytos.napps.kytos/flow_manager] (thread_pool_sb_7) Deleting flow: {'switch': '00:00:00:00:00:00:00:01', 'table_id': 0, 'match': {'dl_vlan': 10000}, '
priority': 32768, 'idle_timeout': 0, 'hard_timeout': 0, 'cookie': 0, 'id': '502866b8912c0eee9e98345b01013524', 'stats': {}, 'cookie_mask': 0, 'instructions': [{'instruction_type': 'app
ly_actions', 'actions': [{'port': 2, 'action_type': 'output'}]}]}, xid: 1301772539, cookie: 0, error: {'error_command': 'add', 'error_type': UBInt16(<ErrorType.OFPET_BAD_MATCH: 4>), 'e
rror_code': <BadMatchCode.OFPBMC_BAD_VALUE: 7>}
kytos $>
viniarck commented 2 years ago

Very nice that you get this error! I've commented on the issue that this hex conversion indeed was done for convenience in the past when we have to troubleshoot log entries from physical switches and Kytos log. Relying on the external transformations on the log message although possible would speed down the tshoot process, and that is why we request the hex version of the XID (IMHO, we don't actually need the int format, only hex format would be enough).

With that in mind, I made a comment/suggestion with another strategy to fix the problem.

Got it. I've just applied the commit, I think we can consider implementing the dunder __format__ method in the future, this patch solves for now. Thanks @italovalcy, I'm glad you recalled about this original requirement.