Conversation
| self.handle_interface_link_up(interface, event) | ||
|
|
||
| @listen_to('.*.topology.switch.interface.created') | ||
| @listen_to('.*.switch.interface.created') |
There was a problem hiding this comment.
@Alopalao can you please double check possible overload when multiple switches connect at the same time and each switch having 30 or so interfaces. Wont this overload the database?
There was a problem hiding this comment.
You are right this could be improve. I will be looking to implement some sort of pacer so only the last switch upsert call is executed.
There was a problem hiding this comment.
Indeed only subscribing to .*.switch.interfaces.created was meant to minimize dozens of extra DB writes. So, the key to simplify for us here while not increasing too much extra DB writes is to only publish 'kytos/of_core.switch.interface.created' on of_core on port status OFPPR_ADD after this statement, and then on topology we also handle here as @Alopalao implemented.
Can you double check this approach @Alopalao? Good catch @italovalcy
There was a problem hiding this comment.
Created a draft PR with that approach. I have tested it and it seems to be working fine. I am going to wait for the end-to-end results.
There was a problem hiding this comment.
@Alopalao, great. Just realized that update_port_status(port_status, soruce) was already sending kytos/of_core.switch.interface.created when handling OFPPR_ADD. Check out if we also need to augment an unit explicit unit test to be extra safe too. Other than that, it's looking great to me if you also locally tested adding an interface after a switch handshake
There was a problem hiding this comment.
I tested locally with changes in of_core and found no issues.
| created event again and it can be belong to a link. | ||
| """ | ||
| interface = event.content['interface'] | ||
| if "of_core" in event.name: |
There was a problem hiding this comment.
@Alopalao, can you refactor this if to compare the event name completely? "kytos/of_core.switch.interface.created", that will improve readability and easier for us to remember/understand why we did it without having to navigate in a lot of git history. Plus a comment in the docstring explaining it's for handling interface creation from a OFPPR_ADD that'd be great too. Wdyt?
| - ``.*.switch.(new|reconnected)`` | ||
| - ``.*.connection.lost`` | ||
| - ``.*.switch.interface.created`` | ||
| - ``.*.switch.interfaces.created`` |
There was a problem hiding this comment.
Good catch. Thanks for improving this event subscription doc.
| created event again and it can be belong to a link. | ||
| """ | ||
| interface = event.content['interface'] | ||
| if "of_core" in event.name: |
There was a problem hiding this comment.
This if and its statements are susceptible to events out of order if OFPPR_ADD from more than one ports get sent shortly after and get preempted, it won't result in any major issue, but it will waste extra DB writes reinserting the same switch, but since this is expected to be only a few extra writes if any in most cases, and in production usage a switch won't keep creating interfaces constantly than it's acceptable as it is.
Plus, please, map a new issue on 2026.1 to improve this and assign yourself, including when handling '.*.switch.interfaces.created it needs a switch lock and keeping track of when it was last inserted at, and if out of order you skip the write. But let's only tackle this minor enhancement after @Ktmi changes since he's refactoring some locks, so this will minimize conflicts. OK?
Closes #290
Summary
Fixed updating switch interfaces in the database when an interface is added while Kytos is running.
Only added updated after
OFPPR_ADDwhich comes only fromof_coreLocal Tests
Created new interface on Mininet with this commands:
End-to-End Tests
This PR has a new related test and the results when running.