Make it so that links have their own tag pool separate from interfaces#583
Make it so that links have their own tag pool separate from interfaces#583
Conversation
That's fine @Ktmi as you progress. I'll recommend you leave it as a draft though in the meantime. When it's ready for review, and the issue discussion is ready too let me know. |
…oval of regular tags
…s' into tag_capable+premature_optimizations
6631803 to
53254ce
Compare
7aa75bf to
64d445e
Compare
|
I noticed some potential inconsistencies with locks in |
There was a problem hiding this comment.
I've just done partial review but it was meant for this other PR
I'll review this one here later |
1a4b61a to
b746863
Compare
a5d39e3 to
a83382e
Compare
| - Each ``Link`` now has a ``threading.Lock`` to perform any change or check on its attributes. | ||
| - Kytos shutdown will now wait for every NApp to shutdown completely. | ||
| - ``Links`` now have a separate tag pool from ``Interfaces``. | ||
| - ``Interfaces`` should now use the lock of their ``Switch`` to maintain consistency. |
There was a problem hiding this comment.
@Ktmi we also need to make it explicit here that's a breaking changing and NApps who used to use the related interface old tags method need to refactor as you've done for our core NApps, and then in the release notes you just mention that's expected for developers to refactor if they have other non core NApps.
Note: it's probably not worth trying to maintain deprecation compatibility due to extra effort it'd require in terms of code maintenance.
viniarck
left a comment
There was a problem hiding this comment.
Another partial review, also last comment hasn't been fully addressed
| @@ -94,9 +94,8 @@ def get_validated_tags( | |||
| def range_intersection( | |||
There was a problem hiding this comment.
Same as prior comment, unless we're aiming for other major improvements, I'm not sure if the refactor improves much readability here. It's harder to follow, and consequently maintain the code. Can you revert this part too? And use the old method and its signature?
Improvements welcome in the part in the future, but we have to have a clear goal of the main benefits, as it is, I'm not willing to take the risk.
Part of #554, still needs changes on topology, of_lldp, and mef_eline. This PR exists in its current form to make sure that I'm moving in the right direction.
Summary
This PR adds in a new class called
TAGCapable. This implements the handling of tags for any object which inherits it. As a result, all the tag handling code was removed fromInterfaceandLinkand moved to this class.InterfaceandLinknow inherit fromTAGCapble. Additionally, this PR makes a few changes to how tags should be handled:_tag_lockwas renamed totag_lock. `tag_lock is no longer implicitly held, and instead should be held by the caller, before making any change to the tags. This was done to allow multiple changes to the tags while holding the lock, rather than one at a time, as well as allowing for holding locks on multiple objects simultaneously.notify_tag_listeners. This was done to allow for not sending out tag changes, if the change needs to be cancelled.func(tag_value, tag_type='vlan')tofunc(tag_type, tag_value). The code we are writing now doesn't implicitly assumetag_type='vlan', and this follows a more natural ordering to the arguments.default_tag_rangesanddefualt_special_tags. These are used as the values thattag_rangesandspecial_tagswill be set to whenreset_tag_rangesandreset_special_tagsare called.Local Tests
Need the topology implementation finished before proper local tests can be done. But just running this in the unit tests in this, and the unit tests for other NApps, it seems to work as expected. Will need to do more testing.
End-to-End Tests
E2E results: