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.

Sign up for GitHub

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

Jump to bottom

feat: add instance dialing to health check #871

Merged
merged 40 commits into from
Aug 12, 2021
Merged

feat: add instance dialing to health check #871

merged 40 commits into from
Aug 12, 2021

Conversation

tifftoff
Copy link
Contributor

@tifftoff tifftoff commented Aug 5, 2021 •
edited

  • Add function to client that exposes instances list
  • Add Dial op to proxy readiness criteria
  • Close each connection after dialing
  • Add error path tests
  • Add health check flags to README
  • Dial all specified instances in non-fuse modes
  • Remove InstanceGetter
  • Change tests so they work with new NewServer
  • Add sample yaml config w/ probes
  • Add fields for each probe
  • Validate instances in fuse mode
  • Add end to end tests for successful dialing

Co-authored-by: Mona Zhang monazhang@google.com

@google-cla google-cla bot added the cla: yes label Aug 5, 2021
@enocom enocom changed the title Add instance dialing to health check feat: add instance dialing to health check Aug 5, 2021
@enocom enocom self-requested a review August 6, 2021 16:34
enocom
enocom reviewed Aug 6, 2021
View reviewed changes
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looking pretty good. A number of comments below.

cmd/cloud_sql_proxy/cloud_sql_proxy.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck_test.go Outdated Show resolved Hide resolved
proxy/proxy/client.go Outdated Show resolved Hide resolved
tests/healthcheck_test.go Outdated Show resolved Hide resolved
tests/healthcheck_test.go Outdated Show resolved Hide resolved
proxy/util/cloudsqlutil.go Outdated Show resolved Hide resolved
proxy/proxy/client.go Outdated Show resolved Hide resolved
tifftoff and others added 10 commits August 9, 2021 20:56
@tifftoff @monazhn
Add instance dialing to health check
dc5ae8b 
* Add function to client that exposes instances list
* Add Dial op to proxy readiness criteria
* Close each connection after dialing
* Add error path tests for dialing in readiness test
* Add health check flags to README
* Dial all specified instances in non-fuse modes
* Remove InstanceGetter
* Change tests so they work with new NewServer
* Fix flag
* Add sample yaml config w/ probes
* Add fields for each probe
* Validate instances in fuse mode
* Add end to end tests for successful dialing
@tifftoff @monazhn
add flag to skip these tests
9c82b43
@tifftoff @monazhn
add healthcheck end to end test environment variables
80d3e26
@tifftoff @monazhn
change instance name format
563f633
@tifftoff @monazhn
remove new env variables, use old env variables
5bf0f83
@tifftoff @monazhn
requested changes
759baf9
@tifftoff @monazhn
convert dialing tests into table driven tests
6988576
@tifftoff @monazhn
remove old example yaml file
952936a
@tifftoff @monazhn
remove build tags
dde5234
@monazhn
Formatting
75d078f
@enocom enocom requested review from enocom and kurtisvg August 9, 2021 21:07
enocom
enocom reviewed Aug 9, 2021
View reviewed changes
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few accidental additions that need to be removed.

proxy/util/cloudsqlutil.go Outdated Show resolved Hide resolved
proxy/proxy/client.go Outdated Show resolved Hide resolved
tests/healthcheck_test.go Outdated Show resolved Hide resolved
kurtisvg
kurtisvg suggested changes Aug 12, 2021
View reviewed changes
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/internal/healthcheck/healthcheck.go Outdated Show resolved Hide resolved
cmd/cloud_sql_proxy/cloud_sql_proxy.go Outdated Show resolved Hide resolved
tests/alldb_test.go Outdated Show resolved Hide resolved
tests/healthcheck_test.go Outdated Show resolved Hide resolved
@tifftoff tifftoff requested a review from kurtisvg August 12, 2021 17:45
@kurtisvg kurtisvg requested a review from enocom August 12, 2021 18:17
@enocom enocom merged commit eca3793 into GoogleCloudPlatform:main Aug 12, 2021
@enocom enocom deleted the my-cool-branch branch August 12, 2021 20:26
@monazhn monazhn mentioned this pull request Aug 12, 2021
Closed
@kurtisvg kurtisvg mentioned this pull request Oct 15, 2021
Closed
@enocom enocom mentioned this pull request Jul 19, 2022
Merged
@release-please release-please bot mentioned this pull request Aug 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monazhn monazhn monazhn left review comments

@enocom enocom enocom approved these changes

@kurtisvg kurtisvg kurtisvg approved these changes

Assignees

@enocom enocom

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tifftoff @enocom @kurtisvg @monazhn @kokoro-team