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
kubeadm: use non-serializable startup probe for etcd pods #110744
kubeadm: use non-serializable startup probe for etcd pods #110744
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/triage accepted /cc @ahrtr |
@neolit123: GitHub didn't allow me to request PR reviews from the following users: ahrtr. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
Looks good to me, but probably you need to add at least one test case? |
/lgtm |
@ahrtr: changing LGTM is restricted to collaborators 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 |
I checked the cluster etcd configuration in #97034 changed the liveness probe to use kubernetes/cluster/gce/manifests/etcd.manifest Lines 72 to 84 in 487512b
Does
|
For the NOSPACE alarm, |
The benefit of using |
Yes, there is no change on this. And users can exclude some alarm using the parameter |
cmd/kubeadm/app/phases/etcd/local.go
Outdated
@@ -204,8 +203,8 @@ func GetEtcdPodSpec(cfg *kubeadmapi.ClusterConfiguration, endpoint *kubeadmapi.A | |||
v1.ResourceMemory: resource.MustParse("100Mi"), | |||
}, | |||
}, | |||
LivenessProbe: staticpodutil.LivenessProbe(probeHostname, etcdHealthEndpoint, probePort, probeScheme), | |||
StartupProbe: staticpodutil.StartupProbe(probeHostname, etcdHealthEndpoint, probePort, probeScheme, cfg.APIServer.TimeoutForControlPlane), | |||
LivenessProbe: staticpodutil.LivenessProbe(probeHostname, "/health?serializable=true", probePort, probeScheme), |
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.
LivenessProbe: staticpodutil.LivenessProbe(probeHostname, "/health?serializable=true", probePort, probeScheme), | |
LivenessProbe: staticpodutil.LivenessProbe(probeHostname, "/health?exclude=NOSPACE&serializable=true", probePort, probeScheme), |
@ahrtr so /health?exclude=NOSPACE&serializable=true
would be better here.
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.
I am not sure about if we need the \
curl -i -v -G http://127.0.0.1:2381/health?serializable=false\&exclude=NOSPACE
[root@paco ~]# curl -i -v -G http://127.0.0.1:2381/health?serializable=false\&exclude=NOSPACE
* Trying 127.0.0.1:2381...
* Connected to 127.0.0.1 (127.0.0.1) port 2381 (#0)
> GET /health?serializable=false&exclude=NOSPACE HTTP/1.1
> Host: 127.0.0.1:2381
> User-Agent: curl/7.61.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Date: Mon, 27 Jun 2022 10:31:36 GMT
Date: Mon, 27 Jun 2022 10:31:36 GMT
< Content-Length: 29
Content-Length: 29
< Content-Type: text/plain; charset=utf-8
Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host 127.0.0.1 left intact
{"health":"true","reason":""}[root@paco ~]#
[root@paco ~]#
[root@paco ~]# curl -i -v -G http://127.0.0.1:2381/health?serializable=false&exclude=NOSPACE
[1] 27110
[root@paco ~]# * Trying 127.0.0.1:2381...
* Connected to 127.0.0.1 (127.0.0.1) port 2381 (#0)
> GET /health?serializable=false HTTP/1.1
> Host: 127.0.0.1:2381
> User-Agent: curl/7.61.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 503 Service Unavailable
HTTP/1.1 503 Service Unavailable
< Content-Type: text/plain; charset=utf-8
Content-Type: text/plain; charset=utf-8
< X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
< Date: Mon, 27 Jun 2022 10:31:41 GMT
Date: Mon, 27 Jun 2022 10:31:41 GMT
< Content-Length: 44
Content-Length: 44
<
{"health":"false","reason":"ALARM NOSPACE"}
* Connection #0 to host 127.0.0.1 left intact
If no \
, curl
will send only the first param.
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.
I don't think we need ' \ ' here. The library can handle it automatically, no need to worry about it.
Please try the URL in browser, and browser can handle it, just in the same way as the golang library does.
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 I test it, I find there is a thing. etcd will not be killed after the first kill.
In the past, if there is NOSPACE alert, then pod's liveness will fail for /health.
Then etcd pod will be started again. The startup probe will never pass. So no liveness probe will be triggered.
Pod will keep running like below
[root@paco ~]# kubectl get events -n kube-system -w| grep etcd
13m Warning Unhealthy pod/etcd-paco Startup probe failed: HTTP probe failed with statuscode: 503
12m Normal Killing pod/etcd-paco Stopping container etcd
[root@paco ~]# kubectl get pod -n kube-system etcd-paco
NAME READY STATUS RESTARTS AGE
etcd-paco 1/1 Running 0 13m
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.
So the difference
- adding
exclude=NOSPACE
: no restart if it is nospace - without
exclude=NOSPACE
: one restart
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.
It's expected behavior for /health?exclude=NOSPACE&serializable=true
. It depends on what's your expected behavior.
With exclude=NOSPACE
- Good side: etcd can still serve range/read requests. Administrator can fix the space issue for each member one by one, and the etcd cluster can always serve range/read requests during the process.
- Bad side: The member (even the whole cluster) is actually in unhealthy status. Users may not notice it unless there is an automatic monitoring system, such as prometheus.
Without exclude=NOSPACE
- Good side: The etcd member will be restarted by the liveness Probe, and will never get started due to startup Probe. Accordingly it can get immediate attention from administrator;
- Bad side: It can't serve even range/read requests. The system which depends on the etcd cluster will be totally down.
Proposal
How about expose the configuration to users? And the default value is with exclude=NOSPACE
?
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.
How about expose the configuration to users? And the default value is with exclude=NOSPACE ?
users are already able to customize the probes with static pod manifests patches:
https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/control-plane-flags/#patches
so i guess the discussion here is leading towards having the suggestion as here:
https://github.com/kubernetes/kubernetes/pull/110744/files#r905816216
by default.
i will update the PR.
As per the etcd maintainers' recommendation - startup probes shouldn't be serialized, while the liveness probes should be.
e71cc7c
to
2829fc0
Compare
/hold cancel |
/lgtm |
Q: Can someone help me understand why startupProbe (serializable=false) != livenessProbe (serializable=true) ? I've read the background etcd comment, which says:
I think the usual k8s pattern suggests we use readinessProbe to prevent an etcd member from being used until it is ready. When readinessProbe uses serializable=false, I think that verifies pod has exited "startup phase" before clients use it. Why is serializable=false startupProbe also useful/necessary? To be clear, I think this startupProbe is mostly harmless too, and I don't have an actual objection to it. I think startupProbe serializable=false means an etcd pod in an unhealthy cluster will restart a few extra times when startupProbe eventually times out (compared to startupProbe serializable=true, which would transition to live=true ready=false). I think the extra restarts might extend the recovery time of the cluster, but won't actually prevent it recovering. Iiuc. |
on this PR and on the issue kubernetes/kubeadm#2567 we had all the discussion on this topic. @ahrtr might be able to provide additional comments. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
As per the etcd maintainers' recommendation - startup probes
shouldn't be serialized, while the liveness probes should be.
etcd-io/etcd#14048 (comment)
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#2567
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: