-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(cloudrun): create sample service for cloud run readiness probe and service health feature #5368
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
Conversation
6dc5066 to
61ce8ed
Compare
um I couldn't reproduce it locally |
9681278 to
41ef965
Compare
|
https://btx.cloud.google.com/invocations/c0a39662-c209-4994-bbc2-387355fb9dcb/targets is not failing run/testing tests 🎉 |
|
/gemini summary |
Summary of ChangesThis pull request introduces a new sample service for Cloud Run, demonstrating the upcoming readiness probe and service health features. It includes updates to the internal CI tooling to support readiness probe configurations, a new Go service with a web UI, and associated GCS bucket management for health data. Highlights
Changelog
Activity
|
|
/gemini review |
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.
Code Review
This pull request introduces a new sample service for Cloud Run to demonstrate the readiness probe and service health features. The changes include updates to the CI tooling to support readiness probe configurations, the new service's Go source code, HTML templates, and end-to-end tests.
My review has identified a few areas for improvement. There are some typos in variable names and test data. The CI tooling has a bug in how it constructs the gcloud command for readiness probes. The new service's HTML has some minor issues with invalid attributes. Most importantly, the service implementation has a potential performance bottleneck due to excessive locking around network operations, which could lead to request timeouts under load. I've provided detailed comments and suggestions for each of these points.
0042fb0 to
71c40e5
Compare
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.
When structuring samples, please consider exactly what you're intending to demonstrate. You may want to read the samples style guide for additional advice.
This sample can be simplified a lot by removing the caching, and using appropriate API clients instead of rolling your own.
| var err error | ||
|
|
||
| var longRegion string | ||
| if longRegion, err = queryMetadataServer("/computeMetadata/v1/instance/region"); err != nil { |
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.
please use the metadata client from https://pkg.go.dev/cloud.google.com/go/compute/metadata - it has some builtin retry logic that makes it more robust than raw http, and avoids hardcoding many of these strings.
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.
Seems like Cloud Run metadata server doesn't exactly have the same interface as GCE metadata server. For example "computeMetadata/v1/instance/region" not present in GCE metadata server.
| log.Fatal(err) | ||
| } | ||
|
|
||
| apiURL := fmt.Sprintf( |
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.
again, you should be using a client library. https://pkg.go.dev/cloud.google.com/go/run
this will greatly simplify this code by removing the hardcoded urls, the structs for returned objects, and the authentication wrangling.
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.
the go client doesn't have this "readiness probe" field in the container type yet, but it is in v1 API
| } | ||
| }() | ||
|
|
||
| go func() { |
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.
spawning goroutines in samples is generally frowned upon unless there's some concurrent processing necessary.
these seem to be tidying up the GCS cache? what is the significance of the GCS cache to this specific sample? it seems to be adding a lot of complexity here and distracting from readiness probes entirely.
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.
The UI looks like this. The main reason to leverage GCS is to manipulate the "readiness" state for other Cloud Run instances. If enough readiness status are reporting unhealthy, and if the Cloud Run service is attached to a load balancer, that load balancer will failover the traffic to a different region. This is why each running instance has to periodically sync from GCS.
run/service-health/go.mod
Outdated
| @@ -0,0 +1,57 @@ | |||
| module service_health | |||
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.
(minor) Go does not usually have separators in package or directory names. directory name and package name should match.
see the style guide https://google.github.io/styleguide/go/decisions#package-names
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.
oh the style guide said it should be "short and contain only lowercase letters."
but I see other examples like module github.com/GoogleCloudPlatform/golang-samples/run/hello-broken
should I use just module servicehealth or module github.com/GoogleCloudPlatform/golang-samples/run/service-health?
b9786eb to
d89524a
Compare
`cloudrunci` updates to support readiness probe additions in #5368 * feat(cloudrun): allow adding new readiness probe to cloud run * fix(cloudrun): add comment to Readiness field --------- Co-authored-by: Brian Dorsey <[email protected]>
…nd service health feature
|
@briandorsey This PR should be ready for review, thank you in advance! |
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.
Thank you for the submission and all the edits.
I confirmed that we're not seeing new test-suite breakage due to these changes. (The overall tests failed due to errors in other packages.)
@mysterytony I recognize that it's not possible to use client libraries in cases where they don't yet support new/unreleased features. Could I ask you to make a follow up PR to update this sample code to use the client libraries once they do have the support you need (ReadinessProbe)?
Yes I will do that. |
|
I've taken over this review from @muncus, so I'm going to go head and merge. |
Description
Create a sample service for Cloud Run to demonstrate the upcoming readiness probe & service health feature
Checklist
go test -v ./..(see Testing)run/testingdirectoryGOLANG_SAMPLES_E2E_TEST=true GOLANG_SAMPLES_PROJECT_ID=lihenan-s8s-networking-test go test -run ^TestServiceHealth$ -v .gofmt(see Formatting)go vet(see Formatting)