Kubernetes refactoring #450

cbosdo · 2024-09-10T17:35:44Z

What does this PR change?

The installation, migration, upgrade and uninstall for Kubernetes has been refactored to:

no longer use a helm chart
use jobs for the migration phases
merge all the installation, migration and upgrade code into a single Reconcile() function.

This PR paves the way for the transition to an operator and improves the reliability of migrations.

As a collateral, SSH agent can no longer be used as that would require
running on a cluster node again. The tool creates a ConfigMap to store the SSH config and known_hosts and a Secret
for a password-less SSH key.

Using Kubernetes API modules also helps for code reuse with a future operator.

Test coverage

Unit tests were added
DONE

Links

Issue(s): #431 #432 #433 #436

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!

During a migration to kubernetes the server is deployed after the rsync to prepare the SSL secrets and PVC. This has the nasty effect to corrupt the synchronized data with a too recent catalog version ID. This would let the DB migration to fail starting the old postgresql server. To workaround this, move the data to the the backup place after the rsync instead of the begining of the db upgrade.

Some pods require a long time to run. This is the case for the DB upgrade finalization that runs a potentially long reindex.

Migration deploys the helm chart multiple times. Identifying which revision corresponds to which step in helm history is hard without a description.

…allCRDs

Of of the issuers creation function had two distinct behaviors and this was only generating confusion when reading the whole code. This function has been split and some useless intermediary functions have been merged. This with better function naming should make the SSL setup code more understandable.

In the kubernetes world we need to link the ports to services. For now we only have a TCP and an UDP service for server and the same for proxy, but in the short term, we will need more services to allow splitting into multiple pods. This refactoring is preparing this split.

Running commands in a running container only works if there is a running container and is harder to unit test. In order to help sharing code for Kubernetes, the SanityCheck now gets the existing deployment version with inspecting its image. This also helps adding unit tests for those checks.

In order to later share code between those 3 very similar commands, we need to share the parameters data structure.

Migration to kubernetes is rather fragile, with: 1. tasks running in `kubectl exec` or as `pod`. 2. the uyuni helm chart being deployed multiple times 3. `hostPath` mounts are used everywhere for the scripts to run and data to read and force the script to run on the cluster node. Here are the solutions to those problems: 1. Each step will run as a Job and those won't be deleted automatically for the user to access their logs after. 2. Stop using the helm chart and deploy the resources when we need them. This will allow more control of what runs when and reduces the number of useless starts of the giant container. Postgresql DB upgrade will disable SSL temporarily in the postgresql.conf in order to not rely on the SSL certificates to be migrated. 3. The scripts to run for each step will be passed directly as `sh -c` parameter to the generated Jobs. The migration data are be stored in a special volume and not on the host. As a collateral, SSH agent can no longer be used as that would require running on a cluster node again. The tool now creates a ConfigMap to store the SSH config and known_hosts and a Secret for a passwordless SSH key. The PersistentVolumes are not destroyed after the end of the first job and are then reused by the next ones and the final deployment. Using Kubernetes API modules also helps for code reuse with a future operator. Note that the old postgresql database cannot be moved to a separate PersistentVolumes. As we run a `db_upgrade --link`, the old database is linked by the new one and cannot be disposed of.

In order to share the same code for installation, migration and upgrade the RunSetup() function needs to move to the mgradm shared utils module.

Remove all server resources without relying on the helm chart.

Refactor upgrade and install of the server to no longer need the helm chart as initiated for the migration, but merge all those logics into a single Reconcile() function to avoid redundancy. Merging the code into a single function will also help figuring out how to implement an operator in the future.

There is no need to run a potentially lengthy reindexing on minor upgrades, only on major ones. Don't call su with `-` parameter as it shows the warning message for terminals... and that looks ugly in logs.

Since helm is no longer used installing Uyuni, but only cert-manager, rename the flags. Also drop those that are no longer used for the server after the refactoring.

With CGO enabled there are include problems on that architecture and that would probably require cross-compiling.

Traefik helm chart changed the structure of the expose property starting version 27. Read the chart version from the trafik.yaml file and write the config accordingly.

Running the first user creation from outside the container relies on the pod to be seen as ready by kubernetes... and sometimes it takes longer than others. Calling the API from the setup script inside the container allows to use localhost and not rely on ingress to route the request.

During the installation, there was a message indicating that the timezone from the host couldn't be set in the container. This was due to no removing the line end from the command output.

aaannz

Just couple notes, probably for future.
I think we should merge this ASAP even it there are things to change, reviewing this is couple hours job.

aaannz · 2024-11-18T14:29:01Z

mgradm/cmd/inspect/podman.go

