kg: add new handler for rendering the topology graph via the metrics webserver #214
Conversation
cmd/kg/handlers.go
Outdated
| for _, n := range ns { | ||
| if n.Ready() { | ||
| nodes[n.Name] = n | ||
| hostname = n.Name |
There was a problem hiding this comment.
We always know the name of the node running this process, since it's provided as a command line argument to kg. I would saw we should use this value always. It will also help future-proof the code I think
There was a problem hiding this comment.
Done :) I now pass the hostname and the Kilo subnet to the handler.
However, wouldnt it be nicer if we export those fields from the Mesh and only pass the mesh to the handler?
There was a problem hiding this comment.
yes, something in this direction could be nice. Maybe we want the mesh to actually provide a method for getting ready nodes and peers? We need this logic in a bunch of places, like kgctl, so it could be nice to reuse
There was a problem hiding this comment.
This is a nice feature! Thanks for working on this, @stv0g!!!
Do you think you could also add an e2e test that at a minimum validates that the response contains some SVG XML? This would be sufficient IMO. Let me know if you need help here, or we can try to tackle it in a follow-up.
|
Hi @squat, thanks for the review :) I think i've addressed most of them. But it seems like Mesh does not really export anything. So I am not too sure how you want to handle this. I also gave the e2e tests a try in my last commit. Cheers, |
|
When I generate a png with |
Co-authored-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
|
Thanks @leonnicolas for looking into it. I had trouble with my dev environment to properly debug the issues here.
Thats due to missing fonts in your containers. The PR includes a change in the Dockerfile to install some standard font: |
I was building the container images from your branch :( |
|
I checked out your branch and run the e2e test. The svg is fine, but the png looks like this. Either we fix this, or disable the png (and other formats) options. Installing font-noto fixed the issue. |
|
@leonnicolas thanks for investigating. I added font-noto to the Dockerfile |
Dockerfile
Outdated
| @@ -11,7 +11,7 @@ ARG GOARCH | |||
| ARG ALPINE_VERSION=v3.12 | |||
| LABEL maintainer="squat <lserven@gmail.com>" | |||
| RUN echo -e "https://alpine.global.ssl.fastly.net/alpine/$ALPINE_VERSION/main\nhttps://alpine.global.ssl.fastly.net/alpine/$ALPINE_VERSION/community" > /etc/apk/repositories && \ | |||
| apk add --no-cache ipset iptables ip6tables wireguard-tools | |||
| apk add --no-cache ipset iptables ip6tables wireguard-tools graphviz font-bitstream-type1 font-noto | |||
There was a problem hiding this comment.
Can we do this without font-bitstream-type1 and just noto? It would be nice to keep the container with just the essentials
This commit leaves Noto as the only font package, as one font package is sufficient for the container. Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
|
I went ahead and added a commit removing the extra font. When testing this with the e2e tests everything works. I'm able to spin up a kind cluster, port forward to a Kilo pod and open |
|
See 1b5ad03 for the squashed commits in upstream. |
Closes #23
@squat , @leonnicolas: I am not too happy about the code-duplication for creating a
mesh.Topologyinstance.At the same time, I am lacking the deeper insight into Kilos code-base to understand where we could put this code.
I was thinking about a function
mesh.Backend.Topology()ormesh.Mesh.Topology().I guess the main question would be then weather we want to query the current topology directly from the backend or if we take the current state from the mesh object?