Feature/snmp-lldp #18
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
A new parameter has been added to the Descendants step so nodes linked with edge types "ownership_shared" are also "descendants" nodes. This is added so we can have several parents for a single node.
Proccon accepts HTTP POST JSON messages with metrics. It create Server and Software nodes as needed and add network info to Software nodes. It decides where to put each network info based on the cmdline of the process. If it does not find any suitable Software, it creates an "others" Software, with this approach we can capture most of the connections between nodes, even though we haven't the specific Software. Clean connections with a "garbage collector", delete connections not updated more than a configurable time. It runs periodically (configurable). Procpeering create edges between Software nodes that match (one have a connetion to an endpoint, the other have the listener for that endpoint). Creation of edges is always initilizated by client connections (no by process listeners) It handles also the delete of edges if the match is no longer available.
Since commit 27637e9 (hub: flush volatile resources from backend at startup), nodes/edges stored in the backend with Origin=analyzer.\.* are not loaded when starting skydive. Created a new function to insert nodes/edges into the graph with a different Origin value. Also modified to use the metric host as the field Host in nodes/edges.
When data is stored in the backend and recuperated, we need this metadata decoder to generate the appropiate object in go
Use a custom value in edge Origin value so skydive will load that edge from a persistant backend at boot time Also, instead of adding the edge to the graph in the GetABLinks, just return the edge, and the caller will add it to the graph.
If we have nodes sharing same private subnets, the previous implementation does not provide any option to differentiate those networks, so procpeering could end linking nodes incorrectly. This new feature could prefix all IPs with a value in the metric, so we move the responsability of knowing how subnets are used outside of skydive.
Until now, the CreatedAt/UpdatedAt values stored with each connection were set once received by Skydive, using time.Now(). This could generate incorrect data when the communication between clients and Skydive is unavailable, and clients start caching values. When the connection is restored, all those cached metrics are inserted with the same value in Skydive, losing that piece of information. Using the metric timestamp could also generate some problems if the clocks of skydive server and clients are not synced. Metrics from a client with its clock in the past could be deleted by the garbage collector, but the threshold to delete those metric will be usually much larger than the clock sync difference.
With the previous commit we avoid syncing to the backend each time a connection/listener is updated. But this open another problem, the backend not being updated. This change forces a sync each several node revisions to avoid that problem.
When the archive index is rolled, it was not keeping the mappings from the original archive index. This produces an incorrect index of values and could reach the max fields limit if we are using some of the fields that should be "flattened"
This probes were incorrectly started always
Moved some common parts to inside of addNetworkInfo. Added node.Revision checks inside some tests. Added test with multi metrics.
Instead of iterating through all metrics indididually, group all metrics going to the "others" software to avoid many changes to that node.
Isn't the best practice [1], but the upstream repo doesn't include the go.sum [2], so let's get rid of it to avoid building problems. [1] https://github.com/golang/go/wiki/Modules#should-i-commit-my-gosum-file-as-well-as-my-gomod-file [2] skydive-project#2111
If Metadata.TCPConn or Metadata.TCPListen was defined empty ({}), the
function removeOldNetworkInformation was panicking because an invalid
conversion to *NetworkInfo.
Now that type conversion is checked and return a warning message in case
of an invalid data type.
If a Software node does not have TCPConn or TCPListen Metadata keys, do not generate an error with the garbage collector. Software nodes could be created without those metadata keys and that is correct.
Like for TCPConn, patching others parts of the node should not affect the content of Metadata.TCPListen. Probably patching Metadata.TCPListen does't work, but we are not intended to modify that values by hand.
Completing commit 356688d ("api: create node and edge with the 'api' service type").
io.ReadAll requires Go >= 1.16, so this change allows Skydive to be built with older releases.
In some cases, under unknown circumstances, this "field" value could be empty and panic. The one in updateNetworkMetadata() has been seen in production. The other one is to avoid possible future problems.
When the connection passes through several proxies, X-Real-IP contains a proxy IP.
proccon: use X-Forwarded-For header for client logging
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
fef29e1 to
28b146f
Compare
The reserve side of Descendants step.
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
28b146f to
d7a843f
Compare
Se separa el procesado de cada métrica en una función propia (processMetric). Simplifica el código, reduce la complejidad ciclomática y nos permite testear de forma más sencilla. Se añaden test benchmark para esta función. Lo que queremos ver es como afecta la cantidad de nodos almacenados en el backend a la función processMetric. También se analiza como afecta el paralelismo (mayores bloqueos entre gorutinas). Se añade tracing usando OpenTelemetry, únicamente para esta probe. Se configura directamente un exporter de Jaeger. La idea es enviar a un colector de opentelemetry, pero hay problemas con las dependencias (etcd necesita una versión de gorpc y la librería del colector de opentelemetry una más moderna no soportada por etcd). Se ha añadido traceo en modo "debug", quiero decir que se han metido muchos spans probablemente innecesarios y se capturan todos. Esto posiblemente no sea razonable en producción.
Hasta ahora, por cada métrica que se recibía, se realizaba una llamada a GetNodes buscando por un Name y Type determinados. Esta función, por debajo, recorre todos los nodos del backend intentando hacer match, por lo que cuanto mayor número de nodos, peor performance. Y aún peor, mientras se hace esa búsqueda tenemos bloqueado el graph (para evitar escrituras mientras buscamos), por lo que todas las gorutinas que estén procesando métricas se bloquean. A partir de un número de nodos en el backend se supera un punto crítico donde llegan metricas más rápido de lo que es capaz de proesar, estas gorutinas se empiezan a encolar (esperando el lock) consumiendo memoria. En producción hemos llegado a ver tasas de crecimiento de 0.5GB/min. Para evitar esto tenemos que eliminar la llamada a GetNodes. Una opción podría ser añadir una cache que matcheara el Name+Type al id de un nodo. El problema es luego gestionar esa cache, cuanto tamaño debe tener, que hacer si no se encuentra el nodo (almacenamos que no existe o buscamos de nuevo?), etc. El backend almacena la información en un map[identificador]nodo, que al final es lo que queremos implementar para la caché. Por lo que se toma la decisión de generar un identificador a partir de los metadatos, de manera que podamos ir a buscar el nodo directamente sin tener que iterar por todos los nodos. Esto solo lo hacemos para los nodos tipo Server, que son sobre los que debemos buscar de manera general (los Software se buscan a partir del nodo Server padre). Ese identificador lo generamos como "Server__Name". De esta manera estamos asumiendo que el Name de un server debe ser único. Esta asunción parece razonable, ya que coincide con como se almacenan los datos en la CMDB. Con este cambio logramos reducir el tiempo de procesado, que antes crecía linealmente con el número de nodos para mantenerlo estable alrededor de 1ms (valor medido con el benchmark BenchmarkProcessMetricsSameNode, donde no se tiene en cuenta el almacenado real de los datos y borrado de others). Es dificil la comparación al haber cambiado la implementación, pero en simulaciones de producción se veían tiempos de decenas de milisegundos únicamente para la función GetNodes(). En una prueba haciendo replay de un tráfico real, con la implementación antigua, tras 60s de peticiones, la memoria se iba a más de 400MB, se habían creando 378 gorutinas y parte de los envíos fallaban. Con la nueva implementación ningún envío falla, la memoria se quedaba en unos 170MB (unos 20MB más que en el arranque) y el número de gorutinas se quedaba parejo al arranque (unas 150). Para conseguir ese tiempo también se ha mejorado el proceso de métricas. En vez de iterar por cada métrica, buscando su Server y luego gestionando el Software, las métricas se agrupan por host, se busca el nodo Server y luego se procesan las métricas para gestionar los nodos Software. De esta manera evitamos llamadas innecesarias, ya que cada grupo de métricas recibidas viene del mismo host y por lo tanto tendrá el mismo "host". En caso de que no sea así (por ejemplo, que hubiese una especie de proxy), la implementación funcionará correctamente, ya que agrupará las métricas por ese tag "host".
Ahora mismo no tenemos configurados receivers para enviar trazas. Tampoco está claro si deberíamos usar opentelemetry como SDK, ya que aún está en beta (https://opentelemetry.io/docs/go/). También parece útil que el tracing fuese global para todo skydive y las probes únicamente extendieran ese traceo. Tal vez se podría usar el SDK para go de jaeger y enviar a APM de elastic, que soporta ese protocolo. A nivel de código, tal vez se podría usar el graph Context para meter lo que necesite jaeger y que así fuese accesible por todas las probes.
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
d7a843f to
ef3f721
Compare
Fixes #22 proccon almacena en Metadata tipos de datos struct para su funcionamiento. Al parchear un nodo (API PATCH) se observaba que la conversión desde "interface" (el tipo de dato que obtenemos al usar GetMetadata sobre un nodo) no era convertible a estos structs que usábamos. Revisando el funcionamiento de API PATCH, se hace una versión de Metadata a JSON y luego se hace de nuevo el Unmarshal, pero sin usar los MetadataDecoders (encargados de hacer la conversión de JSON a tipos de datos determinados). Por eso, modificamos el Unmarshal de types.Nodes, para que use el Unmarshal de graph.Nodes y use los MetadataDecoders, de manera que el nodo parcheado se decodifique correctamente y se mantengan esos strucs almacenandos en Metadata.
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
ef3f721 to
f334441
Compare
Al hacer el último refactor para separar el procesamiento de métricas en
una función a parte se introdujo un error, no bloquear el grafo cuando
solicitamos los nodos child (Software) de un Server (LookupChildren en
la función processMetric).
Si en el momento de esa lectura, otra parte de skydive estaba intentando
escribir en el grafo, se producía un panic:
fatal error: concurrent map read and map write
Para solventar este problema se ha hecho una recolocación de los locks,
simplificando esta gestión.
Dentro de proccon los locks ahora quedan de la siguiente manera:
ServeHTTP
processMetrics
for hostMetrics
¦ LOCK
¦ get/create server
¦ processMetrics
¦ removeFromOthers
¦ UNLOCK
generateOthers
for hosts
¦ LOCK
¦ UNLOCK
cleanSoftwareNodes
LOCK
UNLOCK
De esta manera, cuando el servidor procesa un paquete de métricas, se
hace un lock por cada host que se está procesando y se elimina al
terminar ese host entero.
También solucionamos otra condición de carrera que se podía producir en
processMetrics, si entre que obteníamos un Server y se actualizaba su
metadata se producía alguna modificación en el nodo.
Esto era muy poco probable, ya que cada nodo en principio envía sus
métricas separadas por grandes intervalos de tiempo.
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
f334441 to
de5b872
Compare
Cambiadas las variables para tener un mejor significado (processMetric y processHostMetrics creaba confusión). El handler http sigue haciendo lo mismo, pero se configura de esta manera para poder añadir el net/http/pprof simplemente con un import, por si lo necesitamos meter de nuevo para analizar algún otro problema.
Tras el último commit se detectó un fallo por el cual, si se producia un
error al procesar las métricas, el Lock obtenido nunca se liberaba.
En las función ``generateOthers`` y ``processMetrics``, el Unlock se
realizaba al final de cada vuelta del bucle for, pero si sucedía algún
error, había llamadas return que podían salir de la función sin haber
llegado al Unlock.
Tener que gestionar el Lock+Unlock dentro de un bucle for (y en cada uno
de sus returns internos) indicaba una pobre arquitectura.
También se modifica la gestión del software "others", que antes se
realizaba como último paso del procesado global de métricas, pero tras
la agrupación de métricas por Servers realizada en anteriores commits,
ya no tenía sentido realizarla de ese modo.
La nueva arquitectura queda:
ServerHTTP (recibe petición con los datos en formato msgpack)
processMetrics (agrupa las métricas por la tag host)
processHost (crea/obtiene el nodo Server)
LOCK
processHostMetrics (añade las métricas a los softwares)
handleOthers (crea/obtiene el software 'others')
removeFromOthers (borra conex que ahora tienen sw propio)
addNetworkInfo (añade conex sin sw propio)
UNLOCK
También se ha modiicado el mensaje que se devuelve, con el número de
métricas recibidas y las que han generado algún error.
Telegraf (el que envía las métricas en nuestro caso), no gestiona ese
mensaje, pero nos puede servir para, analizando el tráfico HTTP, saber
si se están produciendo errores.
Esos errores siempre tendrán una traza en Skydive.
También se deja de asumir que las métricas siempre tendrán el tag
"host", ahora se comprueba y genera warning en caso de que no esté
presente.
This probe allows us to build a topology graph querying the LLDP information of a list of devices using the SNMP protocol.
pablombg
force-pushed
the
feature/snmp-lldp
branch
from
de5b872 to
e41db7d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.