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

Improved Hub XML-RPC support #357

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

nadvornik
Copy link
Contributor

What does this PR change?

This PR adds Hub XML-RPC support to podman migrate and upgrade and to all kubernetes commands.

Also, it fixes some kubernetes problems:

  • incorrect arg to GetNode calls
  • error detection on traefik setup

This PR depends on uyuni-project/uyuni#8902

Test coverage

  • No tests: add explanation

  • No tests: already covered

  • Unit tests were added

  • DONE

Links

Issue(s): #292

  • 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!

@nadvornik nadvornik force-pushed the hub-xmlrpc2 branch 3 times, most recently from 6aeba80 to 8dbb800 Compare June 11, 2024 12:39
@nadvornik nadvornik requested a review from cbosdo June 11, 2024 12:56
mgradm/shared/kubernetes/install.go Outdated Show resolved Hide resolved
mgradm/shared/kubernetes/install.go Outdated Show resolved Hide resolved
mgradm/shared/kubernetes/install.go Outdated Show resolved Hide resolved
mgradm/shared/kubernetes/k3s.go Outdated Show resolved Hide resolved
mgradm/shared/podman/podman.go Outdated Show resolved Hide resolved
shared/kubernetes/utils.go Outdated Show resolved Hide resolved
shared/kubernetes/utils.go Outdated Show resolved Hide resolved
@nadvornik nadvornik requested a review from cbosdo June 14, 2024 08:21
mgradm/cmd/migrate/shared/flags.go Outdated Show resolved Hide resolved
mgradm/cmd/migrate/podman/utils.go Outdated Show resolved Hide resolved
Comment on lines 28 to 30
dummyMigration := types.ImageFlags{}
dummyCoco := types.ImageFlags{}
dummyHubXmlrpc := types.ImageFlags{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a variable for each flags, could we just have an emptyImageFlags and use it for all parameters?

Comment on lines 28 to 31
tag := hubXmlrpcFlags.Tag
if tag == "" {
tag = imageFlags.Tag
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll need to make sure I have this logic wired in the new implementation of ComputeImage I'm working on

@@ -373,3 +373,15 @@ func GenerateOverrideDeployment(deployData types.Deployment) (string, error) {
}
return string(ret), nil
}

// GetRunningImage returns the image running in the current system.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// GetRunningImage returns the image running in the current system.
// GetRunningImage returns the image of containerName for the server running in the current system.

@@ -0,0 +1 @@
- Improved Hub XML-RPC support
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Improved Hub XML-RPC support
- Handle Hub XML-RPC in migration and kubernetes.

@@ -40,10 +41,11 @@ func Deploy(
isK3s := clusterInfos.IsK3s()
IsRke2 := clusterInfos.IsRke2()
if !prepare {
tcpPorts, udpPorts := GetPortLists(hubXmlrpcFlags.Replicas > 0, debug)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to setup traefik / nginx with hub ports in all cases or not. The problem is that if the user decides to scale the number of replicas of the hub API, then traefik / nginx config is not ready.

cnx := shared.NewConnection("kubectl", "", kubernetes.ServerFilter)

origHubXmlrpcImage, err := kubernetes.GetRunningImage("hub-xmlrpc-api")
Copy link
Contributor

Choose a reason for hiding this comment

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

This would work for Hub XML-RPC API since we can have at most 1 replica, but I would see a Connection.GetReplicasCount("hub-xmlrpc-api"). This could also be reused for CoCo containers in the future.

stylecheck reports that modules should not have underscore in their
names: rename test_utils to fix this. Rule ST1003.

Note that mixed caps are not allowed in module names either.
stylecheck reports that modules should not have underscore in their
names: rename flags_tests to fix this. Rule ST1003.

Note that mixed caps are not allowed in module names either.
cbosdo and others added 8 commits October 24, 2024 16:35
Since the testing module provides a TempDir function, use it in our
tests. This also removes a number of errcheck errors about unchecked
error in defer.
Returning the function cleaning the temporary folder from its creation
function factorizes the removal and removes the missing checks for the
os.RemoveAll() error.
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.
After the k8s migration the pod has been started again since the initial
connection creation. We need to reset the connection to not search for
the old pod name.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants