-
Notifications
You must be signed in to change notification settings - Fork 7
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
entrypoint: add liveness probe endpoint to the sidecar #33
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import ( | |
"fmt" | ||
"io/ioutil" | ||
"log" | ||
"net/http" | ||
"os" | ||
"os/signal" | ||
"path/filepath" | ||
|
@@ -34,6 +35,9 @@ var userConfigFile = "/etc/rungmp/config.yaml" | |
var otelConfigFile = "/run/rungmp/otel.yaml" | ||
var configRefreshInterval = 20 * time.Second | ||
var selfMetricsPort = 0 | ||
var livenessProbePort = 13133 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used this because this is the port we set up for the OTel sidecar https://cloud.google.com/run/docs/tutorials/custom-metrics-opentelemetry-sidecar#ship-code Its the default port for the healthcehck extension in OTel. But maybe we don't want to reuse this port because of that? I don't have strong opinions - wdyt @jefferbrecht? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as there's never a reason to run both at the same time then it should be fine. |
||
var livenessProbePath = "/liveness" | ||
var delayLivenessProbe = 5 * time.Second | ||
|
||
func getRawUserConfig(userConfigFile string) (string, error) { | ||
_, err := os.Stat(userConfigFile) | ||
|
@@ -84,6 +88,16 @@ func generateOtelConfig(ctx context.Context, userConfigFile string) error { | |
return nil | ||
} | ||
|
||
// The container is allocated CPU for the duration of the healthcheck. Delaying | ||
// the response to this probe allows the container to complete telemetry flushes | ||
// that may have been throttled. | ||
// | ||
// TODO(b/342463831): Use a more reliable way of checking if telemetry is being | ||
// flushed instead of using a static sleep. | ||
func healthcheckHandler(_ http.ResponseWriter, _ *http.Request) { | ||
time.Sleep(delayLivenessProbe) | ||
} | ||
|
||
func main() { | ||
// SIGINT handles Ctrl+C locally. | ||
// SIGTERM handles Cloud Run termination signal. | ||
|
@@ -101,6 +115,16 @@ func main() { | |
log.Fatal(err) | ||
} | ||
|
||
entrypointMux := http.NewServeMux() | ||
entrypointMux.HandleFunc(livenessProbePath, healthcheckHandler) | ||
|
||
go func() { | ||
err := http.ListenAndServe(fmt.Sprintf(":%d", livenessProbePort), entrypointMux) | ||
if err != nil && err != http.ErrServerClosed { | ||
log.Fatal(err) | ||
} | ||
}() | ||
|
||
// Spin up new-subprocess that runs the OTel collector and store the PID. | ||
// This OTel collector should use the generated config. | ||
var procAttr os.ProcAttr | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this; why no need for openssl anymore? Or was it there by accident in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need it, we pinned it to a version because the one alpine packaged had a vuln in it. Latest has now updated and this is no longer necessary so cleaned it up.