Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor code for the move to Kubernetes #5

Merged
merged 13 commits into from
Nov 25, 2024
Merged

Conversation

nikodemas
Copy link
Member

@nikodemas nikodemas commented Oct 25, 2024

This PR together with dmwm/CMSKubernetes#1559 prepares the udp-collector for the move to the Kubernetes infrastructure.

FYI @vkuznet @leggerf @brij01

@nikodemas nikodemas requested a review from vkuznet October 25, 2024 15:48
@vkuznet
Copy link
Contributor

vkuznet commented Oct 25, 2024

@nikodemas , I doubt that the changes you propose are valid. Let me break down for you the k8s requirements:

  • the /health end-point should provide status of underlying service. In case of your changes you have UDP server serving UPD requests and separate goroutine (thread) of HTTP server using health end-point. If UDP server experience an issue the HTTP server knows nothing about it. Therefore proposed implementation does not capture the state of health of UDP server
  • removing shell script has nothing to do with k8s, I suggest to keep them around for references. You may not need to pack them into a final image.
  • you remove code with prometheus metrics which are useful to understand performance of the underlying service. In k8s you can rely on those to know what are the limits required to ru the service.

I suggest to re-factor the code in the following way:

  • you may keep your HTTP server goroutine but it should do ping request to UDP server and receive back a reply. Doing this way it will probe the status of UDP server
  • you may keep code under one main which will run both UDP and HTTP server but you must expose proper ports for them in k8s, one to serve UDP incoming requests, and another to expose HTTP to make health probes
  • I suggest to put back prometheus metrics and use them within UDP server. What we should measure is performance of UDP server and not HTTP one. You may create a new UDP request for that, similar how original code is doing ping/pong exchange. A new UDP request will return UDP server prometheus metrics. And you HTTP server will need another end-point /metrics which will place UDP request to UDP server to get its metrics.

@nikodemas
Copy link
Member Author

@vkuznet thanks for your comments. I have adjusted everything accordingly:

  • the monitoring service now constantly sends ping messages to the udp server and /health endpoint checks the time since the last pong is received
  • Prometheus metrics are added back to the udp server monitor script
  • udp_server and udp_server_monitor are now started from one main since I couldn't start them with a shell script from a distroless image. however, having a shell script doesn't make sense anymore because now everything can be started with a single go executable, so I would like to keep the shell script deleted
  • I also updated the version in the GitHub actions as it was failing because of some package mismatch
  • after this is merged I should update the go.mod and the main.go to reference the github.com/dmwm/udp-collector/ again instead of the local folder

README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@vkuznet vkuznet merged commit 551b960 into dmwm:master Nov 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants