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

sync all resources to cluster fields #2713

Merged
merged 27 commits into from
Aug 13, 2024
Merged

sync all resources to cluster fields #2713

merged 27 commits into from
Aug 13, 2024

Conversation

FxKu
Copy link
Member

@FxKu FxKu commented Aug 2, 2024

While developing owner references support I realized once again that we store many child resources as cluster objects so we can access them even after the manifest is gone. But not for all resources, which might cause issues when deleting them because c.Namespace field might be empty:

time="2024-08-01T12:32:33Z" level=debug msg="removing leftover Patroni objects (endpoints / services and configmaps)" cluster-name=default/acid-clone-cluster pkg=cluster worker=2
time="2024-08-01T12:32:33Z" level=warning msg="could not fetch service \"/\": an empty namespace may not be set when a resource name is provided" cluster-name=default/acid-clone-cluster pkg=cluster worker=2

Fixing it for missing resources such as logical backup cron job and streams is easy. But it gets a bit tricky with objects created by Patroni, namely config service/endpoint and optionally configmaps. This PR introduces extra sync treatment for these, but only mapping to cluster fields. No diffs, no updates. This should allow us to loop over them on deletion.

Some more minor changes:

  • we don't need to init the clusters kubeResources with nil
  • bugfix: patched secrets and endpoints are now synced in cluster fields
  • Patroni endpoints are treated separately to master/replica endpoints (only config service reused existing code, hence the extra PostgresRole)
  • have only one function to determine the service/endpoint name
  • small refactoring of code when services/endpoints are already deleted
  • deleteSecret does not require passing the secret, we fetch it from cluster.Secrets map via uid
  • if second parameter from a range loop is not needed we don't need to write an underscore (idx, _ := range -> idx := range)

The streams feature saw some bigger refactoring:

  • createOrUpdateStreams was turned into syncStream for a single stream CRD which is more in line with the rest of the code
  • loop over appIds is now done in syncStreams as well as the deletion of streams that got removed from the manifest (and, thus, missing in appIds list)
  • renamed gatherApplicationIds function to more suitable name getDistinctApplicationIds
  • deleteStream does not require passing stream, we fetch it from cluster.Streams map via appId
  • the recently added check for existing slots (stream: slot and FES should not be created if the publication creation fails #2704) to allow FES creation is now comparing desired databaseSlots with actual slotsToSync. The previous comparison was not complete: appId loop with index lookup on c.Spec.Streams, which can be bigger, also with a different order.

fixes #2719

@FxKu FxKu added the minor label Aug 2, 2024
@FxKu FxKu added this to the 1.13.0 milestone Aug 2, 2024
@hughcapet
Copy link
Member

not true anymore?

# pvcs and Patroni config service/endpoint should not be affected by owner reference

(+ add Patroni objects to check_cluster_child_resources_owner_references() in e2e and add config service/endpoints and configmaps to the TestInheritedAnnotations unit test)

hughcapet
hughcapet previously approved these changes Aug 12, 2024
@hughcapet hughcapet self-requested a review August 12, 2024 16:20
Copy link
Member

@hughcapet hughcapet left a comment

Choose a reason for hiding this comment

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

comment in test_owner_references e2e test is still not adjusted (patroni resources are affected by owner reference)

pkg/cluster/sync.go Outdated Show resolved Hide resolved
pkg/cluster/util_test.go Outdated Show resolved Hide resolved
@FxKu
Copy link
Member Author

FxKu commented Aug 13, 2024

👍

2 similar comments
@hughcapet
Copy link
Member

👍

@hughcapet
Copy link
Member

👍

@FxKu FxKu merged commit 25ccc87 into master Aug 13, 2024
12 checks passed
@FxKu FxKu deleted the fix-orphaned-service-delete branch August 13, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Removing cluster do not removes objects from k8s
3 participants