Merge pull request #5546 from isabella-janssen/mco-1924

MCO-1924: MCO-500: Migrate direct runGetOut() to use CommandRunner in the MCD

Author: openshift-merge-bot[bot]

pkg/daemon/bootc.go

@@ -181,17 +181,6 @@ func (b *BootcClient) GetBootedAndStagedImage() (*BootEntry, *BootEntry, error)
 	return status.Status.GetBootedImage(), status.Status.GetStagedImage(), nil
 }
 
-// // GetStatus returns multi-line human-readable text describing system status
-// // Bootc is working on status update: https://github.com/containers/bootc/issues/408
-// func (r *BootcClient) GetStatus() (string, error) {
-// 	output, err := runGetOut("bootc", "status")
-// 	if err != nil {
-// 		return "", err
-// 	}
-
-// 	return string(output), nil
-// }
-
 // GetBootedImageInfo() returns the image URL as well as the image version(for logging) and the ostree commit (for comparisons)
 func (b *BootcClient) GetBootedImageInfo() (*BootedImageInfo, error) {
 	bootedImage, _, err := b.GetBootedAndStagedImage()

pkg/daemon/command_runner.go

@@ -35,14 +35,6 @@ func (r *CommandRunnerOS) RunGetOut(command string, args ...string) ([]byte, err
 	return rawOut, nil
 }
 
-// TODO: Delete this function to always consume the interface instance
-// Tracking story: https://issues.redhat.com/browse/MCO-1924
-//
-//	Conserved the old signature to avoid a big footprint bugfix with this change
-func runGetOut(command string, args ...string) ([]byte, error) {
-	return (&CommandRunnerOS{}).RunGetOut(command, args...)
-}
-
 // MockCommandRunner is a test implementation that returns pre-configured outputs.
 type MockCommandRunner struct {
 	outputs map[string][]byte

pkg/daemon/daemon.go

@@ -1310,7 +1310,7 @@ func (dn *Daemon) RunFirstbootCompleteMachineconfig(machineConfigFile string) er
 
 	// Setting the Kernel Arguments is for comparison only with the desired MachineConfig.
 	// The resulting MC should not be for updating node configuration.
-	if err = setRunningKargs(oldConfig, mc.Spec.KernelArguments); err != nil {
+	if err = dn.setRunningKargs(oldConfig, mc.Spec.KernelArguments); err != nil {
 		return fmt.Errorf("failed to set kernel arguments from /proc/cmdline: %w", err)
 	}
 
@@ -1830,7 +1830,7 @@ func (dn *Daemon) getStateAndConfigs() (*stateAndConfigs, error) {
 func (dn *Daemon) LogSystemData() {
 	// Print status if available
 	if dn.os.IsCoreOSVariant() {
-		out, err := runGetOut("rpm-ostree", "status")
+		out, err := dn.cmdRunner.RunGetOut("rpm-ostree", "status")
 		if err != nil {
 			klog.Fatalf("unable to get rpm-ostree status: %s", err)
 		}
@@ -1839,7 +1839,7 @@ func (dn *Daemon) LogSystemData() {
 		logProvisioningInformation()
 	}
 
-	boots, err := runGetOut("journalctl", "--list-boots")
+	boots, err := dn.cmdRunner.RunGetOut("journalctl", "--list-boots")
 	if err != nil {
 		klog.Errorf("Listing boots: %v", err)
 	}
@@ -1852,7 +1852,7 @@ func (dn *Daemon) LogSystemData() {
 	// to also watch dynamically of course.
 	//
 	// also xref https://github.com/coreos/console-login-helper-messages/blob/e8a849f4c23910e7c556c10719911cc59873fc23/usr/share/console-login-helper-messages/profile.sh
-	failedServices, err := runGetOut("systemctl", "list-units", "--state=failed", "--no-legend")
+	failedServices, err := dn.cmdRunner.RunGetOut("systemctl", "list-units", "--state=failed", "--no-legend")
 	switch {
 	case err != nil:
 		klog.Errorf("Listing failed systemd services: %v", err)
@@ -2690,7 +2690,7 @@ func (dn *Daemon) getMachineConfigFromClusterIfNotSet(mc *mcfgv1.MachineConfig,
 // validateKernelArguments checks that the current boot has all arguments specified
 // in the target machineconfig.
 func (dn *CoreOSDaemon) validateKernelArguments(currentConfig *mcfgv1.MachineConfig) error {
-	rpmostreeKargsBytes, err := runGetOut("rpm-ostree", "kargs")
+	rpmostreeKargsBytes, err := dn.cmdRunner.RunGetOut("rpm-ostree", "kargs")
 	if err != nil {
 		return err
 	}

pkg/daemon/on_disk_validation.go

@@ -241,7 +241,8 @@ func checkUnitEnabled(name string, expectedEnabled *bool) error {
 	if expectedEnabled == nil {
 		return nil
 	}
-	outBytes, _ := runGetOut("systemctl", "is-enabled", name)
+	cmdRunner := &CommandRunnerOS{}
+	outBytes, _ := cmdRunner.RunGetOut("systemctl", "is-enabled", name)
 	out := strings.TrimSpace(string(outBytes))
 
 	switch {

pkg/daemon/podman.go

@@ -31,6 +31,8 @@ type PodmanInterface interface {
 	GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error)
 	// GetPodmanInfo retrieves Podman system information.
 	GetPodmanInfo() (*PodmanInfo, error)
+	// CreatePodmanContainer creates a container from the specified image URL.
+	CreatePodmanContainer(additionalArgs []string, containerName, imgURL string) ([]byte, error)
 }
 
 // PodmanExecInterface is the production implementation that executes real podman commands.
@@ -87,3 +89,15 @@ func (p *PodmanExecInterface) GetPodmanInfo() (*PodmanInfo, error) {
 	}
 	return &podmanInfo, nil
 }
+
+// CreatePodmanContainer creates a container from the specified image URL.
+// It executes 'podman create <args> --name <containerName> <imgURL>'.
+func (p *PodmanExecInterface) CreatePodmanContainer(additionalArgs []string, containerName, imgURL string) ([]byte, error) {
+	args := []string{"create"}
+	if len(additionalArgs) > 0 {
+		args = append(args, additionalArgs...)
+	}
+	args = append(args, "--name", containerName, imgURL)
+
+	return p.cmdRunner.RunGetOut("podman", args...)
+}

pkg/daemon/podman_test.go

@@ -219,3 +219,85 @@ func TestGetPodmanInfo_InvalidJSON(t *testing.T) {
 	assert.Nil(t, info)
 	assert.Contains(t, err.Error(), "failed to decode podman system info output")
 }
+
+// Assisted by: Cursor
+func TestCreatePodmanContainer_Success(t *testing.T) {
+	containerName := "test-container"
+	imgURL := "quay.io/openshift/test:latest"
+	additionalArgs := []string{"--net=none", "--annotation=org.openshift.machineconfigoperator.pivot=true"}
+	expectedOutput := []byte("container-id-12345")
+
+	mock := &MockCommandRunner{
+		outputs: map[string][]byte{
+			"podman create --net=none --annotation=org.openshift.machineconfigoperator.pivot=true --name test-container quay.io/openshift/test:latest": expectedOutput,
+		},
+		errors: map[string]error{},
+	}
+
+	podman := NewPodmanExec(mock)
+	output, err := podman.CreatePodmanContainer(additionalArgs, containerName, imgURL)
+
+	assert.Nil(t, err)
+	assert.Equal(t, expectedOutput, output)
+}
+
+// Assisted by: Cursor
+func TestCreatePodmanContainer_NoAdditionalArgs(t *testing.T) {
+	containerName := "test-container"
+	imgURL := "quay.io/openshift/test:latest"
+	expectedOutput := []byte("container-id-67890")
+
+	mock := &MockCommandRunner{
+		outputs: map[string][]byte{
+			"podman create --name test-container quay.io/openshift/test:latest": expectedOutput,
+		},
+		errors: map[string]error{},
+	}
+
+	podman := NewPodmanExec(mock)
+	output, err := podman.CreatePodmanContainer(nil, containerName, imgURL)
+
+	assert.Nil(t, err)
+	assert.Equal(t, expectedOutput, output)
+}
+
+// Assisted by: Cursor
+func TestCreatePodmanContainer_EmptyAdditionalArgs(t *testing.T) {
+	containerName := "test-container"
+	imgURL := "quay.io/openshift/test:latest"
+	expectedOutput := []byte("container-id-empty")
+
+	mock := &MockCommandRunner{
+		outputs: map[string][]byte{
+			"podman create --name test-container quay.io/openshift/test:latest": expectedOutput,
+		},
+		errors: map[string]error{},
+	}
+
+	podman := NewPodmanExec(mock)
+	output, err := podman.CreatePodmanContainer([]string{}, containerName, imgURL)
+
+	assert.Nil(t, err)
+	assert.Equal(t, expectedOutput, output)
+}
+
+// Assisted by: Cursor
+func TestCreatePodmanContainer_CommandError(t *testing.T) {
+	containerName := "test-container"
+	imgURL := "quay.io/openshift/test:latest"
+	additionalArgs := []string{"--net=none"}
+
+	mock := &MockCommandRunner{
+		outputs: map[string][]byte{},
+		errors: map[string]error{
+			"podman create --net=none --name test-container quay.io/openshift/test:latest": fmt.Errorf("podman create failed"),
+		},
+	}
+
+	podman := NewPodmanExec(mock)
+	output, err := podman.CreatePodmanContainer(additionalArgs, containerName, imgURL)
+
+	assert.Error(t, err)
+	assert.Nil(t, output)
+	assert.Contains(t, err.Error(), "podman create failed")
+}

pkg/daemon/update.go

@@ -389,8 +389,8 @@ func setRunningKargsWithCmdline(config *mcfgv1.MachineConfig, requestedKargs []s
 	return nil
 }
 
-func setRunningKargs(config *mcfgv1.MachineConfig, requestedKargs []string) error {
-	rpmostreeKargsBytes, err := runGetOut("rpm-ostree", "kargs")
+func (dn *Daemon) setRunningKargs(config *mcfgv1.MachineConfig, requestedKargs []string) error {
+	rpmostreeKargsBytes, err := dn.cmdRunner.RunGetOut("rpm-ostree", "kargs")
 	if err != nil {
 		return err
 	}
@@ -469,14 +469,15 @@ func pullExtensionsImage(podmanInterface PodmanInterface, imgURL string) error {
 	return err
 }
 
-func podmanCopy(imgURL, osImageContentDir string) (err error) {
+func podmanCopy(podmanInterface PodmanInterface, imgURL, osImageContentDir string) (err error) {
 	// make sure that osImageContentDir doesn't exist
 	os.RemoveAll(osImageContentDir)
 
 	// create a container
 	var cidBuf []byte
 	containerName := pivottypes.PivotNamePrefix + string(uuid.NewUUID())
-	cidBuf, err = runGetOut("podman", "create", "--net=none", "--annotation=org.openshift.machineconfigoperator.pivot=true", "--name", containerName, imgURL)
+	additionalArgs := []string{"--net=none", "--annotation=org.openshift.machineconfigoperator.pivot=true"}
+	cidBuf, err = podmanInterface.CreatePodmanContainer(additionalArgs, containerName, imgURL)
 	if err != nil {
 		return
 	}
@@ -517,7 +518,7 @@ func (dn *CoreOSDaemon) ExtractExtensionsImage(imgURL string) (osExtensionsImage
 		return osExtensionsImageContentDir, fmt.Errorf("error pulling extensions image %s: %w", imgURL, err)
 	}
 	// Extract the image using `podman cp`
-	return osExtensionsImageContentDir, podmanCopy(imgURL, osExtensionsImageContentDir)
+	return osExtensionsImageContentDir, podmanCopy(dn.podmanInterface, imgURL, osExtensionsImageContentDir)
 }
 
 // Remove pending deployment on OSTree based system

pkg/daemon/update_test.go

@@ -687,28 +687,6 @@ func checkIrreconcilableResults(t *testing.T, key string, reconcilableError erro
 	}
 }
 
-func TestRunGetOut(t *testing.T) {
-	o, err := runGetOut("true")
-	assert.Nil(t, err)
-	assert.Equal(t, len(o), 0)
-
-	o, err = runGetOut("false")
-	assert.NotNil(t, err)
-
-	o, err = runGetOut("echo", "hello")
-	assert.Nil(t, err)
-	assert.Equal(t, string(o), "hello\n")
-
-	// base64 encode "oops" so we can't match on the command arguments
-	o, err = runGetOut("/bin/sh", "-c", "echo hello; echo b29wcwo= | base64 -d 1>&2; exit 1")
-	assert.Error(t, err)
-	errtext := err.Error()
-	assert.Contains(t, errtext, "exit status 1\noops\n")
-
-	o, err = runGetOut("/usr/bin/test-failure-to-exec-this-should-not-exist", "arg")
-	assert.Error(t, err)
-}
-
 // TestOriginalFileBackupRestore tests backikg up and restoring original files (files that are present in the base image and
 // get overwritten by a machine configuration)
 func TestOriginalFileBackupRestore(t *testing.T) {
@@ -942,3 +920,57 @@ func TestGenerateExtensionsArgs(t *testing.T) {
 		})
 	}
 }
+
+// MockPodmanInterface for testing podmanCopy function
+type MockPodmanInterface struct {
+	createContainerFunc func(additionalArgs []string, containerName, imgURL string) ([]byte, error)
+}
+
+func (m *MockPodmanInterface) GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error) {
+	return nil, fmt.Errorf("not implemented in mock")
+}
+
+func (m *MockPodmanInterface) GetPodmanInfo() (*PodmanInfo, error) {
+	return nil, fmt.Errorf("not implemented in mock")
+}
+
+func (m *MockPodmanInterface) CreatePodmanContainer(additionalArgs []string, containerName, imgURL string) ([]byte, error) {
+	if m.createContainerFunc != nil {
+		return m.createContainerFunc(additionalArgs, containerName, imgURL)
+	}
+	return []byte("test-container-id"), nil
+}
+
+// Assisted by: Cursor
+func TestPodmanCopy_CreateContainerCall(t *testing.T) {
+	imgURL := "quay.io/openshift/test:latest"
+	osImageContentDir := "/tmp/test-content"
+
+	// Track the call to CreatePodmanContainer
+	var capturedArgs []string
+	var capturedContainerName string
+	var capturedImgURL string
+
+	mockPodman := &MockPodmanInterface{
+		createContainerFunc: func(additionalArgs []string, containerName, imgURL string) ([]byte, error) {
+			capturedArgs = additionalArgs
+			capturedContainerName = containerName
+			capturedImgURL = imgURL
+			return []byte("test-container-id-123"), nil
+		},
+	}
+
+	// Note: This test will fail at podmanRemove since we're only testing the interface call
+	// The function will return an error, but we can verify the CreatePodmanContainer was called correctly
+	err := podmanCopy(mockPodman, imgURL, osImageContentDir)
+
+	// Verify the CreatePodmanContainer was called with correct parameters
+	expectedArgs := []string{"--net=none", "--annotation=org.openshift.machineconfigoperator.pivot=true"}
+	assert.Equal(t, expectedArgs, capturedArgs, "CreatePodmanContainer should be called with correct additional args")
+	assert.Contains(t, capturedContainerName, "pivot-", "Container name should contain pivot prefix")
+	assert.Equal(t, imgURL, capturedImgURL, "Image URL should match")
+
+	// The function will error at podmanRemove, but that's expected in this unit test
+	// We're only testing that the CreatePodmanContainer interface is called correctly
+	assert.Error(t, err, "Expected error from podmanRemove since we're not mocking that part")
+}