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

entrypoint: add liveness probe endpoint to the sidecar #33

Merged
merged 2 commits into from
May 28, 2024

Conversation

ridwanmsharif
Copy link
Collaborator

@ridwanmsharif ridwanmsharif commented May 23, 2024

b/326279447

This change uses the liveness probes as a way to guarantee a regular period of uninterrupted CPU for the sidecar to complete its prometheus scrapes and flushes to GMP.

Change-Id: Ic6f0ed38ade237d6179dea12268cd2575d221146

@@ -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
Copy link
Collaborator Author

@ridwanmsharif ridwanmsharif May 23, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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.

This change uses the liveness probes as a way to guarantee a regular
period of uninterrupted CPU for the sidecar to complete its prometheus
scrapes and flushes to GMP.

Change-Id: Ic6f0ed38ade237d6179dea12268cd2575d221146
@ridwanmsharif ridwanmsharif force-pushed the ridwanmsharif/sidecar-healthcheck branch from b48c63a to cc1ede6 Compare May 23, 2024 22:59
@ridwanmsharif ridwanmsharif requested a review from a team May 24, 2024 18:46
Change-Id: Id31f5294721e6583af84aa4127b8bae21a243267
@@ -11,7 +11,6 @@ RUN make build

FROM alpine:latest
RUN apk add --no-cache ca-certificates
RUN apk add openssl=3.1.4-r6 && apk upgrade openssl --no-cache

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?

Copy link
Collaborator Author

@ridwanmsharif ridwanmsharif May 24, 2024

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.

@ridwanmsharif ridwanmsharif requested a review from braydonk May 24, 2024 19:42
@ridwanmsharif ridwanmsharif merged commit 46199e0 into main May 28, 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.

3 participants