This adds an etcd health check endpoint to kube-apiserver #49412
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Hi @bjhaid. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/M
|
/assign @deads2k |
k8s-ci-robot
added
cncf-cla: yes
| @@ -20,6 +20,9 @@ import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/spf13/pflag" | |||
| "net/http" | |||
| "k8s.io/apiserver/pkg/server/healthz" | |||
| "k8s.io/kubernetes/cmd/kube-apiserver/app/preflight" | |||
There was a problem hiding this comment.
This dependency won't be allowed. You'll need to move the etcd check code to the k8s.io/apiserver staging section somewhere.
There was a problem hiding this comment.
Is the reverse allowed, that is adding a k8s.io/apiserver/... dependency to k8s.io/kubernetes/cmd/kube-apiserver/app/preflight? So I don't end up duplicating code
There was a problem hiding this comment.
Is the reverse allowed, that is adding a k8s.io/apiserver/... dependency to k8s.io/kubernetes/cmd/kube-apiserver/app/preflight
yes the reverse is allowed
There was a problem hiding this comment.
Can I just move preflight and it's test in to staging and fix the references?
There was a problem hiding this comment.
Can I just move preflight and it's test in to staging and fix the references?
Yes. This seems more like apiserver/pkg/storage/etcd3/healthz
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
This is looking good. |
bjhaid
force-pushed
the
etcd_healthz_endpoint
branch
2 times, most recently
from
89ee978
to
9e62dc9
Compare
|
looks like something is going wrong. |
bjhaid
changed the title
I spoke to folks in #sig-testing and they told me to run |
bjhaid
force-pushed
the
etcd_healthz_endpoint
branch
from
9e62dc9
to
9f77bb5
Compare
bjhaid
changed the title
bjhaid
force-pushed
the
etcd_healthz_endpoint
branch
4 times, most recently
from
0630735
to
bbed16e
Compare
|
/test pull-kubernetes-bazel |
2 similar comments
|
/test pull-kubernetes-bazel |
|
/test pull-kubernetes-bazel |
|
/test Submit Queue |
|
@deads2k needs a lgtm.. |
| @@ -20,6 +20,9 @@ import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/spf13/pflag" | |||
| "k8s.io/apiserver/pkg/server/healthz" | |||
| "k8s.io/apiserver/pkg/storage/etcd3/preflight" | |||
| "net/http" | |||
There was a problem hiding this comment.
nit: this goes in first section of imports since its a stdlib
| @@ -20,6 +20,9 @@ import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/spf13/pflag" | |||
| "k8s.io/apiserver/pkg/server/healthz" | |||
There was a problem hiding this comment.
nit: want an empty line before these. we normally put a gap between dependencies and in-project deps
bjhaid
force-pushed
the
etcd_healthz_endpoint
branch
from
bbed16e
to
419f60a
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
the
approved
bjhaid
force-pushed
the
etcd_healthz_endpoint
branch
from
419f60a
to
47d748c
Compare
k8s-github-robot
removed
the
lgtm
|
/test pull-kubernetes-unit |
|
/test pull-kubernetes-verify |
|
/test pull-kubernetes-e2e-kops-aws |
|
/test pull-kubernetes-e2e-gce-etcd3 |
|
@bjhaid: you can't request testing unless you are a kubernetes member. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@bjhaid: you can't request testing unless you are a kubernetes member. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-kubernetes-e2e-kops-aws |
|
@bjhaid: you can't request testing unless you are a kubernetes member. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bjhaid, deads2k Associated issue: 48215 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 49989, 49806, 49649, 49412, 49512) |
addressing #48215.
What this PR does / why we need it:
This ensures kube-apiserver
/healthzendpoint fails whenever connectivity cannot be established to etcd, also ensures the etcd preflight checks works with unix socketsWhich issue this PR fixes: fixes #48215
Special notes for your reviewer:
This PR does not use the etcd client directly as the client object is wrapped behind the storage interface and not exposed directly for use, so I decided to reuse what's being done in the preflight. So this will only check fail for connectivity and not etcd auth related problems. I did not write tests for the endpoint because I couldn't find examples that I could follow for writing tests for healthz related endpoints, I'll be willing to write those tests if someone can point me at a relevant one.
Release note:
@deads2k please help review, thanks!