@@ -9,8 +9,6 @@ import (

 	"github.com/rs/zerolog/log"
 	"github.com/spf13/cobra"
-	adm_utils "github.com/uyuni-project/uyuni-tools/mgradm/shared/utils"
-	"github.com/uyuni-project/uyuni-tools/shared"
 	. "github.com/uyuni-project/uyuni-tools/shared/l10n"
 	shared_podman "github.com/uyuni-project/uyuni-tools/shared/podman"


Suggested change

shared_podman "github.com/uyuni-project/uyuni-tools/shared/podman"

"github.com/uyuni-project/uyuni-tools/shared/podman"

A nitpick, probably due to various refactoring we keep this, but it is no longer needed, there is no namespace clash anymore.
Ofc the rest of the code will need to drop shared_ prefix.

This will also make it the same between inspect and install.

aaannz · 2024-11-18T14:32:53Z

mgradm/cmd/install/kubernetes/utils.go

+func installForKubernetes(
+	globalFlags *types.GlobalFlags,
+	flags *kubernetes.KubernetesServerFlags,
 	cmd *cobra.Command,
 	args []string,
 ) error {


I think for a clarity of code, as this is now just couple lines, can they be moved from utils.go directly to kubernetes.go and drop utils.go?

Seems strange to me to have utils file with single 2 lines command.

aaannz · 2024-11-18T14:38:19Z

mgradm/cmd/migrate/kubernetes/dataExtractor.go

+	out, err := kubernetes.RunPodLogs(namespace, "uyuni-data-extractor", image,
+		pullPolicy, pullSecret, []types.VolumeMount{volume},
+		"sh", "-c",
+		"for f in /var/lib/uyuni-tools/*; do echo \"`basename $f`: |2\"; cat $f | sed 's/^/  /'; done",
+	)


I would appreciate some comment what is this supposed to do?

aaannz · 2024-11-18T16:49:26Z

mgradm/cmd/migrate/kubernetes/ssh.go

+const sshSecretName = "uyuni-migration-key"
+const sshConfigName = "uyuni-migration-ssh"


I would put the constants before functions.

aaannz · 2024-11-18T16:57:19Z

mgradm/shared/kubernetes/certificates.go

+func DeployReusedCa(namespace string, ca *types.SSLPair) ([]string, error) {
+	helmArgs := []string{}


Suggested change

func DeployReusedCa(namespace string, ca *types.SSLPair) ([]string, error) {

helmArgs := []string{}

func DeployReusedCa(namespace string, ca *types.SSLPair) error {

aaannz · 2024-11-18T16:57:32Z

mgradm/shared/kubernetes/certificates.go

@@ -90,33 +94,82 @@ func installSSLIssuers(helmFlags *cmd_utils.HelmFlags, sslFlags *cmd_utils.Insta
 		log.Fatal().Err(err).Msg(L("Failed to create issuer"))
 	}

-	// Wait for issuer to be ready
+	return helmArgs, nil


Suggested change

return helmArgs, nil

return nil

aaannz · 2024-11-18T16:58:23Z

mgradm/shared/kubernetes/reconcile.go

+			}
+
+			// Install the cert-manager issuers
+			if _, err := DeployReusedCa(namespace, &ca); err != nil {


Suggested change

if _, err := DeployReusedCa(namespace, &ca); err != nil {

if err := DeployReusedCa(namespace, &ca); err != nil {

aaannz · 2024-11-18T17:08:55Z

mgradm/shared/kubernetes/ingress.go

+
+var noSSLPaths = []string{
+	"/pub",
+	"/rhn/([^/])+/DownloadFile",
+	"/(rhn/)?rpc/api",
+	"/rhn/errors",
+	"/rhn/ty/TinyUrl",
+	"/rhn/websocket",
+	"/rhn/metrics",
+	"/cobbler_api",
+	"/cblr",
+	"/httpboot",
+	"/images",
+	"/cobbler",
+	"/os-images",
+	"/tftp",
+	"/docs",
+}


This will need some crosslink with relevant parts of uyuni code so we know when remove/add here where else it should be checked.

Like https://github.com/uyuni-project/uyuni/blob/master/spacewalk/config/etc/httpd/conf.d/zz-spacewalk-www.conf

aaannz · 2024-11-18T17:18:12Z

mgradm/shared/templates/reusedCaIssuerTemplate.go

+const uyuniCaIssuer = `apiVersion: cert-manager.io/v1
+kind: Issuer
+metadata:
+  name: uyuni-ca-issuer


Can this reuse constant from kubernetes.CaIssuerName?

aaannz · 2024-11-18T17:23:01Z

uyuni-tools.spec

+%ifarch i386
+%if 0%{?debian}
+# Disable CGO build for debian 32 bits to avoid cross-compilation
+export CGO_ENABLED=0
+%endif
+%endif


Do we care about 32bit Debian systems?

cbosdo marked this pull request as draft September 10, 2024 17:36

cbosdo force-pushed the k8s-refactoring branch 7 times, most recently from 997fe46 to 8f9ea9e Compare September 16, 2024 21:01

cbosdo force-pushed the k8s-refactoring branch 3 times, most recently from e95556e to 837af3c Compare September 23, 2024 13:56

cbosdo force-pushed the k8s-refactoring branch 7 times, most recently from e5c4acd to d0bcbeb Compare October 6, 2024 19:34

cbosdo force-pushed the k8s-refactoring branch from d0bcbeb to c54d24e Compare October 9, 2024 16:11

cbosdo mentioned this pull request Oct 18, 2024

Persist the search indexes in a volume #470

Merged

3 tasks

cbosdo force-pushed the k8s-refactoring branch 3 times, most recently from dcbc987 to ffc6888 Compare October 25, 2024 07:08

cbosdo force-pushed the k8s-refactoring branch 2 times, most recently from d0ca4d4 to b20b6c7 Compare November 6, 2024 08:42

cbosdo force-pushed the k8s-refactoring branch 3 times, most recently from debd9af to c406d19 Compare November 13, 2024 07:39

nadvornik and others added 2 commits November 13, 2024 10:17

Kubernetes support for Hub XML-RPC

c39b6e5

cbosdo added 3 commits November 13, 2024 10:17

Wait for 3 hours when running a pod

e3b2544

Some pods require a long time to run. This is the case for the DB upgrade finalization that runs a potentially long reindex.

Add deployments reason for helm history

7560bc1

Migration deploys the helm chart multiple times. Identifying which revision corresponds to which step in helm history is hard without a description.

Add crds.keep value for cert-manager to keep feature parity with inst…

38864fb

…allCRDs

cbosdo force-pushed the k8s-refactoring branch from c406d19 to 2a9f8f0 Compare November 13, 2024 10:09

cbosdo marked this pull request as ready for review November 13, 2024 10:16

cbosdo force-pushed the k8s-refactoring branch from 2a9f8f0 to c3cd3ef Compare November 13, 2024 11:10

cbosdo requested review from aaannz, mbussolotto and nadvornik November 13, 2024 11:16

cbosdo force-pushed the k8s-refactoring branch 2 times, most recently from 43ee4eb to 9dd02b9 Compare November 15, 2024 10:19

This was referenced Nov 15, 2024

Adjust the server_containerized module to kubernetes refactoring uyuni-project/sumaform#1747

Merged

Use K3S CI to test Kubernetes refactoring SUSE/susemanager-ci#1465

Merged

cbosdo added 4 commits November 18, 2024 14:59

Use one data structure for install, migrate, upgrade flags

475be63

In order to later share code between those 3 very similar commands, we need to share the parameters data structure.

cbosdo force-pushed the k8s-refactoring branch from 9dd02b9 to 4bb6e26 Compare November 18, 2024 14:00

cbosdo added 10 commits November 18, 2024 16:17

Move the RunSetup function to shared

0a7d697

In order to share the same code for installation, migration and upgrade the RunSetup() function needs to move to the mgradm shared utils module.

Refactor kubernetes uninstall

92c7c29

Remove all server resources without relying on the helm chart.

Migration script improvements

bf0def5

There is no need to run a potentially lengthy reindexing on minor upgrades, only on major ones. Don't call su with `-` parameter as it shows the warning message for terminals... and that looks ugly in logs.

Change the --helm-* parameters into --kubernetes-*

5c44006

Since helm is no longer used installing Uyuni, but only cert-manager, rename the flags. Also drop those that are no longer used for the server after the refactoring.

Disable CGO build for Debian i586 to avoid cross-compiling

1409477

With CGO enabled there are include problems on that architecture and that would probably require cross-compiling.

Handle traefik helm chart breaking change to expose ports

3a48af0

Traefik helm chart changed the structure of the expose property starting version 27. Read the chart version from the trafik.yaml file and write the config accordingly.

Remove the line end in the local timezone

f65f29a

During the installation, there was a message indicating that the timezone from the host couldn't be set in the container. This was due to no removing the line end from the command output.

cbosdo force-pushed the k8s-refactoring branch from 4bb6e26 to f65f29a Compare November 18, 2024 15:18

aaannz approved these changes Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubernetes refactoring #450

Kubernetes refactoring #450

cbosdo commented Sep 10, 2024 •

edited

Loading

aaannz left a comment

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

aaannz Nov 18, 2024

	shared_podman "github.com/uyuni-project/uyuni-tools/shared/podman"
	"github.com/uyuni-project/uyuni-tools/shared/podman"

		const sshSecretName = "uyuni-migration-key"
		const sshConfigName = "uyuni-migration-ssh"

		func DeployReusedCa(namespace string, ca *types.SSLPair) ([]string, error) {
		helmArgs := []string{}

	if _, err := DeployReusedCa(namespace, &ca); err != nil {
	if err := DeployReusedCa(namespace, &ca); err != nil {

Kubernetes refactoring #450

Are you sure you want to change the base?

Kubernetes refactoring #450

Conversation

cbosdo commented Sep 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbosdo commented Sep 10, 2024 •

edited

Loading