From aa8c2ef458e1d66ef2eefab70fd41eea288322c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Bosdonnat?= Date: Mon, 29 Sep 2025 17:15:43 +0200 Subject: [PATCH] Actually use --dbupgrade-tag when computing the image URL There are a few points fixed in this commit around the dbupgrade computation (bsc#1249400): - the default tag should be "" to reuse the global tag - appending the migration suffix to the server image name should not be done in the ComputeImage as it makes it more complex. - unit tests are badly needed The added unit test could be extended to cover more than the image that are computed and used, but this will wait. --- mgradm/shared/kubernetes/dbUpgradeJob.go | 6 +- mgradm/shared/podman/podman.go | 15 ++- mgradm/shared/podman/podman_test.go | 53 ++++++++ mgradm/shared/utils/cmd_utils.go | 2 +- shared/utils/utils.go | 7 +- shared/utils/utils_test.go | 152 ++++++++++------------- uyuni-tools.changes.cbosdo.5.1-dbupgrade | 2 + 7 files changed, 137 insertions(+), 100 deletions(-) create mode 100644 uyuni-tools.changes.cbosdo.5.1-dbupgrade diff --git a/mgradm/shared/kubernetes/dbUpgradeJob.go b/mgradm/shared/kubernetes/dbUpgradeJob.go index c0717340679..130be7dbf09 100644 --- a/mgradm/shared/kubernetes/dbUpgradeJob.go +++ b/mgradm/shared/kubernetes/dbUpgradeJob.go @@ -37,11 +37,9 @@ func StartDBUpgradeJob( var migrationImageURL string var err error if migrationImage.Name == "" { - imageName := fmt.Sprintf("-migration-%s-%s", oldPgsql, newPgsql) - migrationImageURL, err = utils.ComputeImage(image.Registry.Host, image.Tag, image, imageName) - } else { - migrationImageURL, err = utils.ComputeImage(image.Registry.Host, image.Tag, migrationImage) + migrationImage.Name = fmt.Sprintf("%s-migration-%s-%s", image.Name, oldPgsql, newPgsql) } + migrationImageURL, err = utils.ComputeImage(image.Registry.Host, image.Tag, migrationImage) if err != nil { return "", utils.Error(err, L("failed to compute image URL")) } diff --git a/mgradm/shared/podman/podman.go b/mgradm/shared/podman/podman.go index fdbba8cdc6f..4b5d56b77fb 100644 --- a/mgradm/shared/podman/podman.go +++ b/mgradm/shared/podman/podman.go @@ -198,6 +198,9 @@ func restoreSELinuxContext() error { return nil } +var prepareImage = podman.PrepareImage +var runContainer = podman.RunContainer + // RunPgsqlVersionUpgrade perform a PostgreSQL major upgrade. func RunPgsqlVersionUpgrade( authFile string, @@ -219,8 +222,10 @@ func RunPgsqlVersionUpgrade( upgradeImageURL := "" var err error if upgradeImage.Name == "" { - upgradeImageURL, err = utils.ComputeImage(image.Registry.Host, utils.DefaultTag, image, - fmt.Sprintf("-migration-%s-%s", oldPgsql, newPgsql)) + if upgradeImage.Name == "" { + upgradeImage.Name = fmt.Sprintf("%s-migration-%s-%s", image.Name, oldPgsql, newPgsql) + } + upgradeImageURL, err = utils.ComputeImage(image.Registry.Host, image.Tag, upgradeImage) if err != nil { return utils.Errorf(err, L("failed to compute image URL")) } @@ -231,14 +236,14 @@ func RunPgsqlVersionUpgrade( } } - preparedImage, err := podman.PrepareImage(authFile, upgradeImageURL, image.PullPolicy, true) + preparedImage, err := prepareImage(authFile, upgradeImageURL, image.PullPolicy, true) if err != nil { return err } log.Info().Msgf(L("Using database upgrade image %s"), preparedImage) - // We need an aditional volume for database backup during the migration + // We need an additional volume for database backup during the migration // Create or reuse var-pgsql-backup volume volumeMounts := append(utils.PgsqlRequiredVolumeMounts, types.VolumeMount{MountPath: "/var/lib/pgsql/data-backup", Name: "var-pgsql-backup"}) @@ -249,7 +254,7 @@ func RunPgsqlVersionUpgrade( return utils.Errorf(err, L("cannot generate PostgreSQL database version upgrade script")) } - err = podman.RunContainer(pgsqlVersionUpgradeContainer, preparedImage, volumeMounts, extraArgs, + err = runContainer(pgsqlVersionUpgradeContainer, preparedImage, volumeMounts, extraArgs, []string{"bash", "-e", "-c", script}) if err != nil { return err diff --git a/mgradm/shared/podman/podman_test.go b/mgradm/shared/podman/podman_test.go index 89e4259b619..334de7499d5 100644 --- a/mgradm/shared/podman/podman_test.go +++ b/mgradm/shared/podman/podman_test.go @@ -5,9 +5,11 @@ package podman import ( + "fmt" "testing" "github.com/uyuni-project/uyuni-tools/shared/testutils" + "github.com/uyuni-project/uyuni-tools/shared/types" ) func TestHasDebugPorts(t *testing.T) { @@ -66,3 +68,54 @@ ExecStart=/bin/sh -c '/usr/bin/podman run \ testutils.AssertEquals(t, "Unexpected result for "+definition, expected, actual) } } + +func TestRunPgsqlVersionUpgrade(t *testing.T) { + cases := []struct { + registry string + image types.ImageFlags + upgradeImage types.ImageFlags + expectedImage string + }{ + // Default Uyuni case with global tag set + { + "registry.opensuse.org/uyuni", + types.ImageFlags{ + Name: "registry.opensuse.org/uyuni/server", + Tag: "2025.08", + PullPolicy: "ifnotpresent", + }, + types.ImageFlags{}, + "registry.opensuse.org/uyuni/server-migration-14-16:2025.08", + }, + // own registry case with a special image for the main server but not upgrade + { + "registry.example.com/product", + types.ImageFlags{ + Name: "registry.example.com/product/server", + Tag: "fix-123", + PullPolicy: "always", + }, + types.ImageFlags{ + Name: "registry.example.com/product/server-migration-14-16", + Tag: "4.5.2", + }, + "registry.example.com/product/server-migration-14-16:4.5.2", + }, + } + + expectedAuthfile := "authfile to pass" + for i, testCase := range cases { + prepareImage = func(authFile string, image string, pullPolicy string, _ bool) (string, error) { + // test that the image computation + testutils.AssertEquals(t, "auth file not passed down", expectedAuthfile, authFile) + testutils.AssertEquals(t, fmt.Sprintf("case %d: wrong image", i), testCase.expectedImage, image) + testutils.AssertEquals(t, fmt.Sprintf("case %d: wrong pull policy", i), testCase.image.PullPolicy, pullPolicy) + return image, nil + } + runContainer = func(_ string, image string, _ []types.VolumeMount, _ []string, _ []string) error { + testutils.AssertEquals(t, fmt.Sprintf("case %d: wrong image used for container", i), testCase.expectedImage, image) + return nil + } + _ = RunPgsqlVersionUpgrade(expectedAuthfile, testCase.image, testCase.upgradeImage, "14", "16") + } +} diff --git a/mgradm/shared/utils/cmd_utils.go b/mgradm/shared/utils/cmd_utils.go index d706a280de0..bbbd119e99d 100644 --- a/mgradm/shared/utils/cmd_utils.go +++ b/mgradm/shared/utils/cmd_utils.go @@ -203,7 +203,7 @@ func AddImageFlag(cmd *cobra.Command) { // AddDBUpgradeImageFlag add Database upgrade image flags to a command. func AddDBUpgradeImageFlag(cmd *cobra.Command) { cmd.Flags().String("dbupgrade-image", "", L("Database upgrade image")) - cmd.Flags().String("dbupgrade-tag", "latest", L("Database upgrade image tag")) + cmd.Flags().String("dbupgrade-tag", "", L("Database upgrade image tag, overrides the default value if set")) _ = utils.AddFlagHelpGroup(cmd, &utils.Group{ID: "dbupgrade-image", Title: L("Database Upgrade Image Flags")}) _ = utils.AddFlagToHelpGroupID(cmd, "dbupgrade-image", "dbupgrade-image") diff --git a/shared/utils/utils.go b/shared/utils/utils.go index 0bea5b58946..91588ea6f3b 100644 --- a/shared/utils/utils.go +++ b/shared/utils/utils.go @@ -202,7 +202,6 @@ func ComputeImage( globalRegistry string, globalTag string, imageFlags types.ImageFlags, - appendToName ...string, ) (string, error) { // Compute the registry registry := globalRegistry @@ -230,17 +229,13 @@ func ComputeImage( if len(tag) <= 0 { return name, fmt.Errorf(L("tag missing on %s"), name) } - if len(appendToName) > 0 { - name = name + strings.Join(appendToName, ``) - } // No tag provided in the URL name, append the one passed imageName := fmt.Sprintf("%s:%s", name, tag) imageName = strings.ToLower(imageName) // podman does not accept repo in upper case log.Info().Msgf(L("Computed image name is %s"), imageName) return imageName, nil } - imageName := submatches[1] + strings.Join(appendToName, ``) + `:` + submatches[2] - imageName = strings.ToLower(imageName) // podman does not accept repo in upper case + imageName := strings.ToLower(name) // podman does not accept repo in upper case log.Info().Msgf(L("Computed image name is %s"), imageName) return imageName, nil } diff --git a/shared/utils/utils_test.go b/shared/utils/utils_test.go index 2f3e434df1c..5244e60361d 100644 --- a/shared/utils/utils_test.go +++ b/shared/utils/utils_test.go @@ -291,118 +291,102 @@ func TestComputePTF(t *testing.T) { func TestComputeImage(t *testing.T) { tests := []struct { - expected string - name string - tag string - registry string - appendToImage []string + expected string + name string + tag string + registry string }{ { - expected: "registry:5000/path/to/image:foo", - name: "REGISTRY:5000/path/to/image:foo", - tag: "bar", - registry: "", - appendToImage: []string{""}, + expected: "registry:5000/path/to/image:foo", + name: "REGISTRY:5000/path/to/image:foo", + tag: "bar", + registry: "", }, { - expected: "registry:5000/path/to/image:foo", - name: "REGISTRY:5000/path/to/image:foo", - tag: "BAR", - registry: "", - appendToImage: []string{""}, + expected: "registry:5000/path/to/image:foo", + name: "REGISTRY:5000/path/to/image:foo", + tag: "BAR", + registry: "", }, { - expected: "registry:5000/path/to/image:bar", - name: "registry:5000/path/to/image", - tag: "bar", - registry: "", - appendToImage: []string{""}, + expected: "registry:5000/path/to/image:bar", + name: "registry:5000/path/to/image", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image:foo", - name: "registry/path/to/image:foo", - tag: "bar", - registry: "", - appendToImage: []string{""}, + expected: "registry/path/to/image:foo", + name: "registry/path/to/image:foo", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image:bar", - name: "registry/path/to/image", - tag: "bar", - registry: "", - appendToImage: []string{""}, + expected: "registry/path/to/image:bar", + name: "registry/path/to/image", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image:bar", - name: "path/to/image", - tag: "bar", - registry: "registry", - appendToImage: []string{""}, + expected: "registry/path/to/image:bar", + name: "path/to/image", + tag: "bar", + registry: "registry", }, { - expected: "registry:5000/path/to/image:foo", - name: "path/to/image:foo", - tag: "BAR", - registry: "REGISTRY:5000", - appendToImage: []string{""}, + expected: "registry:5000/path/to/image:foo", + name: "path/to/image:foo", + tag: "BAR", + registry: "REGISTRY:5000", }, { - expected: "registry:5000/path/to/image-migration-14-16:foo", - name: "registry:5000/path/to/image:foo", - tag: "bar", - registry: "", - appendToImage: []string{"-migration-14-16"}, + expected: "registry:5000/path/to/image-migration-14-16:foo", + name: "registry:5000/path/to/image-migration-14-16:foo", + tag: "bar", + registry: "", }, { - expected: "registry:5000/path/to/image-migration-14-16:bar", - name: "registry:5000/path/to/image", - tag: "bar", - registry: "", - appendToImage: []string{"-migration-14-16"}, + expected: "registry:5000/path/to/image-migration-14-16:bar", + name: "registry:5000/path/to/image-migration-14-16", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image-migration-14-16:foo", - name: "registry/path/to/image:foo", - tag: "bar", - registry: "", - appendToImage: []string{"-migration-14-16"}, + expected: "registry/path/to/image-migration-14-16:foo", + name: "registry/path/to/image-migration-14-16:foo", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image-migration-14-16:bar", - name: "registry/path/to/image", - tag: "bar", - registry: "", - appendToImage: []string{"-migration-14-16"}, + expected: "registry/path/to/image-migration-14-16:bar", + name: "registry/path/to/image-migration-14-16", + tag: "bar", + registry: "", }, { - expected: "registry/path/to/image-migration-14-16:bar", - name: "path/to/image", - tag: "bar", - registry: "registry", - appendToImage: []string{"-migration-14-16"}, + expected: "registry/path/to/image-migration-14-16:bar", + name: "path/to/image-migration-14-16", + tag: "bar", + registry: "registry", }, { expected: "registry.suse.de/suse/sle-15-sp6/update/products/manager50/containerfile/" + "suse/manager/5.0/x86_64/server:bar", - name: "suse/manager/5.0/x86_64/server", - tag: "bar", - registry: "registry.suse.de/suse/sle-15-sp6/update/products/manager50/containerfile", - appendToImage: []string{""}, + name: "suse/manager/5.0/x86_64/server", + tag: "bar", + registry: "registry.suse.de/suse/sle-15-sp6/update/products/manager50/containerfile", }, { - expected: "cloud.com/suse/manager/5.0/x86_64/server:5.0.0", - name: "/suse/manager/5.0/x86_64/server", - tag: "5.0.0", - registry: "cloud.com", - appendToImage: []string{""}, + expected: "cloud.com/suse/manager/5.0/x86_64/server:5.0.0", + name: "/suse/manager/5.0/x86_64/server", + tag: "5.0.0", + registry: "cloud.com", }, // ignore registry if name contains fqdn { - expected: "cloud.com/my/path/server:5.0.0", - name: "cloud.com/my/path/server:5.0.0", - tag: "5.0.0", - registry: "registy.suse.com", - appendToImage: []string{""}, + expected: "cloud.com/my/path/server:5.0.0", + name: "cloud.com/my/path/server:5.0.0", + tag: "5.0.0", + registry: "registy.suse.com", }, } @@ -412,18 +396,18 @@ func TestComputeImage(t *testing.T) { Tag: testCase.tag, } - actual, err := ComputeImage(testCase.registry, "defaulttag", image, testCase.appendToImage...) + actual, err := ComputeImage(testCase.registry, "defaulttag", image) if err != nil { t.Errorf( - "Testcase %d: Unexpected error while computing image with %s, %s, %s, %s: %s", - i, testCase.registry, image.Name, image.Tag, strings.Join(testCase.appendToImage, ","), err, + "Testcase %d: Unexpected error while computing image with %s, %s, %s: %s", + i, testCase.registry, image.Name, image.Tag, err, ) } if actual != testCase.expected { t.Errorf( - "Testcase %d: Expected %s got %s when computing image with %s, %s, %s, %s", - i, testCase.expected, actual, testCase.registry, image.Name, image.Tag, strings.Join(testCase.appendToImage, ","), + "Testcase %d: Expected %s got %s when computing image with %s, %s, %s", + i, testCase.expected, actual, testCase.registry, image.Name, image.Tag, ) } } diff --git a/uyuni-tools.changes.cbosdo.5.1-dbupgrade b/uyuni-tools.changes.cbosdo.5.1-dbupgrade new file mode 100644 index 00000000000..a49f84c3ab4 --- /dev/null +++ b/uyuni-tools.changes.cbosdo.5.1-dbupgrade @@ -0,0 +1,2 @@ +- Actually use the --dbupgrade-tag parameter when computing the + image URL (bsc#1249400)