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.

Sign up for GitHub

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

Jump to bottom

✨ Downstream Namespace cleanup: ensure namespace is empty and delete after a grace period #2299

Merged
merged 4 commits into from
Nov 14, 2022
Merged

✨ Downstream Namespace cleanup: ensure namespace is empty and delete after a grace period #2299

merged 4 commits into from
Nov 14, 2022

Conversation

jmprusi
Copy link
Member

@jmprusi jmprusi commented Nov 3, 2022 •
edited by davidfestal

Summary

Improve downstream namespace deletion:

  • Make sure the namespace is empty before starting deletion
  • Delay deletion of a namespace just in case the syncer can recover.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2022
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch 2 times, most recently from 167f4a0 to c0bb5f9 Compare November 4, 2022 17:28
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2022
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch 2 times, most recently from 8f269a9 to 830db5c Compare November 4, 2022 18:19
@jmprusi jmprusi changed the title ✨ WIP- Downstream Namespace cleanup: ensure empty and delay deletion Downstream Namespace cleanup: ensure namespace is empty and delete after a grace period Nov 4, 2022
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch from 830db5c to 6713b3c Compare November 5, 2022 01:44
@jmprusi jmprusi changed the title Downstream Namespace cleanup: ensure namespace is empty and delete after a grace period ✨ Downstream Namespace cleanup: ensure namespace is empty and delete after a grace period Nov 5, 2022
davidfestal
davidfestal reviewed Nov 7, 2022
View reviewed changes
pkg/syncer/namespace/namespace_downstream_process.go Outdated Show resolved Hide resolved
pkg/syncer/namespace/namespace_downstream_process.go Outdated Show resolved Hide resolved
pkg/syncer/resourcesync/controller.go Outdated
return nil, err
}

return getAllGVRs(syncTarget), nil
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you simply return the keys of the controller.syncerInformerMap ?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@davidfestal
Copy link
Member

@jmprusi Could you run make imports ?

@davidfestal
Copy link
Member

/retest

@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch 4 times, most recently from 1596a84 to 1391451 Compare November 10, 2022 17:33
@davidfestal
Copy link
Member

/retest

@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch from 1391451 to b75624a Compare November 14, 2022 10:05
davidfestal
davidfestal reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Did you make the grace period configurable finally ?

pkg/syncer/namespace/namespace_downstream_process.go Outdated Show resolved Hide resolved
pkg/syncer/namespace/namespace_downstream_process.go Outdated Show resolved Hide resolved
pkg/syncer/resourcesync/controller.go Outdated Show resolved Hide resolved
pkg/syncer/shared/cleaner.go Show resolved Hide resolved
pkg/syncer/shared/cleaner.go Outdated Show resolved Hide resolved
pkg/syncer/shared/cleaner.go Outdated Show resolved Hide resolved
pkg/syncer/shared/cleaner.go Outdated Show resolved Hide resolved
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch 2 times, most recently from c8747fc to ca9580f Compare November 14, 2022 13:13
@jmprusi
syncer/namespace: improve namespace deletion
455d377 
* Ensure downstream namespaces are empty
* Delay deletion of downstream namespaces to tolerate disconnections.
@jmprusi
syncer/spec: wire namespace cleaner to syncer spec
d5cbd2b
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch from ca9580f to a00ed0b Compare November 14, 2022 13:29
@jmprusi
Copy link
Member Author

jmprusi commented Nov 14, 2022

Did you make the grace period configurable finally ?

done

davidfestal
davidfestal reviewed Nov 14, 2022
View reviewed changes
pkg/syncer/shared/cleaner.go Outdated Show resolved Hide resolved
pkg/syncer/shared/cleaner.go Outdated Show resolved Hide resolved
cmd/syncer/options/options.go Outdated Show resolved Hide resolved
@jmprusi jmprusi force-pushed the jmprusi/namespace-cleanup-pending-resources branch from 324ca90 to b7775c0 Compare November 14, 2022 15:12
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 14, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidfestal

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 Nov 14, 2022
@openshift-merge-robot openshift-merge-robot merged commit 5c6814d into kcp-dev:main Nov 14, 2022
@davidfestal davidfestal mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@davidfestal davidfestal davidfestal approved these changes

@sttts sttts Awaiting requested review from sttts

Assignees

@davidfestal davidfestal

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jmprusi @davidfestal @stevekuznetsov @openshift-merge-robot