Dedicated container for database #482

nadvornik · 2024-10-30T16:36:55Z

What does this PR change?

Current state:

Works with the image from uyuni-project/uyuni#9469
mgradm install podman --pgsql-image <image> --pgsql-tag <tag>
Uses workarounds for the issues discussed in https://github.com/SUSE/spacewalk/issues/25363
Missing kubernetes.

Test coverage

No tests: add explanation
No tests: already covered
Unit tests were added
DONE

Links

Issue(s): #

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Before you merge

Check How to branch and merge properly!

aaannz

Couple comments, mostly typos.

Once this is merged, smdba autotune call can be also removed from mgr-setup and others.

aaannz · 2025-01-22T16:31:43Z

mgradm/shared/pgsql/pgsql.go

+) error {
+	image := pgsqlFlags.Image
+	currentReplicas := systemd.CurrentReplicaCount(podman.PgsqlService)
+	log.Debug().Msgf("Current HUB replicas running are %d.", currentReplicas)


Suggested change

log.Debug().Msgf("Current HUB replicas running are %d.", currentReplicas)

log.Debug().Msgf("Current pgsql replicas running are %d.", currentReplicas)

aaannz · 2025-01-22T16:32:28Z

mgradm/shared/pgsql/pgsql.go

+	log.Debug().Msgf("Current HUB replicas running are %d.", currentReplicas)
+
+	if pgsqlFlags.Replicas == 0 {
+		log.Debug().Msg("No pgsql requested.")


Suggested change

log.Debug().Msg("No pgsql requested.")

log.Info().Msg(L("No Postgres container requested."))

aaannz · 2025-01-22T16:33:01Z

mgradm/shared/pgsql/pgsql.go

+		log.Debug().Msg("No pgsql requested.")
+	}
+	if !pgsqlFlags.IsChanged {
+		log.Info().Msgf(L("No changes requested for hub. Keep %d replicas."), currentReplicas)


Suggested change

log.Info().Msgf(L("No changes requested for hub. Keep %d replicas."), currentReplicas)

log.Info().Msgf(L("No changes requested for pgsql. Keep %d replicas."), currentReplicas)

aaannz · 2025-01-22T16:34:01Z

mgradm/shared/pgsql/pgsql.go

+	if err := cnx.WaitForHealthcheck(); err != nil {
+		return err
+	}
+	// Now the servisce is up and ready, the admin credentials are no longer needed


Suggested change

// Now the servisce is up and ready, the admin credentials are no longer needed

// Now the service is up and ready, the admin credentials are no longer needed

aaannz · 2025-01-22T16:34:12Z

mgradm/shared/pgsql/pgsql.go

@@ -0,0 +1,191 @@
+// SPDX-FileCopyrightText: 2024 SUSE LLC


Suggested change

// SPDX-FileCopyrightText: 2024 SUSE LLC

// SPDX-FileCopyrightText: 2025 SUSE LLC

aaannz · 2025-01-22T16:43:09Z

mgradm/shared/templates/pgsqlVersionUpgradeScriptTemplate.go

 OLD_VERSION={{ .OldVersion }}
 NEW_VERSION={{ .NewVersion }}
-FAST_UPGRADE=--link
+FAST_UPGRADE= #--link


Not needed anymore

aaannz · 2025-01-22T16:44:18Z

mgradm/shared/utils/cmd_utils.go

@@ -209,3 +209,21 @@ Leave it unset if you want to keep the previous number of replicas.
 	_ = utils.AddFlagToHelpGroupID(cmd, "saline-replicas", "saline-container")
 	_ = utils.AddFlagToHelpGroupID(cmd, "saline-port", "saline-container")
 }
+
+// AddPgsqlFlags adds hub XML-RPC related parameters to cmd.


Suggested change

// AddPgsqlFlags adds hub XML-RPC related parameters to cmd.

// AddPgsqlFlags adds postgres related parameters to cmd.

aaannz · 2025-01-22T16:44:33Z

mgradm/shared/utils/cmd_utils.go

+	_ = utils.AddFlagToHelpGroupID(cmd, "pgsql-replicas", "pgsql-container")
+}
+
+// AddUpgradePgsqlFlags adds hub XML-RPC related parameters to cmd upgrade.


