skydive-project / skydive

An open source real-time network topology and protocols analyzer
https://skydive.network
Apache License 2.0
2.68k stars 404 forks source link

Elasticsearch: store prevRevision when fetching nodes/edges #2330

Closed adrianlzt closed 3 years ago

adrianlzt commented 3 years ago

When skydive fetchs nodes and edges at boot time from elasticsearch, those nodes/edges were not being saved in the b.prevRevision map.

If later we tried to change the metadata of that nodes/edges (using MetadataUpdated) it failed because it does not recognise that node/edge.

This commits modifies the GetNodes and GetEdges functions to store into b.prevRevision nodes/edges fetched from Elasticsearch.

Fixes #2328

adrianlzt commented 3 years ago

I am not sure the full implications could take this change.

Maybe there are cases when we want to GetNodes but not to index them?

I guess another approach is in func (c *CachedBackend) OnStarted(), for each node/edge fetched, add it again, to populate b.prevRevision. But that will create unneeded traffic to elasticsearch.

lebauce commented 3 years ago

Thanks for spotting and fixing the problem. I don't see a use case for not indexing neither.

Doing so in GetNodes/GetEdges work but it needs to be done only once and GetNodes/GetEdges can be called many times. I like your proposal to do it in the OnStarted function. One possibility would be to add a need a Sync(Backend, *ElementFilter) on the PersistentBackend interface that would do https://github.com/skydive-project/skydive/blob/master/graffiti/graph/cachedbackend.go#L224-L231, and in the case of Elasticsearch would provision the prevRevision field (it would also avoid having to return a potentially big list of nodes and edges just to call NodeAdded and EdgeAdded.

Then in CachedBackedn.OnStarted, we would simply call c.persistent.Sync(c.memory, elementFilter). What do you think ?

lebauce commented 3 years ago

I just pushed a branch with the proposal : https://github.com/lebauce/skydive/commit/c4d00c95cb0300bacaba700b7b16472b2dbf7b63

adrianlzt commented 3 years ago

Sure, much better your option