Pull conn status from oxp - #203
Conversation
Pull Request Test Coverage Report for Build 22147447197Details
💛 - Coveralls |
italovalcy
left a comment
There was a problem hiding this comment.
Hi @congwang09 very nice PR you sent! I left a few comments regarding the code structure.
| "asgiref >= 3.7.2", | ||
| "pymongo > 3.0", | ||
| "sdx-datamodel @ git+https://github.com/atlanticwave-sdx/datamodel@v3.2.1", | ||
| "sdx-datamodel @ git+https://github.com/atlanticwave-sdx/datamodel@v3.2.0", |
There was a problem hiding this comment.
Hi Cong, can you please confirm this change? like using a older version?
There was a problem hiding this comment.
Somehow datamodel's most recent tag is 3.2.0 (https://github.com/atlanticwave-sdx/datamodel/tags). I'll check with Yufeng to make sure tags are consistent.
| OXP_TOPOLOGY_URL=http://192.168.201.205:8088/SDX-LC/1.0.0/topology | ||
| OXP_PULL_TOPOLOGY_INTERVAL=180 | ||
| OXP_CONNECTION_URL=http://192.168.201.205:8088/SDX-LC/1.0.0/connection | ||
| OXP_LIST_CONNECTIONS_URL=http://192.168.201.205:8088/SDX-LC/1.0.0/connections |
There was a problem hiding this comment.
according to the L2VPN provisioning API, the same endpoint used for creating L2VPNs is used to list all L2VPNs. Thus, I believe we could have the same env var here.
There was a problem hiding this comment.
Correct me if I was wrong, the endpoint for creating l2vpn is "1.0.0/connection", while the one list all connections is "1.0.0/connections"
There was a problem hiding this comment.
that was the earlier endpoint, but when we finished the spec for L2VPN provisioning API, we refactor the Kytos SDX Napp to support the new endpoints, which are both (for creating and listing) /l2vpn/1.0
|
|
||
| for service_id, connection in connections_json.items(): | ||
| # Fetch existing connection from DB | ||
| existing_connection = db_instance.get_value_by_key(service_id) |
There was a problem hiding this comment.
Cong, another doubt arrived while reviewing the SDX-COntroller side: can you please double check if we really want to maintain a database entry in the SDX-LC? It will make things a bit more complicated to make sure the L2VPNs are removed when removed, inserted, updated, etc. My question is: what is the actual advantage of keeping state on the SDX-LC (at least for l2vpns)?
There was a problem hiding this comment.
Hi Italo, the db in LC is to compare existing l2vpn's "status" with new "status" pulled from OXP, and only send specific l2vpn to SDX controller if status changes. If without database, we will need to periodically forward a long list of all l2vpn to SDX controller, and let SDX controller decide if status has changed. I think this would not be very efficient. I'm open to other options if you have better ideas.
There was a problem hiding this comment.
Yes, you are right Cong. From the efficiency perspective, this would save a few messages and bytes from being exchanged. However, when we designed the SDX-LC one of the ideas was not to maintain state or having much intelligence on it, so that we could have one central entity where the actual processing would happen (and then we can concentrate the decision making process and intelligence).
If we are to change this idea, that is fine for me. However, in that case, you have to delete the L2VPN from SDX-LC local database upon receiving a removal request from sdx-controller, right? can you please double check that?
sdx-lc/sdx_lc/handlers/sdx_controller_msg_handler.py
Lines 122 to 144 in 85bff21
Also, the pull L2VPN from OXP routine should check the local DB for missing L2VPNs and notify the Controller that missing L2VPN, right? Currently, this does not seems covered on the routine
Finally, for "alien" L2VPNs (i.e., the ones returned by OXP but not found on the local DB) we also should notify the SDX-Controller, right? currently, the routine is just ignoring them https://github.com/atlanticwave-sdx/sdx-lc/pull/203/changes#diff-92d2d9f91fcf0945f76b15e8823c1b9cc0b5106b66ea27f2c54d73caa69ff713R67
Periodically pull l2vpn status from OXP, and handle status change. If detected status change, send update to sdx-controller.