Suggested change

// AddUpgradePgsqlFlags adds hub XML-RPC related parameters to cmd upgrade.

// AddUpgradePgsqlFlags adds postgres related parameters to cmd upgrade.

rjmateus

overall looks good, just a few questions on this

mgradm/cmd/install/podman/podman_test.go

rjmateus · 2025-01-31T11:36:04Z

mgradm/shared/pgsql/pgsql.go

+	if err := EnablePgsql(systemd, 0); err != nil {
+		return err
+	}
+	if err := EnablePgsql(systemd, pgsqlFlags.Replicas); err != nil {


Could you explain why first enable with 0 replicas and then with the correct number of replicas?

rjmateus · 2025-01-31T11:36:58Z

mgradm/shared/pgsql/pgsql.go

+// This function is meant for installation or migration, to enable or disable the service after, use ScaleService.
+func EnablePgsql(systemd podman.Systemd, replicas int) error {
+	if replicas > 1 {
+		log.Warn().Msg(L("Multiple Hub XML-RPC container replicas are not currently supported, setting up only one."))


Suggested change

log.Warn().Msg(L("Multiple Hub XML-RPC container replicas are not currently supported, setting up only one."))

log.Warn().Msg(L("Multiple Pgsql container replicas are not supported, setting up only one."))

rjmateus · 2025-01-31T11:40:11Z

mgradm/shared/pgsql/pgsql.go

+	if err != nil {
+		return err
+	}
+	err = podman.RunContainer("uyuni-db-migrate", pgsqlImage, utils.PgsqlRequiredVolumeMounts, []string{},


The updade command also deals with major version upgrade? I think that at version 5.0 Michele prepared the upgrade process with the upgrade container to allow major upgrades where the database version would. Is this taken into consideration?

rjmateus · 2025-01-31T11:43:37Z

mgradm/shared/pgsql/pgsql.go

+		Image:      image,
+	}
+	if err := utils.WriteTemplateToFile(
+		pgsqlData, podman.GetServicePath(podman.PgsqlService+"@"), 0555, true,


It uses the replica mechanism with 0 or 1. In this case would make sense to have it or should we keep more simple and generate or not the systems configuration. We can allow remote database in the future, but as for first version on 5.1 it would be possible.
Also, in the future when we support remote database and user wants to migrate to there, it would be a documented manual step, since it would have a lot involved.

rjmateus · 2025-01-31T11:45:21Z

mgradm/shared/podman/podman.go

+		user,
+		false,
+		prepare,
+		"uyuni-pgsql-server.mgr.internal",


In future we may allow to use external database. for now it should work, But I saw somewhere in the code that users can specify the dbhostname. We should limit that or adapt this part of the code. At this stage we can have it flexible, but users should not be allowed to define an external database fqdn.

I don't agree: this was already allowed for Amazon databases in 4.3 and we have to keep at least this around. Preventing users from setting an external DB FQDN would be a regression... Also remember that the product supported by SUSE is not the only one around: why preventing Uyuni users to use this?

rjmateus · 2025-01-31T12:08:53Z

mgradm/shared/templates/pgsqlFinalizeScriptTemplate.go

-
-{{ if .RunAutotune }}
-echo "Running smdba system-check autotuning..."
-smdba system-check autotuning


will we stop running the autotuning on install and upgrade? We would need a new mechanism or notify the user.

This is in the postgres container uyuni-project/uyuni#9469

rjmateus · 2025-01-31T12:14:41Z

shared/testutils/flagstests/mgradm.go

+var PgsqlFlagsTestArgs = []string{
+	"--pgsql-image", "pgsqlimg",
+	"--pgsql-tag", "pgsqltag",
+	"--pgsql-replicas", "0",


I'm not sure if we want to expose the replicas in here and at this stage. We may use internally, but for now users should always use pgsql container active. When we have the external database support we may add a parameter that make it clear, like --external-db which ideally would not deploy the database container (with external database have a service to start the containerized DB can be confusing)

rjmateus · 2025-01-31T12:15:52Z

shared/utils/volumes.go

-// VarPgsqlVolumeMount defines the /var/lib/pgsql volume mount.
-var VarPgsqlVolumeMount = types.VolumeMount{MountPath: "/var/lib/pgsql", Name: "var-pgsql", Size: "50Gi"}
+// VarPgsqlDataVolumeMount defines the /var/lib/pgsql/data volume mount.
+var VarPgsqlDataVolumeMount = types.VolumeMount{MountPath: "/var/lib/pgsql/data", Name: "var-pgsql", Size: "50Gi"}


Will this version work in the migration from 5.0 to 5.1, since the mounting point will be different?

This is detected in inspect and eventually fixed in pgsqlMigrationScriptTemplate

sonarqubecloud · 2025-01-31T14:42:20Z

Quality Gate failed

Failed conditions
4 New issues
8.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

cbosdo · 2025-02-03T10:37:27Z

mgradm/shared/pgsql/pgsql.go

+		environment += fmt.Sprintf("Environment=POSTGRES_USER=\"%s\"\n", admin)
+	}
+	if password != "" {
+		environment += fmt.Sprintf("Environment=POSTGRES_PASSWORD=\"%s\"\n", password)


We cannot use passwords as environment with systemd. We'll need to use a podman secret here

Dedicated container for database

a16a87c

nadvornik force-pushed the postgres branch from a80fa6a to fbf2549 Compare January 9, 2025 09:20

Create dedicated postgres container in upgrade and migration

10155ce

nadvornik force-pushed the postgres branch from fbf2549 to 10155ce Compare January 9, 2025 09:55

Fixed unit tests

effcc85

aaannz reviewed Jan 22, 2025

View reviewed changes

rjmateus reviewed Jan 31, 2025

View reviewed changes

cbosdo reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedicated container for database #482

Dedicated container for database #482

nadvornik commented Oct 30, 2024 •

edited

Loading

aaannz left a comment

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

aaannz Jan 22, 2025

rjmateus left a comment

rjmateus Jan 31, 2025

rjmateus Jan 31, 2025

rjmateus Jan 31, 2025

rjmateus Jan 31, 2025

rjmateus Jan 31, 2025

cbosdo Feb 3, 2025

rjmateus Jan 31, 2025

nadvornik Feb 5, 2025

rjmateus Jan 31, 2025

rjmateus Jan 31, 2025

nadvornik Feb 5, 2025

sonarqubecloud bot commented Jan 31, 2025 •

edited

Loading

cbosdo Feb 3, 2025

	log.Debug().Msgf("Current HUB replicas running are %d.", currentReplicas)
	log.Debug().Msgf("Current pgsql replicas running are %d.", currentReplicas)

	log.Debug().Msg("No pgsql requested.")
	log.Info().Msg(L("No Postgres container requested."))

	log.Info().Msgf(L("No changes requested for hub. Keep %d replicas."), currentReplicas)
	log.Info().Msgf(L("No changes requested for pgsql. Keep %d replicas."), currentReplicas)

	// Now the servisce is up and ready, the admin credentials are no longer needed
	// Now the service is up and ready, the admin credentials are no longer needed

	// SPDX-FileCopyrightText: 2024 SUSE LLC
	// SPDX-FileCopyrightText: 2025 SUSE LLC

	// AddPgsqlFlags adds hub XML-RPC related parameters to cmd.
	// AddPgsqlFlags adds postgres related parameters to cmd.

	// AddUpgradePgsqlFlags adds hub XML-RPC related parameters to cmd upgrade.
	// AddUpgradePgsqlFlags adds postgres related parameters to cmd upgrade.

	log.Warn().Msg(L("Multiple Hub XML-RPC container replicas are not currently supported, setting up only one."))
	log.Warn().Msg(L("Multiple Pgsql container replicas are not supported, setting up only one."))

Dedicated container for database #482

Are you sure you want to change the base?

Dedicated container for database #482

Conversation

nadvornik commented Oct 30, 2024 • edited Loading

What does this PR change?

Test coverage

Links

Changelogs

Before you merge

aaannz 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

rjmateus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 31, 2025 • edited Loading

Quality Gate failed

Choose a reason for hiding this comment

nadvornik commented Oct 30, 2024 •

edited

Loading

sonarqubecloud bot commented Jan 31, 2025 •

edited

Loading