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

use DeleteDatabaseAndAccountFinalizers #862

Conversation

zzzeek
Copy link
Contributor

@zzzeek zzzeek commented Sep 17, 2024

This switches the use of GetDatabaseAndAccount +
database.DeleteFinalizers() to use a new function in mariadb DeleteDatabaseAndAccountFinalizers, which looks up each individual component used for database access individually and removes finalizers from all of those that were found, regardless of other elements not
existing. All controllers should be changed to use this method to
ensure that early-created MariaDBAccount objects are removed from a
control plane where MariaDBDatabase objects did not get created before
the control plane were deleted, as well as similar scenarios.

relies upon openstack-k8s-operators/mariadb-operator#268

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/355719ef7f73466189f1f535b5b9c1e6

✔️ openstack-meta-content-provider SUCCESS in 2h 51m 15s
✔️ nova-operator-kuttl SUCCESS in 39m 08s
nova-operator-tempest-multinode FAILURE in 1h 16m 37s
nova-operator-tempest-multinode-ceph FAILURE in 1h 23m 18s

@zzzeek
Copy link
Contributor Author

zzzeek commented Sep 19, 2024

recheck

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/4594b1e8fd484d02aa6bb428df5ccad7

✔️ openstack-meta-content-provider SUCCESS in 1h 43m 06s
✔️ nova-operator-kuttl SUCCESS in 41m 26s
nova-operator-tempest-multinode FAILURE in 1h 16m 07s
nova-operator-tempest-multinode-ceph FAILURE in 1h 26m 39s

@auniyal61
Copy link
Contributor

auniyal61 commented Sep 27, 2024

this is a [DNM] patch. as its targeting a unmerged patch/build

because the depends-on mariadb patch did not merged yet here

okay its defined here

though DeleteDatabaseAndAccountFinalizers additionaly delete "unused" MariaDBAccounts, which seems like something do be done on restart/start, instead of individual req.
while DeleteFinalizer only for asked account.
but that is a part of mariadb-operator..

the change seems to be working fine here. but it is a [DNM] patch.

@zzzeek
Copy link
Contributor Author

zzzeek commented Sep 27, 2024

though DeleteDatabaseAndAccountFinalizers additionaly delete "unused" MariaDBAccounts, which seems like something do be done on restart/start, instead of individual req. while DeleteFinalizer only for asked account. but that is a part of mariadb-operator..

this is not new functionality in the mariadb operator, it was there previously, and it's to delete any other MariaDBAccounts that may have existed previously regarding the particular MariaDBDatabase that the nova operator here wants to ensure is gone. when an account rotation occurs, a new MariaDBAccount is created, and finalizers will be removed from the old MariaDBAccount, however it does not automatically delete the old MariaDBAccount. this is not any kind of change right now.

Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

Looks good to me. Lets land the mariadb-operator PR and then we can land this.

go.mod Outdated
@@ -85,6 +85,8 @@ require (

replace github.com/openstack-k8s-operators/nova-operator/api => ./api

replace github.com/openstack-k8s-operators/mariadb-operator/api => github.com/zzzeek/mariadb-operator/api v0.3.1-0.20240917193141-8598d28fb998 //allow-merging
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use allow-merging, let the github check fail, this way we won't merge the PR before the dependency really lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

i added hold until that is fixed feel free to drop it when its rebased

@gibizer
Copy link
Contributor

gibizer commented Oct 8, 2024

need a rebase and the go.mod replace line can be removed now as the mariadb-operator PR is landed.h

This switches the use of GetDatabaseAndAccount +
database.DeleteFinalizers() to use a new function in mariadb
DeleteDatabaseAndAccountFinalizers, which looks up each individual
component used for database access individually and removes finalizers
from all of those that were found, regardless of other elements not
existing.    All controllers should be changed to use this method to
ensure that early-created MariaDBAccount objects are removed from a
control plane where MariaDBDatabase objects did not get created before
the control plane were deleted, as well as similar scenarios.
Copy link
Contributor

@gibizer gibizer left a comment

Choose a reason for hiding this comment

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

looks good.

Copy link
Contributor

@SeanMooney SeanMooney left a comment

Choose a reason for hiding this comment

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

the depency is now merged and im ok with this change so approving

Copy link
Contributor

openshift-ci bot commented Oct 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, SeanMooney, zzzeek

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-merge-bot openshift-merge-bot bot merged commit 2b3d7d2 into openstack-k8s-operators:main Oct 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants