migrate from databaseUsername to databaseAccount and fully use MariaDBAccount #681

zzzeek · 2024-02-12T22:45:39Z

This moves nova to fully use MariaDBAccount based on the dev work being done for mariadb-operator:

https://github.com/openstack-k8s-operators/mariadb-operator/pull/184/files

openshift-ci · 2024-02-12T22:45:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

softwarefactory-project-zuul · 2024-02-12T23:22:09Z

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://review.rdoproject.org/zuul/buildset/5f254123d77e48df99bfe3842ed6a787

❌ nova-operator-content-provider FAILURE in 6m 34s
⚠️ nova-operator-kuttl SKIPPED Skipped due to failed job nova-operator-content-provider
⚠️ nova-operator-tempest-multinode SKIPPED Skipped due to failed job nova-operator-content-provider

softwarefactory-project-zuul · 2024-02-13T14:50:13Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/openstack-operator for 656,b5e236beee28729c77e3e70b8b352f873c67e513

softwarefactory-project-zuul · 2024-02-13T16:25:34Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/openstack-operator for 656,b5e236beee28729c77e3e70b8b352f873c67e513

softwarefactory-project-zuul · 2024-02-13T22:32:16Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/openstack-operator for 656,b5e236beee28729c77e3e70b8b352f873c67e513

softwarefactory-project-zuul · 2024-02-14T19:18:12Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/nova-operator for 681,c4cf25a88bc848c8eac8e7a9ef1d587856af821f

zzzeek · 2024-02-14T22:08:29Z

/retest

softwarefactory-project-zuul · 2024-02-14T22:09:19Z

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://review.rdoproject.org/zuul/buildset/eed32e0dba6141ce8579094f069bd555

❌ nova-operator-content-provider FAILURE in 10m 26s
⚠️ nova-operator-kuttl SKIPPED Skipped due to failed job nova-operator-content-provider
⚠️ nova-operator-tempest-multinode SKIPPED Skipped due to failed job nova-operator-content-provider

softwarefactory-project-zuul · 2024-02-15T17:26:08Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/openstack-k8s-operators/openstack-operator for 656,def81966851fa35aea190d72515cefc590ef816d

zzzeek · 2024-02-15T17:34:04Z

/retest

softwarefactory-project-zuul · 2024-02-15T22:39:11Z

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://review.rdoproject.org/zuul/buildset/8f35d1ed9f4d445cbecc717f3653409d

✔️ nova-operator-content-provider SUCCESS in 2h 13m 13s
❌ nova-operator-kuttl RETRY_LIMIT in 26m 25s
✔️ nova-operator-tempest-multinode SUCCESS in 1h 49m 41s

zzzeek · 2024-02-16T13:49:34Z

recheck

softwarefactory-project-zuul · 2024-02-16T16:07:34Z

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://review.rdoproject.org/zuul/buildset/f126069545674ca080e7faa6ffd1fbe1

✔️ nova-operator-content-provider SUCCESS in 2h 15m 47s
❌ nova-operator-kuttl FAILURE in 41m 31s
✔️ nova-operator-tempest-multinode SUCCESS in 1h 53m 54s

controllers/nova_controller.go

zzzeek

this also needs GetDatabaseByName updated for the finalizer removal

softwarefactory-project-zuul · 2024-02-23T00:09:07Z

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://review.rdoproject.org/zuul/buildset/2ed79e0ce148433fae3b0ac7d647ccb0

✔️ nova-operator-content-provider SUCCESS in 2h 06m 28s
❌ nova-operator-kuttl FAILURE in 44m 26s
✔️ nova-operator-tempest-multinode SUCCESS in 1h 45m 39s

softwarefactory-project-zuul · 2024-03-04T23:25:16Z

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://review.rdoproject.org/zuul/buildset/ed6da443e61c4743b83a31492629a7d6

✔️ nova-operator-content-provider SUCCESS in 2h 09m 05s
❌ nova-operator-kuttl FAILURE in 41m 50s
✔️ nova-operator-tempest-multinode SUCCESS in 1h 50m 40s

gibizer

I think all my comments can be fixed in a follow up. So let me know if you want to do that and I will approve this.

gibizer · 2024-03-05T11:22:50Z

controllers/nova_controller.go

 		// cell.Spec.APIDatabaseHostname is empty for cells without APIDB access
 		"api_db_address":         apiDBHostname,
 		"api_db_port":            3306,
 		"cell_db_name":           getCellDatabaseName(cell.Spec.CellName),
-		"cell_db_user":           cell.Spec.CellDatabaseUser,
-		"cell_db_password":       string(ospSecret.Data[cellTemplate.PasswordSelectors.Database]),


now cellTemplate become unused so can be removed from the function params. Can be fixed n a followup

gibizer · 2024-03-05T11:56:08Z

controllers/nova_controller.go

+	apiDatabaseAccount, apiDbSecret, err := mariadbv1.GetAccountAndSecret(ctx, h, instance.Spec.APIDatabaseAccount, instance.Namespace)
+	if err != nil {
+		return nil, "", "", err
+	}
+
+	cellDatabaseAccount, cellDbSecret, err := mariadbv1.GetAccountAndSecret(ctx, h, cell.Spec.CellDatabaseAccount, instance.Namespace)
+	if err != nil {
+		return nil, "", "", err
+	}
+


we have the cellDB *mariadbv1.Database passed in and the caller could pass the apiDB similarly. So I think these queries are unnecessary, we could just access the accounts and secrets from the facades. Can be fixed in a followup

yes these should be calling from db.GetAccount() / db.GetSecret()

gibizer · 2024-03-05T12:00:30Z

controllers/nova_controller.go

+	// yet associated with any MariaDBDatabase.
+	_, _, err := mariadbv1.EnsureMariaDBAccount(
+		ctx, h, cellTemplate.CellDatabaseAccount,
+		instance.Namespace, false, "nova_"+cellName,


we have a helper for that getCellDatabaseName

gibizer · 2024-03-05T12:01:01Z

controllers/nova_controller.go

+
+	cellDB := mariadbv1.NewDatabaseForAccount(
+		cellTemplate.CellDatabaseInstance, // mariadb/galera service to target
+		"nova_"+cellName,                  // name used in CREATE DATABASE in mariadb


we have a helper for that getCellDatabaseName.

gibizer · 2024-03-05T12:10:14Z

controllers/nova_controller.go

 		cellSpec.APIDatabaseHostname = apiDB.GetDatabaseHostname()
-		cellSpec.APIDatabaseUser = instance.Spec.APIDatabaseUser
+		cellSpec.APIDatabaseAccount = instance.Spec.APIDatabaseAccount


OK we still use the hostname to signal if the api DB is accessible to the cell or not. Not super nice that we default the account in both case but that is not new, we did that with the APIDatabaseUser too. So I think it is good as is.

gibizer · 2024-03-05T12:19:52Z

test/functional/nova_controller_test.go

+		},
+	}
+
+	mariadbCellSuite.RunBasicSuite()


nice. Thanks for the generalized test cases

gibizer · 2024-03-05T12:23:06Z

test/functional/nova_multicell_test.go

+	BeforeEach(func() {
+		apiMariaDBAccount, apiMariaDBSecret := mariadb.CreateMariaDBAccountAndSecret(
+			novaNames.APIMariaDBDatabaseAccount, mariadbv1.MariaDBAccountSpec{})
+		DeferCleanup(k8sClient.Delete, ctx, apiMariaDBAccount)
+		DeferCleanup(k8sClient.Delete, ctx, apiMariaDBSecret)
+
+		cell0Account, cell0Secret := mariadb.CreateMariaDBAccountAndSecret(
+			cell0.MariaDBAccountName, mariadbv1.MariaDBAccountSpec{})
+		DeferCleanup(k8sClient.Delete, ctx, cell0Account)
+		DeferCleanup(k8sClient.Delete, ctx, cell0Secret)
+
+		cell1Account, cell1Secret := mariadb.CreateMariaDBAccountAndSecret(
+			cell1.MariaDBAccountName, mariadbv1.MariaDBAccountSpec{})
+		DeferCleanup(k8sClient.Delete, ctx, cell1Account)
+		DeferCleanup(k8sClient.Delete, ctx, cell1Secret)
+
+		cell2Account, cell2Secret := mariadb.CreateMariaDBAccountAndSecret(
+			cell2.MariaDBAccountName, mariadbv1.MariaDBAccountSpec{})
+		DeferCleanup(k8sClient.Delete, ctx, cell2Account)
+		DeferCleanup(k8sClient.Delete, ctx, cell2Secret)
+
+	})


Is it intentional that we pre-create the accounts in these tests? As we create Nova CR and not some subCR in this tests we could let the nova controller's ensure call do the creation. Of course it is valid to pre-create them to simulate that it is done by the openstack-operator or the user.

If this is intentional then please leave a comment in the code nothing it as this is now different from the nova_controller_test.go and that might confuse the future reader

if we move to openstack-operator being responsible for mariadbaccount creation then we'd be glad to have these here. I have a feeling that EnsureMariaDBAccount is going to be generating username/passwords for a long time though because it's very convenient that it takes care of itself.

given the discussion at openstack-k8s-operators/mariadb-operator#205 (review) ~~I think I want to add Eventually() to the RunURLAssertSuites here~~ before we merge and I can try removing some of these extraneous account creates. Between the many test suites here as well as all the changes made by @stuggi there are likely more mariadbaccoutn creates than needed.

update: I am not using any RunURLAssertSuites. so we can merge this as is and address things in followup, given how much effort it is for me to keep rebasing this each time someone changes something.

gibizer · 2024-03-05T12:25:55Z

test/functional/nova_reconfiguration_test.go

@@ -50,22 +51,42 @@ func CreateNovaWith3CellsAndEnsureReady(novaNames NovaNames) {
 	DeferCleanup(mariadb.DeleteDBService, mariadb.CreateDBService(cell1.MariaDBDatabaseName.Namespace, cell1.MariaDBDatabaseName.Name, serviceSpec))
 	DeferCleanup(mariadb.DeleteDBService, mariadb.CreateDBService(cell2.MariaDBDatabaseName.Namespace, cell2.MariaDBDatabaseName.Name, serviceSpec))

+	apiMariaDBAccount, apiMariaDBSecret := mariadb.CreateMariaDBAccountAndSecret(


ditto, we could let the nova controller create these. If this is intentional then please leave a comment

gibizer

I deployed this and did a cell1 password rotation. Then I checked that the new user / pass was added to the cell mapping table automatically and I could deploy an instance to celll1 too.

the open comments can be fixed in a followup

openshift-ci · 2024-03-05T16:07:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, 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:

~~OWNERS~~ [gibizer]

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

openshift-ci bot added the do-not-merge/work-in-progress label Feb 12, 2024

zzzeek requested a review from gibizer February 12, 2024 22:45

zzzeek force-pushed the integrate_oo_accounts branch from 9a39fa2 to 5bd427a Compare February 13, 2024 14:50

zzzeek force-pushed the integrate_oo_accounts branch from 5bd427a to 5823996 Compare February 13, 2024 16:25

zzzeek force-pushed the integrate_oo_accounts branch from 5823996 to fb5c036 Compare February 13, 2024 22:32

openshift-merge-robot added the needs-rebase label Feb 14, 2024

zzzeek force-pushed the integrate_oo_accounts branch from fb5c036 to c4cf25a Compare February 14, 2024 19:17

zzzeek force-pushed the integrate_oo_accounts branch from c4cf25a to 8534cd9 Compare February 14, 2024 21:56

openshift-merge-robot removed the needs-rebase label Feb 14, 2024

zzzeek marked this pull request as ready for review February 14, 2024 22:30

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 14, 2024

openshift-ci bot requested review from lewisdenny and viroel February 14, 2024 22:30

zzzeek force-pushed the integrate_oo_accounts branch from 8534cd9 to 9f602d6 Compare February 15, 2024 17:26

zzzeek force-pushed the integrate_oo_accounts branch 2 times, most recently from 38cfe99 to ca814eb Compare February 15, 2024 20:24

zzzeek force-pushed the integrate_oo_accounts branch from ca814eb to 0a263f2 Compare February 19, 2024 20:49

zzzeek force-pushed the integrate_oo_accounts branch from 9637d30 to cc65abf Compare February 20, 2024 04:45

zzzeek changed the title ~~migrate from databaseUsername to databaseAccount~~ migrate from databaseUsername to databaseAccount and fully use MariaDBAccount Feb 20, 2024

zzzeek commented Feb 20, 2024

View reviewed changes

controllers/nova_controller.go Show resolved Hide resolved

zzzeek commented Feb 20, 2024

View reviewed changes

zzzeek force-pushed the integrate_oo_accounts branch 4 times, most recently from a3bfc8c to 83a3645 Compare February 22, 2024 22:01

zzzeek force-pushed the integrate_oo_accounts branch 7 times, most recently from 6b00758 to b4c144a Compare February 26, 2024 22:59

openshift-merge-robot added the needs-rebase label Feb 28, 2024

mrkisaolamb mentioned this pull request Feb 29, 2024

Remove DatabaseUser configurability #391

Closed

zzzeek force-pushed the integrate_oo_accounts branch from b4c144a to 245bd98 Compare March 4, 2024 21:14

openshift-merge-robot removed the needs-rebase label Mar 4, 2024

migrate from databaseUsername to databaseAccount

8c8b041

zzzeek force-pushed the integrate_oo_accounts branch from 245bd98 to 8c8b041 Compare March 4, 2024 23:31

gibizer reviewed Mar 5, 2024

View reviewed changes

gibizer approved these changes Mar 5, 2024

View reviewed changes

openshift-ci bot assigned gibizer Mar 5, 2024

openshift-ci bot added the lgtm label Mar 5, 2024

openshift-ci bot added the approved label Mar 5, 2024

openshift-merge-bot bot merged commit 90a97e1 into openstack-k8s-operators:main Mar 5, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate from databaseUsername to databaseAccount and fully use MariaDBAccount #681

migrate from databaseUsername to databaseAccount and fully use MariaDBAccount #681

zzzeek commented Feb 12, 2024 •

edited

Loading

openshift-ci bot commented Feb 12, 2024

softwarefactory-project-zuul bot commented Feb 12, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 14, 2024

zzzeek commented Feb 14, 2024

softwarefactory-project-zuul bot commented Feb 14, 2024

softwarefactory-project-zuul bot commented Feb 15, 2024

zzzeek commented Feb 15, 2024

softwarefactory-project-zuul bot commented Feb 15, 2024

zzzeek commented Feb 16, 2024

softwarefactory-project-zuul bot commented Feb 16, 2024

zzzeek left a comment

softwarefactory-project-zuul bot commented Feb 23, 2024

softwarefactory-project-zuul bot commented Mar 4, 2024

gibizer left a comment

gibizer Mar 5, 2024

gibizer Mar 5, 2024

zzzeek Mar 5, 2024

gibizer Mar 5, 2024

gibizer Mar 5, 2024

gibizer Mar 5, 2024

gibizer Mar 5, 2024

gibizer Mar 5, 2024

zzzeek Mar 5, 2024 •

edited

Loading

zzzeek Mar 5, 2024

gibizer Mar 5, 2024

gibizer left a comment

openshift-ci bot commented Mar 5, 2024

migrate from databaseUsername to databaseAccount and fully use MariaDBAccount #681

migrate from databaseUsername to databaseAccount and fully use MariaDBAccount #681

Conversation

zzzeek commented Feb 12, 2024 • edited Loading

openshift-ci bot commented Feb 12, 2024

softwarefactory-project-zuul bot commented Feb 12, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 13, 2024

softwarefactory-project-zuul bot commented Feb 14, 2024

zzzeek commented Feb 14, 2024

softwarefactory-project-zuul bot commented Feb 14, 2024

softwarefactory-project-zuul bot commented Feb 15, 2024

zzzeek commented Feb 15, 2024

softwarefactory-project-zuul bot commented Feb 15, 2024

zzzeek commented Feb 16, 2024

softwarefactory-project-zuul bot commented Feb 16, 2024

zzzeek left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Feb 23, 2024

softwarefactory-project-zuul bot commented Mar 4, 2024

gibizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzzeek Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibizer left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 5, 2024

zzzeek commented Feb 12, 2024 •

edited

Loading

zzzeek Mar 5, 2024 •

edited

Loading