Handle default system connection transfer properly on machine removal #23945

jakecorrenti · 2024-09-12T19:59:00Z

Changes system connection removal so the following case now works as expected:

Rootless podman machine 1 is initialized and running.
Rootful podman machine 2 is initialized.
Delete podman machine 1.
The rootful system connection associated with podman machine 2 now becomes the default, not the rootless.

Does this PR introduce a user-facing change?

When a user has a rootless machine running and another rootful podman machine initialized, and the rootless machine is removed, the rootful system connection associated with the podman machine now becomes the default as expected.

Adds the function `GetAllMachinesAndRootfulness` which creates a map of all podman machines, of any supported provider, on the system and whether it is rootful or not. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

The `RemoveFilesAndConnections` function is not being used, so its safe to remove it and not carry unnecessary code that we need to maintain. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

pkg/machine/connection/connection.go

baude · 2024-09-13T12:30:01Z

ok , LGTM (one question)

pkg/machine/connection/connection.go

Luap99

Sorry I just realized the test is not safe/working.

Luap99 · 2024-09-24T15:56:00Z

pkg/machine/connection/connection.go

-		logrus.Error(err)
+
+	// If there is a podman-machine-default system connection, immediately set that as the new default
+	const defaultMachineName = "podman-machine-default"


not blocking but this is already defined as DefaultMachineName in pkg/machine.
I know this will cause a import cycle but then it can be moved into pkg/machine/define to solve this.

Luap99 · 2024-09-24T16:01:53Z

pkg/machine/e2e/rm_test.go

+		bm := basicMachine{}
+		sysConnOutput, err := mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Name}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		names := sysConnOutput.outputToStringSlice()
+		sysConnOutput, err = mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "'{{.Default}}'"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		defaults := sysConnOutput.outputToStringSlice()
+		for idx, n := range names {
+			if n == name {
+				Expect(defaults[idx]).To(Equal("true"))
+			}
+		}


I have not tried to run it but I cannot see how this can work, and if it if it is this is super risk as it can be easily be a NOP. "'{{.Name}}'" is not correct usage because the single quote will be part of the template so it will be in the output.

As such you names loop will never match...

What you should do is something like "--format", "{{.Name}}--{{.Default}}"
And then you just match Expect(sysConnOutput.outputToString()).To(ContainSubstring(name+":true"))

Then below you do the same match again.

That's much cleaner, thank you. I definitely made that more complicated than it needed to be.

Moves the `DefaultMachineName` constant out of `pkg/machine` and into `pkg/machine/define`. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Takes the code inside the closure in the function `RemoveConnections` and makes it a separate function to increase readability. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Modify `RemoveConnections` to verify the new default system connection's rootful state matches the rootful-ness of the podman machine it is associated with. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

packit-as-a-service · 2024-09-24T20:02:53Z

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99 · 2024-09-25T08:59:43Z

pkg/machine/e2e/rm_test.go

+		bm := basicMachine{}
+		sysConnOutput, err := mb.setCmd(bm.withPodmanCommand([]string{"system", "connection", "list", "--format", "{{.Name}}--{{.Default}}"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sysConnOutput.outputToString()).To(ContainSubstring(name + ":true"))


name + "--true"

Sorry that happens if I change my mind mid comment. Or you check the format to be {{.Name}}:{{.Default}}.

Sorry about that, forgot to run the test locally before pushing...

pkg/machine/e2e/rm_test.go

jakecorrenti · 2024-09-25T14:19:34Z

~~something's wrong with my macOS env... I don't think this will end up passing the test. Looking into it~~

Modifies the "Remove machine" test to verify the system connections are handled properly on removal. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Luap99

/lgtm

openshift-ci · 2024-09-27T13:52:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakecorrenti, Luap99

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~~ [Luap99,jakecorrenti]

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

jakecorrenti added 2 commits September 12, 2024 14:31

Add GetAllMachinesAndRootfulness

54b71b6

Adds the function `GetAllMachinesAndRootfulness` which creates a map of all podman machines, of any supported provider, on the system and whether it is rootful or not. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Remove RemoveFilesAndConnections

e041f2a

The `RemoveFilesAndConnections` function is not being used, so its safe to remove it and not carry unnecessary code that we need to maintain. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 12, 2024

github-actions bot added the machine label Sep 12, 2024

baude reviewed Sep 13, 2024

View reviewed changes

pkg/machine/connection/connection.go Show resolved Hide resolved

Luap99 requested changes Sep 17, 2024

View reviewed changes

pkg/machine/connection/connection.go Outdated Show resolved Hide resolved

jakecorrenti force-pushed the default-conn branch from a8d631f to 26a5644 Compare September 23, 2024 16:16

Luap99 requested changes Sep 24, 2024

View reviewed changes

jakecorrenti added 3 commits September 24, 2024 15:36

Move DefaultMachineName to pkg/machine/define

9febd2c

Moves the `DefaultMachineName` constant out of `pkg/machine` and into `pkg/machine/define`. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Simplify RemoveConnections

c709be3

Takes the code inside the closure in the function `RemoveConnections` and makes it a separate function to increase readability. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

Update connection on removal

24deec8

Modify `RemoveConnections` to verify the new default system connection's rootful state matches the rootful-ness of the podman machine it is associated with. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

jakecorrenti force-pushed the default-conn branch from 26a5644 to 5cc52c3 Compare September 24, 2024 19:57

Luap99 reviewed Sep 25, 2024

View reviewed changes

jakecorrenti force-pushed the default-conn branch from 5cc52c3 to c1e0071 Compare September 25, 2024 14:08

Modify machine "Remove machine" test

e9b8564

Modifies the "Remove machine" test to verify the system connections are handled properly on removal. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>

jakecorrenti force-pushed the default-conn branch from c1e0071 to e9b8564 Compare September 25, 2024 14:29

Luap99 approved these changes Sep 27, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Sep 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

openshift-merge-bot bot merged commit adbb735 into containers:main Sep 27, 2024
80 of 86 checks passed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 27, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle default system connection transfer properly on machine removal #23945

Handle default system connection transfer properly on machine removal #23945

jakecorrenti commented Sep 12, 2024

baude commented Sep 13, 2024

Luap99 left a comment

Luap99 Sep 24, 2024

jakecorrenti Sep 24, 2024

Luap99 Sep 24, 2024

jakecorrenti Sep 24, 2024

packit-as-a-service bot commented Sep 24, 2024

Luap99 Sep 25, 2024

jakecorrenti Sep 25, 2024

jakecorrenti commented Sep 25, 2024 •

edited

Loading

Luap99 left a comment

openshift-ci bot commented Sep 27, 2024

Handle default system connection transfer properly on machine removal #23945

Handle default system connection transfer properly on machine removal #23945

Conversation

jakecorrenti commented Sep 12, 2024

Does this PR introduce a user-facing change?

baude commented Sep 13, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Sep 24, 2024

Choose a reason for hiding this comment

jakecorrenti Sep 24, 2024

Choose a reason for hiding this comment

Luap99 Sep 24, 2024

Choose a reason for hiding this comment

jakecorrenti Sep 24, 2024

Choose a reason for hiding this comment

packit-as-a-service bot commented Sep 24, 2024

Luap99 Sep 25, 2024

Choose a reason for hiding this comment

jakecorrenti Sep 25, 2024

Choose a reason for hiding this comment

jakecorrenti commented Sep 25, 2024 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 27, 2024

jakecorrenti commented Sep 25, 2024 •

edited

Loading