Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reconciler/workload/resource: remove syncer finalizer when SyncTarget is gone #1293

Merged

Conversation

sttts
Copy link
Member

@sttts sttts commented Jun 20, 2022

When a SyncTarget is deleted, the syncer loses the ability to remove its finalizer from objects because the VW will not let it see its objects. This PR will make the workload resource controller do this by checking existence of the SyncTargets.

Fixes #1353

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2022
@openshift-ci openshift-ci bot requested review from ncdc and stevekuznetsov June 20, 2022 06:45
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@sttts: PR needs rebase.

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.

@ncdc
Copy link
Member

ncdc commented Jun 24, 2022

I guess this is related to #1353 that I just filed

@sttts sttts force-pushed the sttts-cleanup-syncer-finalizer branch from f01393e to 9ee92ff Compare August 1, 2022 16:32
@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022
@sttts sttts changed the title WIP: reconciler/workload/resource: remove syncer finalizer when WorkloadCluster is gone WIP: reconciler/workload/resource: remove syncer finalizer when SyncTarget is gone Aug 1, 2022
@sttts sttts force-pushed the sttts-cleanup-syncer-finalizer branch from 9ee92ff to c65c674 Compare August 1, 2022 16:37
SyncTargetsBySyncTargetKey = "SyncTargetsBySyncTargetKey"
)

func IndexSyncTargetsBySyncTargetKey(obj interface{}) ([]string, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncdc is our plan to move all indexers here? I think I would like that.

@sttts sttts force-pushed the sttts-cleanup-syncer-finalizer branch from c65c674 to 654a4b7 Compare August 1, 2022 20:21
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 1, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2022
@sttts sttts force-pushed the sttts-cleanup-syncer-finalizer branch from 654a4b7 to f5d722a Compare August 1, 2022 20:46
@sttts sttts changed the title WIP: reconciler/workload/resource: remove syncer finalizer when SyncTarget is gone reconciler/workload/resource: remove syncer finalizer when SyncTarget is gone Aug 1, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2022
@sttts sttts force-pushed the sttts-cleanup-syncer-finalizer branch from f5d722a to 1b6bfe5 Compare August 1, 2022 21:05
@sttts
Copy link
Member Author

sttts commented Aug 2, 2022

/retest

@davidfestal
Copy link
Member

FTR this would be done by the Virtual Workspace in the context of the 1st Syncer Virtual Workspace Transformation PR, which manages the deletion process previously managed by the Syncer, and also the cleanup of related annotations / labels.

@jmprusi
Copy link
Member

jmprusi commented Aug 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2022
@sttts
Copy link
Member Author

sttts commented Aug 2, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit 8cd6caa into kcp-dev:main Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some workspaces stuck in permanent failure to delete
5 participants