Remove readiness probe otel-agent sidecars #1791
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @tiffanny29631, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the robustness of the otel-agent deployment by implementing Kubernetes startupProbe definitions. This change provides a dedicated grace period for the agent to fully initialize, mitigating issues where slow startup times could lead to unnecessary pod restarts and improve overall stability of the service.
Highlights
- Kubernetes Probe Configuration: Introduced
startupProbeconfigurations for theotel-agentacross multiple Kubernetes manifest files. This new probe specifically targets port13133with anhttpGetcheck, configured to allow up to 5 minutes for the agent to initialize before standard readiness and liveness checks begin. - Reliability Improvement: The addition of the
startupProbeaims to prevent premature restarts andCrashLoopBackOfferrors for theotel-agent, especially in environments with heavy load or resource constraints where the agent might take longer to bind to its health check port.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
| startupProbe: | ||
| httpGet: | ||
| path: / | ||
| port: 13133 | ||
| failureThreshold: 30 # Allow up to 5 minutes (30 * 10s) | ||
| periodSeconds: 10 | ||
| initialDelaySeconds: 0 |
There was a problem hiding this comment.
Consider defining the startupProbe configuration in a central location (e.g., a common YAML file or a Go struct) and referencing it across these manifests to avoid duplication and improve maintainability. If not feasible, consider adding a comment referencing this file as the source of truth to mitigate inconsistent updates.
| startupProbe: | ||
| httpGet: | ||
| path: / | ||
| port: 13133 | ||
| failureThreshold: 30 # Allow up to 5 minutes (30 * 10s) | ||
| periodSeconds: 10 | ||
| initialDelaySeconds: 0 |
There was a problem hiding this comment.
Consider defining the startupProbe configuration in a central location (e.g., a common YAML file or a Go struct) and referencing it across these manifests to avoid duplication and improve maintainability. If not feasible, consider adding a comment referencing this file as the source of truth to mitigate inconsistent updates.
| startupProbe: | ||
| httpGet: | ||
| path: / | ||
| port: 13133 | ||
| failureThreshold: 30 # Allow up to 5 minutes (30 * 10s) | ||
| periodSeconds: 10 | ||
| initialDelaySeconds: 0 |
There was a problem hiding this comment.
Consider defining the startupProbe configuration in a central location (e.g., a common YAML file or a Go struct) and referencing it across these manifests to avoid duplication and improve maintainability. If not feasible, consider adding a comment referencing this file as the source of truth to mitigate inconsistent updates.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -195,6 +195,13 @@ data: | |||
| volumeMounts: | |||
| - name: otel-agent-config-reconciler-vol | |||
| mountPath: /conf | |||
| startupProbe: | |||
There was a problem hiding this comment.
Could you clarify what the intent is with this change? The commit message mentions liveness probe, however there is no liveness probe defined for these containers (since 2023).
Also I expect the readinessProbe on these containers may be superfluous as none of these Deployments are exposed through a Service (except for otel-collector Deployment)
There was a problem hiding this comment.
Updated in http://b/419026380#comment3. The Readiness probe for otel-agent side car can also be removed to align with the config of other containers. The readiness failure of telemetry containers is not necessary, and could block the reconciler and resource group controller pods.
There was a problem hiding this comment.
The readiness failure of telemetry containers is not necessary, and could block the reconciler and resource group controller pods.
Could you explain why the readiness probe blocks the reconciler and resource-group-controller Pods?
There was a problem hiding this comment.
The PR has been updated to removal of the readiness probe of otel-agent side car in reconciler / reconciler-manager / resource group controller.
There was a problem hiding this comment.
The PR has been updated to removal of the readiness probe of otel-agent side car in reconciler / reconciler-manager / resource group controller.
The motivation and intent for this change is still unclear. Can you explain the why of this change?
There was a problem hiding this comment.
Ack, can you include the justification in the commit message and PR descriptions?
causing operational issues
Can you clarify what are the operational issues?
"pod not ready" alarms
Can you clarify which alarms this is referring to?
The change seems reasonable but I'm a bit surprised by the amount of churn. I'd perhaps consider timeboxing this effort given it doesn't solve the underlying issue of the otel-agent being unresponsive
There was a problem hiding this comment.
Updated, also limited the change to only remove the readinessProbe for consistent test result.
There was a problem hiding this comment.
Updated, also limited the change to only remove the readinessProbe for consistent test result.
Could you elaborate a bit more on this? What was the cause of the prior test failures and why are the tests passing now?
If the readinessProbe is removed, I would expect the health check endpoint/port to be removed as well. Why is the health check endpoint retained?
There was a problem hiding this comment.
The test failure was inconsistent and flaky between runs. You can see the test history in the PR email thread.
Previously even after completely removing the readiness configuration, some tests still showed the resource-group-controller and reconciler-manager pods with readiness settings. This caused failures because the port/health_check extension was already removed.
When I ran the same tests on my local kind cluster, this issue didn't happen, so I can't identify the root cause yet. Suggest we could do this in two steps - first remove the readiness probe, then remove the port & extension infrastructure.
There was a problem hiding this comment.
Wait I might have reproduced it, allow some more time to figure out the rootcause.
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
94c55d8 to
1fe01c7
Compare
tiffanny29631
changed the title
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
1fe01c7 to
650e0f8
Compare
|
/restest-required |
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
ab789e4 to
063cf7a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes readiness probes and health check configurations from otel-agent sidecar containers across multiple controllers to prevent unnecessary pod unready states caused by health check port binding failures or CPU throttling issues.
- Removes health check extension configuration from OpenTelemetry collector configs
- Removes readiness probes and port 13133 from container specifications
- Updates test expectations to reflect the removal of health check endpoints
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/kustomization/expected.yaml | Updates test expectations to remove health check configurations and readiness probes |
| pkg/reconcilermanager/controllers/reconciler_base_test.go | Removes liveness/readiness probe configurations from test cases and updates expected patch strings |
| manifests/templates/resourcegroup-manifest.yaml | Removes health check extension and readiness probe from resourcegroup controller otel-agent |
| manifests/templates/reconciler-manager.yaml | Removes health check port and readiness probe from reconciler-manager otel-agent |
| manifests/templates/reconciler-manager-configmap.yaml | Removes health check port and readiness probe from reconciler template |
| manifests/otel-agent-reconciler-cm.yaml | Removes health check extension configuration from reconciler otel-agent config |
| manifests/otel-agent-cm.yaml | Removes health check extension configuration from base otel-agent config |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
4fe1608 to
554ec98
Compare
| @@ -91,10 +91,6 @@ spec: | |||
| volumeMounts: | |||
| - name: otel-agent-config-vol | |||
| mountPath: /conf | |||
| readinessProbe: | |||
There was a problem hiding this comment.
Odd. I removed the readinessProbe from both the reconciler-manager and resource-group-controller configurations, but when checking the pod description for the failed tests, the readiness probe is still showing up. Also there are two resource-group-controller pods running, with one not being ready due to readiness failure, which seems unusual.
There was a problem hiding this comment.
Same issue does not apply to the test root-reconciler, suspecting caching issue.
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
554ec98 to
063cf7a
Compare
The readiness probe on the otel-agent container was causing operational issues: - False alarms during slow cluster startup due to health check binding failures - Inconsistent with other containers (git-sync, reconciler) which don't use readiness probes - Redundant for a telemetry sidecar that doesn't provide direct user-facing services
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
929eba4 to
7c97762
Compare
tiffanny29631
changed the title
tiffanny29631
force-pushed
the
otel-agent-resource
branch
from
3fcb94d to
83dbb3e
Compare
|
@tiffanny29631: The following test failed, say
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. |
Remove the readiness probe and health check from otel-agent containers in reconciler, reconciler-manager, and resourcegroup controller to align container behavior. The healthcheck component can fail to bind to the port or respond under CPU throttling, causing unnecessary pod unready states even when the container is running. The otel-collector health check in the config-management-monitoring namespace is retained since it is tied to a Service.