[BGPCEP-904] Graph-impl is performing cross-datastore transactions Created: 23/Apr/20 Updated: 03/Aug/20 Resolved: 03/Aug/20 |
|
| Status: | Resolved |
| Project: | bgpcep |
| Component/s: | General |
| Affects Version/s: | None |
| Fix Version/s: | Aluminium |
| Type: | Bug | Priority: | High |
| Reporter: | Robert Varga | Assignee: | Olivier Dugeon |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
ConnectedGraphServer is populating both Operational and Configuration datastores in a single transaction. This leads to sub-optimal performance and is deprecated funcitonality, which is going away in the next release.
|
| Comments |
| Comment by Olivier Dugeon [ 23/Apr/20 ] |
|
Good catch. Indeed, ConnectedGraphServer initializes both Operational and Configuration datastore. Initialize Configuration is wrongly placed. I'll split initialization in two separate transactions. |
| Comment by Robert Varga [ 23/Apr/20 ] |
|
I wonder, what is the intent in actually writing to CONFIG here? I mean that's persisted by default, so should it really be initialized each startup... |
| Comment by Olivier Dugeon [ 23/Apr/20 ] |
|
Graph in Config Datastore is here to give the possibility to manually provide a topology or complete an automatic topology discovery (e.g. through BGP-LS). But Path Algorithms are working on the connected graph which is computed from the Operational Graph. So, GraphListener is here to listen to Graph Configuration and populate Graph Operational like LinkstateGraphBuilder done with Linkstate Routes. If Graph Configuration is not initialized, I'm afraid that GraphListener could not start correctly. I'm thinking to initialize Graph Configuration in GraphListener class, but, this is more complex as simply split the Graph initialization in two separate transactions. Now, regarding persistence, I could add some trigger to initialize the Graph Configuration only if it's not exist if it makes sense. |
| Comment by Robert Varga [ 23/Apr/20 ] |
|
Why would the GraphListener not start? I mean it should be just fine with no config being present, right? Also, the init actually actively overwrite config datastore – so even if there was something there, you'll end up wiping it on start – that does not feel like config at all. As for the BGP-LS discovery, etc. that certainly should not go through config – it very much feels liek you really want to have multiple instances in oper and each instance having a semantic meaning. The user-supplied configuration (which would be persisted) would live in config, but code components would never actively write to it and instead move it to oper – this is very much what BGP already does anyway ... |
| Comment by Olivier Dugeon [ 23/Apr/20 ] |
I try to launch the Graph without the creation of initial GraphModel top node in config datastore, but in this case, you are unable to add some graph in config. I need at least to create the graph-model top node in the config datastore. Now, I agree that it is certainly the role of GraphListnener to do that instead of ConnectedGraphServer.
I just would create the graph-model top node in the config datastore like I do for the operational datastore. Perhaps we could do this differently with some xml file. But well, It is just the top node with no graph inside. Just let me know what it is the better design.
As mention, Graph model in configuration is just here to give the possibility to provide a graph with the REST API instead of through BGP-LS. This allows an operator to provide a network topology that it collect through another tools which not speaks BGP-LS. This could be also helpful to provide static topology or complement a topology acquired through BGP-LS. In all cases, the active topology on which Path Computation Algorithms oper is the Connected Graph Model issue from the Graph Model in operational datastore. Just let me know where is the best place to create the graph-model top node for the Configuration datastore |
| Comment by Olivier Dugeon [ 23/Apr/20 ] |
|
So, I finally simply drop initialization of GraphModel in Configuration datastore |