OCPBUGS-100189: Patch GCP Load Balancer Health Checks - #10731
OCPBUGS-100189: Patch GCP Load Balancer Health Checks#10731patrickdillon wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-100189, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesGCP health-check updates
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-gcp-ovn |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-100189, which is valid. 3 validation(s) were run on this bug
DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/infrastructure/gcp/clusterapi/healthcheck.go`:
- Around line 37-76: Add unit tests covering updateHealthChecks orchestration
and public/private target selection in
pkg/infrastructure/gcp/clusterapi/healthcheck.go:37-76, retry and cancellation
behavior plus terminal failures in
pkg/infrastructure/gcp/clusterapi/healthcheck.go:78-169, and PostProvision error
propagation in pkg/infrastructure/gcp/clusterapi/clusterapi.go:311-318. Use
mocked compute-service interactions to verify global/regional PATCH retries and
interruption outcomes.
- Around line 108-112: Update both retry-error branches in healthcheck.go at
lines 108-112 and 147-151: in the wait.Interrupted handling, wrap waitErr as the
primary error so cancellations and deadlines retain their correct identity, and
include lastErr only as supplemental context when available. Preserve direct
return of non-interrupted waitErr errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1375355c-7993-4168-aee0-9b09260228b5
📒 Files selected for processing (2)
pkg/infrastructure/gcp/clusterapi/clusterapi.gopkg/infrastructure/gcp/clusterapi/healthcheck.go
|
/test verify-vendor |
patrickdillon
changed the title
This commit adds a hook to patch the default load balancer health check values populated by CAPG in order to match the values used by Terraform. The non-configurable CAPG defaults are very lax & this can cause failures during bootstrapping, because the API server on a node frequently goes offline as it bootstraps. The lax health checks means the offline node will stay in the pool and the LB will continue to serve connections to the offline node. As pods try to connect to the API server and fail because the request is sent to an offline API server, the pod may throw an error and fall into CrashLoopBackoff. Instead, the offline API server should be pulled from the LB pool very quickly so that traffic continues to be routed to the bootstrap API server, which stays available throughout bootstrapping. The preferred method for handling this is through CAPG: kubernetes-sigs/cluster-api-provider-gcp#1732 proposes an API for the health checks. Once that merges, we can remove this hook, but this hook is valuable for immediate gain as well as backporting to previous versions, where we might not be able to bump CAPG.
6d5c0d6 to
9380896
Compare
|
/test e2e-gcp-ovn |
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-100189, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| ctx, cancel := context.WithTimeout(ctx, time.Minute*3) | ||
| defer cancel() | ||
|
|
||
| op, err := svc.HealthChecks.Patch(projectID, hcName, hcPatch).Context(ctx).Do() |
There was a problem hiding this comment.
I wonder if a retry pattern is needed here? what happens if CAPG hasn't finished creating the health check resources by the time PostProvision runs? it will fail with 404.
I see iam.go uses a retry loop with back-off
There was a problem hiding this comment.
I am wondering about this exact same thing: how do we make sure these calls are resilient?
The original implementation wrapped the call in an exponential backoff, and it failed because the implementation was buggy. It seemed like that was overly complex: the current implementation uses relatively simple calls to send the operator to GCP, which should process them, handle issues server side, and only return an error when it is a legitimate failure. Is that enough resiliency? I think so but I'm not sure.
Specifically the CAPG health checks are guaranteed (supposedly) to be created by the InfrastructureReady check. This call only runs after InfrastructureReady, in which case CAPG has reconciled the load balancer.
In the IAM case, we are creating those resources directly with the SDK (not created by CAPG). And IAM is notoriously slow to be eventually consistent.
I am very open to the idea of adding some resiliency, but I'm leaning toward testing out how resilient the current implementation is in CI...
3 participants
This commit adds a hook to patch the default load balancer health check values populated by CAPG in order to match the values used by Terraform. The non-configurable CAPG defaults are very lax & this can cause failures during bootstrapping, because the API server on a node frequently goes offline as it bootstraps. The lax health checks means the offline node will stay in the pool and the LB will continue to serve connections to the offline node.
As pods try to connect to the API server and fail because the request is sent to an offline API server, the pod may throw an error and fall into CrashLoopBackoff.
Instead, the offline API server should be pulled from the LB pool very quickly so that traffic continues to be routed to the bootstrap API server, which stays available throughout bootstrapping.
The preferred method for handling this is through CAPG:
kubernetes-sigs/cluster-api-provider-gcp#1732
proposes an API for the health checks. Once that merges, we can remove this hook, but this hook is valuable for immediate gain as well as backporting to previous versions, where we might not be able to bump CAPG.
Summary by CodeRabbit
New Features
Bug Fixes