OCPBUGS-90710: Destroy all private cluster backend service resources#10643
OCPBUGS-90710: Destroy all private cluster backend service resources#10643barbacbd wants to merge 2 commits into
Conversation
This fixes a regression where finding instances by label was not working correctly. The labels field must be requested when querying for instance information.
openshift-ci-robot
added
jira/severity-important
|
@barbacbd: This pull request references Jira Issue OCPBUGS-43779, which is invalid:
Comment 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. |
Walkthrough
ChangesGCP Destroy Logic Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[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 |
barbacbd
changed the title
openshift-ci-robot
removed
the
jira/severity-important
|
@barbacbd: This pull request references Jira Issue OCPBUGS-90710, which is invalid:
Comment 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. |
|
/jira refresh |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-90710, which is invalid:
Comment 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. |
|
/jira refresh |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-90710, which is invalid:
Comment 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. |
|
/jira refresh |
|
@barbacbd: This pull request references Jira Issue OCPBUGS-90710, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@barbacbd: This pull request references Jira Issue OCPBUGS-90710, which is valid. 7 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. |
|
/cc @tthvo |
barbacbd
force-pushed
the
OCPBUGS-90710-release-4.17
branch
from
4fe72c0 to
062b048
Compare
|
/retest |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/retest |
![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/destroy/gcp/cloudcontroller.go`:
- Around line 199-204: Fix the nil pointer dereference in the
listHealthChecksWithFilter call by replacing the nil parameter with
o.healthCheckList, which is the global HealthChecks API object that the function
expects to use. Additionally, either add a case handler for the
"globalhealthcheck" typeName in the destroy loop to handle deletion of these
items when they are inserted via insertPendingItems, or change the typeName
string from "globalhealthcheck" to "healthcheck" to use the existing global
health check handler.
- Around line 43-66: The firewall list is being fetched separately for every
backend service with no backends (N+1 problem) and only returns the first page
due to using .Do() instead of handling pagination. Hoist the firewall list fetch
outside the loop that iterates through backend services so it is called once per
cloudcontroller operation. Replace the Firewalls.List().Do() call with
Firewalls.List().Pages() to properly handle pagination across all firewall
rules. Reuse the paginated firewall list across all backend service checks
instead of refetching for each item, and return the error instead of continuing
to avoid silently skipping cleanup when transient failures occur.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ad8130e-3e8d-45be-a9df-78b865153b2d
📒 Files selected for processing (2)
pkg/destroy/gcp/cloudcontroller.gopkg/destroy/gcp/instance.go
When a private cluster was created, the destroy process would not find and destroy backend services. This was happening specifically when no backends were created/found. Enhanced backend service discovery to check firewall rules for cluster ID tags when backends are empty. This allows proper identification of orphaned backend services that should be deleted during cluster destroy. Also added global health check discovery to ensure all related resources are properly cleaned up.
barbacbd
force-pushed
the
OCPBUGS-90710-release-4.17
branch
from
ecf4ac4 to
631c430
Compare
|
/retest-required |
|
@barbacbd: 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. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/destroy/gcp/cloudcontroller.go`:
- Around line 28-102: The function listCloudControllerBackendServices lacks unit
test coverage, which violates project coding guidelines. Add unit tests that
cover the three main filtering scenarios: (1) backend services with no backends
that are validated by checking if firewall targetTags contain the cluster ID,
(2) backend services with backends where all backend Groups match instance-group
URLs from the urls set, and (3) backend services that should be filtered out due
to mismatched backends or missing firewall tag matches. Ensure your tests mock
the compute service Firewalls.List and RegionBackendServices.List calls
appropriately.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b5f2b3c2-9312-4f66-bc11-1f66162a4027
📒 Files selected for processing (1)
pkg/destroy/gcp/cloudcontroller.go
|
/payload-job periodic-ci-openshift-release-main-ci-4.17-e2e-gcp-ovn |
|
@tthvo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/55e14d40-6f33-11f1-83b6-eb89d48c8156-0 |
3 participants
When a private cluster was created, the destroy process would not find
and destroy backend services. This was happening specifically when no
backends were created/found.
Summary by CodeRabbit