rdctl: linux/darwin: use process group on factory reset #7908

mook-as · 2024-12-09T23:54:58Z

This adjust rdctl to also use process groups on factory reset, as in #7846.

This will need to land after #7846 and #7838 due to conflicts.

This ensures we terminate the whole process group when we do a factory reset. This also changes some of the other process killing to not rely on spawning /usr/bin/pkill or pgrep. Signed-off-by: Mark Yen <mark.yen@suse.com>

This adds tests for (mainly) GetRDLaunchPath and GetMainExecutable; this has meant threading a context through the code so we can override when we get the rdctl path. Signed-off-by: Mark Yen <mark.yen@suse.com>

Nino-K

I just need clarification on including some testing parameters in the actual code to make it testable which is not ideal. I provided a few suggestions about that in some areas, let me know if that makes sense.

Nino-K · 2024-12-12T18:35:13Z

src/go/rdctl/pkg/directories/directories.go

+
+// OverrideRdctlPath produces a context that will override the path of the rdctl
+// executable.  This should only be used in tests.
+func OverrideRdctlPath(ctx context.Context, rdctlPath string) context.Context {


Why is this here? It should be part of the test, that way, you can make sure nobody can call it mistakenly.

Because the relevant tests are cross-package. (Using internal wouldn't work, because we're not using this as a library, so we're just guarding from ourselves.)

Nino-K · 2024-12-12T18:40:42Z

src/go/rdctl/pkg/directories/directories.go

 )

+type rdctlOverrideKeyType struct{}


Does it make sense to change the type to String constant and export it? The context is limited enough here to avoid context key collision(guarantee uniqueness) by using string.

Per the docs, we should define our own context type, and specifically should not be of a built-in type.

Nino-K · 2024-12-12T18:45:02Z

src/go/rdctl/pkg/directories/directories.go

+	var err error
+	override, ok := ctx.Value(rdctlOverrideKey).(string)
+	if ok {
+		exePathWithSymlinks = override


I was gonna suggest to checking for empty paths but filepath.EvalSymlinks(exePathWithSymlinks) should take care of that.

Nino-K · 2024-12-12T18:50:28Z

src/go/rdctl/pkg/directories/directories_test.go

+		actual, err := directories.GetApplicationDirectory(ctx)
+		require.NoError(t, err)
+		assert.Equal(t, dir, actual)
+	})


Would it make sense to add some additional test cases for the following scenarios:

Invalid or empty executable path.

Non-existent executable file or Empty directory.

Symlink loop or recursive symlinks.

Test cases added. (Obviously the symlink test is skipped on Windows.)

Nino-K · 2024-12-12T18:53:46Z

src/go/rdctl/pkg/directories/directories_windows.go

@@ -83,12 +83,11 @@ func getKnownFolder(folder *windows.KNOWNFOLDERID) (string, error) {
 	)
 	// SHGetKnownFolderPath documentation says we _must_ free the result with


Should there be a link?

This is not a doc comment, so the link wouldn't work. I can add the URL as plain text though. (The https://git.io/JMpgD in the function doc comment actually links to the documentation.)

Nino-K · 2024-12-12T18:59:17Z

src/go/rdctl/pkg/paths/paths.go

-		return "", fmt.Errorf("failed to resolve %q: %w", rdctlSymlinkPath, err)
+// Get the path to the resources directory (the parent directory of the
+// platform-specific directory); this is used to fill in [Paths.Resources].
+// The argument is used for testing; do not provide it in non-test code.


The idea here works but seems like a hack to bend the rules for testing. Should this be a struct member function, with a separate SetPath... function that tests can exclusively call?

Actually, since the test in the same package, I don't even need that. But you're right, changing the function signature was silly.

Nino-K · 2024-12-12T19:05:27Z

src/go/rdctl/pkg/paths/paths_darwin.go

+			return dir, nil
+		}
+	}
+	return "", errors.New("search locations exhausted")


Should the returned error be more descriptive to include the locations that were searched (we will thank ourselves during debugging :b)?

Makes sense; will do that here, and for the other platforms, plus for GetMainExecutable.

Nino-K · 2024-12-12T19:12:04Z

src/go/rdctl/pkg/paths/paths_linux.go

+			return candidatePath, nil
+		}
+	}
+	return "", errors.New("search locations exhausted")


Should this also include the search locations?

Nino-K · 2024-12-12T19:35:08Z

src/go/rdctl/pkg/shutdown/shutdown.go

@@ -201,20 +221,36 @@ func runCommandIgnoreOutput(cmd *exec.Cmd) error {
 	return cmd.Run()
 }

-func stopLima() error {
+func stopLima(ctx context.Context) error {
 	return runCommandIgnoreOutput(exec.Command(limaCtlPath, "stop", "0"))


We are injecting ctx we don't even use it, why can't we pass it to exec.CommandContext(ctx,....

Good idea, done (and the next two).

- directories: Add more tests, reject rdctl path being a directory. - paths: Drop test-only argument for GetResourcesPath - paths: Add searched paths to GetRDLaunchPath / GetMainExecutable error - shutdown: Use context when launching limactl Signed-off-by: Mark Yen <mark.yen@suse.com>

rdctl: linux/darwin: Terminate process group on factory reset

fe3f759

This ensures we terminate the whole process group when we do a factory reset. This also changes some of the other process killing to not rely on spawning /usr/bin/pkill or pgrep. Signed-off-by: Mark Yen <mark.yen@suse.com>

mook-as force-pushed the rdx/process-groups-factory-reset branch from 095c575 to c0251b2 Compare December 11, 2024 23:15

rdctl: Add missing tests

4be4ed4

This adds tests for (mainly) GetRDLaunchPath and GetMainExecutable; this has meant threading a context through the code so we can override when we get the rdctl path. Signed-off-by: Mark Yen <mark.yen@suse.com>

mook-as force-pushed the rdx/process-groups-factory-reset branch from c0251b2 to 4be4ed4 Compare December 12, 2024 00:27

mook-as marked this pull request as ready for review December 12, 2024 00:28

gunamata requested a review from Nino-K December 12, 2024 17:53

Nino-K reviewed Dec 12, 2024

View reviewed changes

mook-as requested a review from Nino-K December 12, 2024 21:46

mook-as force-pushed the rdx/process-groups-factory-reset branch from 4bc4229 to 516abf1 Compare December 12, 2024 21:47

mook-as force-pushed the rdx/process-groups-factory-reset branch from 516abf1 to 88e930e Compare December 12, 2024 21:50

Nino-K approved these changes Dec 12, 2024

View reviewed changes

mook-as merged commit bbd7c63 into rancher-sandbox:main Dec 12, 2024
21 checks passed

mook-as deleted the rdx/process-groups-factory-reset branch December 12, 2024 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rdctl: linux/darwin: use process group on factory reset #7908

rdctl: linux/darwin: use process group on factory reset #7908

mook-as commented Dec 9, 2024

Nino-K left a comment

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

Nino-K Dec 12, 2024

Nino-K Dec 12, 2024

mook-as Dec 12, 2024

		@@ -83,12 +83,11 @@ func getKnownFolder(folder *windows.KNOWNFOLDERID) (string, error) {
		)
		// SHGetKnownFolderPath documentation says we _must_ free the result with

rdctl: linux/darwin: use process group on factory reset #7908

rdctl: linux/darwin: use process group on factory reset #7908

Conversation

mook-as commented Dec 9, 2024

Nino-K 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

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