change restore pod readinessProbe to 9980 #1509
Conversation
|
Hi @lance5890. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
openshift-ci
bot
added
the
needs-ok-to-test
WalkthroughUpdated readiness probes in etcd restore pod configurations from TCP socket checks on port 2380 to HTTP GET checks on port 9980 with HTTPS scheme and /readyz endpoint path. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
bindata/etcd/quorum-restore-pod.yaml(1 hunks)bindata/etcd/restore-pod.yaml(1 hunks)
🔇 Additional comments (2)
bindata/etcd/quorum-restore-pod.yaml (1)
67-70: Architecture improvement: probing readyz sidecar instead of etcd directly is sound.The change from TCP socket probe on port 2380 to HTTP GET probe on port 9980 correctly delegates readiness checks to the dedicated
etcd-readyzsidecar container (lines 87–122). The sidecar runscluster-etcd-operator readyzand properly exposes port 9980 (line 110), aligning the probe configuration with the container setup.bindata/etcd/restore-pod.yaml (1)
123-126: Consistent update across restore pod configurations.Both restore pod manifests (quorum-restore-pod.yaml and restore-pod.yaml) are updated identically. The architecture mirrors quorum-restore-pod.yaml: the
etcd-readyzsidecar container (lines 143–177) properly listens on port 9980 (line 156), and the probe configuration aligns with the container setup.
| httpGet: | ||
| port: 9980 | ||
| path: readyz | ||
| scheme: HTTPS |
There was a problem hiding this comment.
🧩 Analysis chain
Verify HTTP probe path format: "readyz" should be "/readyz".
Kubernetes HTTPGet probes expect paths to begin with a forward slash. The path should be specified as "/readyz" rather than "readyz" to comply with the Probe API specification and ensure reliable probe execution.
Proposed fix:
httpGet:
port: 9980
- path: readyz
+ path: /readyz
scheme: HTTPS🌐 Web query:
Kubernetes HTTPGet probe path format requirements
💡 Result:
Short answer
- httpGet.path is a string URL path (defaults to "/"). The kubelet will send an HTTP GET to podIP:port. Use an absolute path (beginning with "/") as a best practice. [1][2]
Details / sources
- Kubernetes docs: httpGet has fields path (Path to access on the HTTP server) and default is "/". The probe is sent to the pod IP and specified port. [1]
- API reference / HTTPGetAction: path is a string (Path to access on the HTTP server). Treat it as a normal URL path. [2]
If you want, I can show examples and edge cases (query strings, virtual-hosting via Host header, or HTTPS scheme).
Sources:
[1] Kubernetes docs — Configure Liveness/Readiness/Startup probes.
[2] Kubernetes HTTPGetAction API reference.
</shell_scripts>
🏁 Script executed:
#!/bin/bash
# Check the current state of the file and related probe configurations
echo "=== Checking quorum-restore-pod.yaml ==="
fd -t f "quorum-restore-pod.yaml" --exec cat {}
echo -e "\n=== Searching for other HTTPGet probes in YAML files ==="
rg -n "httpGet:" --type yaml -A 3 -B 1 bindata/Length of output: 6185
Fix HTTPGet probe paths to use absolute paths across all affected files.
Kubernetes best practice requires httpGet probe paths to begin with "/" (absolute path). The default value is "/" and the probe is sent to podIP:port. Update the following files:
bindata/etcd/quorum-restore-pod.yamlline 69:path: readyz→path: /readyzbindata/etcd/pod.gotpl.yamlline 233:path: readyz→path: /readyzbindata/etcd/pod.gotpl.yamlline 242:path: healthz→path: /healthzbindata/etcd/pod.gotpl.yamlline 253:path: readyz→path: /readyzbindata/etcd/restore-pod.yamlline 125:path: readyz→path: /readyz
🤖 Prompt for AI Agents
In bindata/etcd/quorum-restore-pod.yaml around lines 67-70 (line 69),
bindata/etcd/pod.gotpl.yaml around lines 233, 242, and 253, and
bindata/etcd/restore-pod.yaml around line 125, the httpGet probe path values are
relative (e.g., "readyz", "healthz"); update each to use absolute paths by
prefixing with a slash (e.g., "readyz" -> "/readyz", "healthz" -> "/healthz") so
probes become path: /readyz or path: /healthz respectively. Ensure you update
all listed files and lines to use the absolute path format for Kubernetes
httpGet probes.
| httpGet: | ||
| port: 9980 | ||
| path: readyz | ||
| scheme: HTTPS |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify HTTP probe path format: "readyz" should be "/readyz".
Same issue as in the companion file: Kubernetes HTTPGet probes expect paths to begin with a forward slash. Correct the path to "/readyz" for compliance with the Probe API specification.
Proposed fix:
httpGet:
port: 9980
- path: readyz
+ path: /readyz
scheme: HTTPSVerify HTTP probe path format: "readyz" should be "/readyz".
Kubernetes HTTPGet probes access a path on the HTTP server, with "/" as the default. The kubelet sends an HTTP request to the specified port and path to perform the check. All official Kubernetes examples use leading slashes in paths, such as /healthz. The current path readyz lacks the required leading slash and should be corrected to /readyz to comply with HTTP URI conventions and Kubernetes best practices.
Proposed fix:
httpGet:
port: 9980
- path: readyz
+ path: /readyz
scheme: HTTPS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| httpGet: | |
| port: 9980 | |
| path: readyz | |
| scheme: HTTPS | |
| httpGet: | |
| port: 9980 | |
| path: /readyz | |
| scheme: HTTPS |
🤖 Prompt for AI Agents
In bindata/etcd/restore-pod.yaml around lines 123 to 126, the HTTP probe path is
set to "readyz" without a leading slash; change the path value to "/readyz" so
the HTTPGet probe uses a proper URI path (add the leading "/" to the path
string).
|
/approve /test ? |
|
@tjungblu: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use 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-sigs/prow repository. |
|
/test e2e-aws-etcd-recovery |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance5890, tjungblu 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 |
openshift-ci
bot
added
the
approved
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
/test e2e-aws-etcd-recovery |
|
/retest |
|
@lance5890: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Since PR #1367 was merged, we should consider changing the restore pod's readiness probe from tcpSocket: 2380 to httpGet: 9980. This would allow the probe to actually check etcd's readiness status, rather than just verifying that port 2380 is listening