Skip to content

resourceactuation: halt delete operations when actuation-mode is Paused - #13654

Open
hopper-coder-bot wants to merge 1 commit into
GoogleCloudPlatform:masterfrom
hopper-coder-bot:issue-13650-1790925252
Open

hopper-coder-bot wants to merge 1 commit into
GoogleCloudPlatform:masterfrom
hopper-coder-bot:issue-13650-1790925252

Conversation

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator

Summary

This PR updates ShouldSkipActuation so that setting cnrm.cloud.google.com/actuation-mode: "Paused" halts delete operations as well as create/update/drift operations on individual resources.

Details & Rationale

  1. pkg/controller/resourceactuation/resourceactuation.go:

    • Removed the if isDeleting bypass for opv1beta1.Paused. Now, when an individual resource is annotated with cnrm.cloud.google.com/actuation-mode: "Paused", deletion operations against GCP are skipped, retaining finalizers and leaving the resource in Terminating state until unpaused.
    • Updated documentation comment to reflect that resource-level Paused skips Create/Update/Drift and Deletion operations.
  2. pkg/controller/predicate/predicate.go:

    • Added k8s.ActuationModeAnnotation checks to UnderlyingResourceOutOfSyncPredicate.Update so that modifying or removing the actuation mode annotation immediately enqueues the resource for reconciliation (e.g. to resume deletion when unpaused).
    • Added unit tests in pkg/controller/predicate/predicate_test.go.
  3. Testing:

    • Updated unit tests in pkg/controller/resourceactuation/resourceactuation_test.go.
    • Added E2E scenario test in tests/e2e/testdata/scenarios/resource_pause_delete/ with golden logs and verified behavior against mock GCP.

Fixes #13650

Halt delete operations when `cnrm.cloud.google.com/actuation-mode` is set to `Paused`.

This PR was generated by the overseer agent (powered by the gemini-3.7-flash model).

@google-oss-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign barney-s for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started investigating CI check failures for this pull request.

Note: We recommend waiting for the 'ready-for-human' label before leaving review comments. Comments added while the system is actively working may be associated with outdated commits once a new commit is pushed, causing them to be ignored.

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

Investigating tests-e2e-fixtures-container-1-of-2 failure

Run: 36983412444
Name: tests-e2e-fixtures-container-1-of-2
Cause: Flake
Details: The test failed during containercluster-minimal test initialization with error starting test environment: unable to start control plane itself: failed to start the controlplane. retried 5 times: fork/exec /home/runner/work/k8s-config-connector/k8s-config-connector/.build/envtest-bin/etcd: text file busy. This is a transient OS-level file lock / ETXTBSY issue when starting the envtest etcd binary under parallel test execution and is unrelated to the changes in this PR.
Action Taken: Rerun triggered for failed jobs via gh run rerun 36983412444 --failed.

(This report was generated by overseer)

@maqiuyujoyce

Copy link
Copy Markdown
Collaborator

Thanks for working on this! A couple of quick suggestions:

1. Run Scenario Test Against Real GCP

Please record the scenario golden files against real GCP first, then verify Mock GCP alignment:

./hack/record-gcp TestE2EScript/scenarios/resource_pause_delete

Also please add a brief note in script.yaml clarifying GET calls in _http01.log & _http02.log for future readers.

2. Add Predicate Unit Tests for Unpausing via Removal

Please add test cases in [pkg/controller/predicate/predicate_test.go] covering:

  1. Annotation removed (old has "Paused", new has {} / absent) -> wantEvent: true.
  2. Changed from Reconciling to Paused -> wantEvent: true.
@maqiuyujoyce
maqiuyujoyce requested a review from acpana October 2, 2026 20:24
@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started addressing review feedback for this pull request.

- Remove the isDeleting bypass for opv1beta1.Paused in ShouldSkipActuation so that resource-level pause halts deletion as well.
- Trigger reconciliations on changes to cnrm.cloud.google.com/actuation-mode in UnderlyingResourceOutOfSyncPredicate.
- Update unit tests in resourceactuation_test.go and add unit test in predicate_test.go.
- Add an end-to-end scenario test in tests/e2e/testdata/scenarios/resource_pause_delete/.

Fix: 13650
@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

Thanks for the review! I have addressed your suggestions in the latest commit:

  1. Real GCP Scenario Recording & Alignment:

    • Recorded scenario golden logs against real GCP using ./hack/record-gcp TestE2EScript/scenarios/resource_pause_delete.
    • Added normalization for .resourceName in mockgcp/mocklogging/normalize.go and verified 1:1 alignment with Mock GCP.
    • Added explanatory notes to tests/e2e/testdata/scenarios/resource_pause_delete/script.yaml clarifying that the GET calls in _http01.log and _http02.log are generated by the scenario test harness exporting the live GCP state to verify the resource is retained while paused.
  2. Predicate Unit Tests:

    • Added unit test cases in pkg/controller/predicate/predicate_test.go verifying wantEvent: true when:
      • The cnrm.cloud.google.com/actuation-mode annotation is removed (Paused -> absent).
      • The annotation is changed from Reconciling to Paused.

(This comment was generated by overseer)

@argus-watcher-bot

Copy link
Copy Markdown
Collaborator

🤖 AI Factory started investigating CI check failures for this pull request.

Note: We recommend waiting for the 'ready-for-human' label before leaving review comments. Comments added while the system is actively working may be associated with outdated commits once a new commit is pushed, causing them to be ignored.

@hopper-coder-bot

Copy link
Copy Markdown
Collaborator Author

Investigating tests-e2e-fixtures-container-1-of-2 failure

Run: 37068660821
Name: tests-e2e-fixtures-container-1-of-2
Cause: Flake
Details: The job failed during test execution for pkg/test/resourcefixture/testdata/directives/removedefaultnodepool with /home/runner/work/k8s-config-connector/k8s-config-connector/.build/envtest-bin/etcd: Text file busy. This is a transient OS file lock / ETXTBSY issue when starting the envtest etcd binary under parallel test execution and is unrelated to the changes in this PR.
Action Taken: Rerun triggered for failed jobs via gh run rerun 37068660821 --failed.

Investigating tests-e2e-fixtures-iam failure

Run: 37068660821
Name: tests-e2e-fixtures-iam
Cause: Flake
Details: The job failed during test execution for pkg/test/resourcefixture/testdata/basic/iam/v1beta1/iamworkforcepoolprovider/samlworkforcepoolprovider with /home/runner/work/k8s-config-connector/k8s-config-connector/.build/envtest-bin/kube-apiserver: Text file busy. This is a transient OS file lock / ETXTBSY issue when starting the envtest kube-apiserver binary under parallel test execution and is unrelated to the changes in this PR.
Action Taken: Rerun triggered for failed jobs via gh run rerun 37068660821 --failed.

(This report was generated by overseer)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment