fix(image): give each node import its own archive and always remove it

pkg/svc/image/exec.go

@@ -78,3 +78,14 @@ func (e *ContainerExecutor) ExecInContainer(
 
 	return stdout.String(), nil
 }
+
+// removeNodeArchive deletes an export or import archive from a node. It always
+// runs on a context detached from the caller and bounded by nodeCleanupTimeout,
+// so a cancellation that lands while removal is starting cannot abort it and
+// leave the archive behind.
+func (e *ContainerExecutor) removeNodeArchive(ctx context.Context, nodeName, tmpPath string) {
+	cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), nodeCleanupTimeout)
+	defer cancel()
+
+	_, _ = e.ExecInContainer(cleanupCtx, nodeName, []string{"rm", "-f", tmpPath})
+}

pkg/svc/image/export_test.go

@@ -4,18 +4,22 @@ package image
 // IsHelperContainerForTest exposes isHelperContainer for white-box testing.
 var IsHelperContainerForTest = isHelperContainer
 
-// Every test in this package's binary sees a fixed export archive name, so
+// Every test in this package's binary sees fixed export and import archive names, so
 // node-side command expectations stay exact. The default generator is exposed
 // below and tested on its own.
 //
 //nolint:gochecknoinits // The seam must be in place before parallel tests start.
 func init() {
 	newExportArchiveName = func() string { return "ksail-images-export.tar" }
+	newImportArchiveName = func() string { return "ksail-images-import.tar" }
 }
 
 // DefaultExportArchiveNameForTest exposes the production archive name generator.
 var DefaultExportArchiveNameForTest = defaultExportArchiveName
 
+// DefaultImportArchiveNameForTest exposes the production import archive name generator.
+var DefaultImportArchiveNameForTest = defaultImportArchiveName
+
 // RemoveNodeArchiveForTest exposes removeNodeArchive for white-box testing.
 var RemoveNodeArchiveForTest = (*Exporter).removeNodeArchive
 

pkg/svc/image/exporter.go

@@ -28,11 +28,11 @@ const (
 	ctrNamespaceArg = "--namespace=k8s.io"
 	// ctrImages is the ctr subcommand group for image operations.
 	ctrImages = "images"
-	// nodeCleanupTimeout bounds removing an export archive from a node after the
-	// command's context is done, so a cancelled export still cleans up.
+	// nodeCleanupTimeout bounds removing an export or import archive from a node
+	// after the command's context is done, so a cancelled operation still cleans up.
 	nodeCleanupTimeout = 30 * time.Second
-	// exportArchiveSuffixBytes is the random suffix length of an export archive name.
-	exportArchiveSuffixBytes = 8
+	// nodeArchiveSuffixBytes is the random suffix length of a node-side archive name.
+	nodeArchiveSuffixBytes = 8
 )
 
 // newExportArchiveName returns the file name of one export's archive on a node.
@@ -46,10 +46,15 @@ var newExportArchiveName = defaultExportArchiveName
 // defaultExportArchiveName returns a random node-side archive name, so concurrent
 // exports on the same node never write to the same file.
 func defaultExportArchiveName() string {
-	suffix := make([]byte, exportArchiveSuffixBytes)
+	return randomNodeArchiveName("ksail-images-export-")
+}
+
+// randomNodeArchiveName returns prefix followed by a random hex suffix and ".tar".
+func randomNodeArchiveName(prefix string) string {
+	suffix := make([]byte, nodeArchiveSuffixBytes)
 	_, _ = rand.Read(suffix)
 
-	return "ksail-images-export-" + hex.EncodeToString(suffix) + ".tar"
+	return prefix + hex.EncodeToString(suffix) + ".tar"
 }
 
 // Error definitions.
@@ -403,13 +408,7 @@ func (e *Exporter) writeFileFromTar(dstPath string, tarReader *tar.Reader) error
 	return nil
 }
 
-// removeNodeArchive deletes an export archive from a node. It always runs on a
-// context detached from the caller and bounded by nodeCleanupTimeout, so a
-// cancellation that lands while removal is starting cannot abort it and leave
-// the archive behind.
+// removeNodeArchive deletes an export archive from a node.
 func (e *Exporter) removeNodeArchive(ctx context.Context, nodeName, tmpPath string) {
-	cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), nodeCleanupTimeout)
-	defer cancel()
-
-	_, _ = e.executor.ExecInContainer(cleanupCtx, nodeName, []string{"rm", "-f", tmpPath})
+	e.executor.removeNodeArchive(ctx, nodeName, tmpPath)
 }

pkg/svc/image/importer.go

@@ -24,6 +24,19 @@ type ImportOptions struct {
 	InputPath string
 }
 
+// newImportArchiveName returns the file name of one import's archive on a node.
+// Each import gets its own name, so two imports on the same node never write to
+// or remove each other's archive. Tests replace it to keep the node-side path
+// predictable.
+//
+//nolint:gochecknoglobals // Test seam: tests pin the name so node-side commands stay exact.
+var newImportArchiveName = defaultImportArchiveName
+
+// defaultImportArchiveName returns a random node-side import archive name.
+func defaultImportArchiveName() string {
+	return randomNodeArchiveName("ksail-images-import-")
+}
+
 // Importer handles importing container images to cluster containerd.
 type Importer struct {
 	dockerClient dockerclient.Client
@@ -161,8 +174,12 @@ func (i *Importer) importImagesToNode(
 	inputPath string,
 	tmpBasePath string,
 ) error {
-	// Copy the tar file from host to container
-	tmpPath := tmpBasePath + "/ksail-images-import.tar"
+	// Copy the tar file from host to container. Each import gets its own archive
+	// name, and the archive is removed however the import ends, even after a
+	// partial copy or a cancellation.
+	tmpPath := tmpBasePath + "/" + newImportArchiveName()
+
+	defer i.executor.removeNodeArchive(ctx, nodeName, tmpPath)
 
 	err := i.copyToContainer(ctx, nodeName, inputPath, tmpPath)
 	if err != nil {
@@ -184,9 +201,6 @@ func (i *Importer) importImagesToNode(
 		return fmt.Errorf("ctr import failed: %w", err)
 	}
 
-	// Clean up temporary file in container
-	_, _ = i.executor.ExecInContainer(ctx, nodeName, []string{"rm", "-f", tmpPath})
-
 	return nil
 }
 

pkg/svc/image/importer_test.go

@@ -7,6 +7,8 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
+	"slices"
 	"testing"
 
 	v1alpha1 "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1"
@@ -24,6 +26,10 @@ var (
 	errImportCopyFailed = errors.New("copy to container failed")
 )
 
+// kindImportPath is the node-side import archive on a Kind node under the fixed
+// test archive name.
+const kindImportPath = "/root/ksail-images-import.tar"
+
 func TestNewImporter(t *testing.T) {
 	t.Parallel()
 
@@ -256,11 +262,11 @@ func TestImportSuccessVanilla(t *testing.T) {
 			Return(nil).Once()
 
 		// Mock exec for import command - Kind uses /root path
-		importPath := "/root/ksail-images-import.tar"
+		importPath := kindImportPath
 		setupImportExecMockForImporter(ctx, t, mockClient, nodeName, importPath)
 
 		// Mock exec for cleanup
-		setupExecMockForImporter(ctx, t, mockClient, nodeName)
+		setupCleanupMockForImporter(t, mockClient, nodeName, importPath)
 	}
 
 	importer := image.NewImporter(mockClient)
@@ -322,7 +328,7 @@ func TestImportSuccessK3s(t *testing.T) {
 		setupImportExecMockForImporter(ctx, t, mockClient, nodeName, "/tmp/ksail-images-import.tar")
 
 		// Mock exec for cleanup
-		setupExecMockForImporter(ctx, t, mockClient, nodeName)
+		setupCleanupMockForImporter(t, mockClient, nodeName, "/tmp/ksail-images-import.tar")
 	}
 
 	importer := image.NewImporter(mockClient)
@@ -364,6 +370,7 @@ func TestImportCopyToContainerFails(t *testing.T) {
 	mockClient.EXPECT().
 		CopyToContainer(ctx, "my-cluster-control-plane", "/root", mock.Anything, container.CopyToContainerOptions{}).
 		Return(errImportCopyFailed)
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
 
 	importer := image.NewImporter(mockClient)
 	err = importer.Import(
@@ -419,6 +426,7 @@ func TestImportExecFails(t *testing.T) {
 	mockClient.EXPECT().
 		ContainerExecInspect(ctx, execID).
 		Return(container.ExecInspect{ExitCode: 1}, nil)
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
 
 	importer := image.NewImporter(mockClient)
 	err = importer.Import(
@@ -472,7 +480,7 @@ func TestImportEmptyProvider(t *testing.T) {
 	)
 
 	// Mock exec for cleanup
-	setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane")
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
 
 	importer := image.NewImporter(mockClient)
 	err = importer.Import(ctx, "my-cluster", v1alpha1.DistributionVanilla, "", image.ImportOptions{
@@ -520,12 +528,13 @@ func TestImportMultipleNodesPartialFailure(t *testing.T) {
 		"my-cluster-control-plane",
 		"/root/ksail-images-import.tar",
 	)
-	setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane")
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
 
 	// Second node fails during copy
 	mockClient.EXPECT().
 		CopyToContainer(ctx, "my-cluster-worker", "/root", mock.Anything, container.CopyToContainerOptions{}).
 		Return(errImportCopyFailed).Once()
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-worker", kindImportPath)
 
 	importer := image.NewImporter(mockClient)
 	err = importer.Import(
@@ -579,7 +588,7 @@ func TestImportLargeFile(t *testing.T) {
 		"my-cluster-control-plane",
 		"/root/ksail-images-import.tar",
 	)
-	setupExecMockForImporter(ctx, t, mockClient, "my-cluster-control-plane")
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
 
 	importer := image.NewImporter(mockClient)
 	err = importer.Import(
@@ -595,27 +604,32 @@ func TestImportLargeFile(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// setupExecMockForImporter is a helper to set up ContainerExec* mocks for simple cases.
-func setupExecMockForImporter(
-	ctx context.Context,
+// setupCleanupMockForImporter expects the import archive at archivePath to be
+// removed from containerName exactly once, on a live context detached from the
+// caller, so cleanup also runs after a failed or cancelled import.
+func setupCleanupMockForImporter(
 	t *testing.T,
 	mockClient *docker.MockAPIClient,
 	containerName string,
+	archivePath string,
 ) {
 	t.Helper()
 
-	execID := "exec-" + containerName
+	execID := "exec-cleanup-" + containerName
 
 	mockClient.EXPECT().
-		ContainerExecCreate(ctx, containerName, mock.Anything).
+		ContainerExecCreate(detachedCleanupContext(), containerName,
+			mock.MatchedBy(func(opts container.ExecOptions) bool {
+				return slices.Equal(opts.Cmd, []string{"rm", "-f", archivePath})
+			})).
 		Return(container.ExecCreateResponse{ID: execID}, nil).Once()
 
 	mockClient.EXPECT().
-		ContainerExecAttach(ctx, execID, container.ExecStartOptions{}).
+		ContainerExecAttach(detachedCleanupContext(), execID, container.ExecStartOptions{}).
 		Return(mockDockerStreamResponse("", ""), nil).Once()
 
 	mockClient.EXPECT().
-		ContainerExecInspect(ctx, execID).
+		ContainerExecInspect(detachedCleanupContext(), execID).
 		Return(container.ExecInspect{ExitCode: 0}, nil).Once()
 }
 
@@ -652,3 +666,65 @@ func setupImportExecMockForImporter(
 		ContainerExecInspect(ctx, execID).
 		Return(container.ExecInspect{ExitCode: 0}, nil).Once()
 }
+
+// Two imports on the same node must never share an archive, so every generated
+// name is distinct.
+func TestDefaultImportArchiveNamesAreDistinct(t *testing.T) {
+	t.Parallel()
+
+	pattern := regexp.MustCompile(`^ksail-images-import-[0-9a-f]{16}\.tar$`)
+	seen := make(map[string]struct{})
+
+	for range 64 {
+		name := image.DefaultImportArchiveNameForTest()
+		if !pattern.MatchString(name) {
+			t.Fatalf("archive name %q does not match %s", name, pattern)
+		}
+
+		if _, duplicate := seen[name]; duplicate {
+			t.Fatalf("archive name %q was generated twice", name)
+		}
+
+		seen[name] = struct{}{}
+	}
+}
+
+// A cancelled import must still remove its archive from the node, so cleanup
+// runs on a live context after the caller's context is done.
+func TestImportCancelledStillRemovesArchive(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	mockClient := docker.NewMockAPIClient(t)
+
+	inputPath := filepath.Join(t.TempDir(), "images.tar")
+	require.NoError(t, os.WriteFile(inputPath, []byte("fake tar content"), 0o600))
+
+	mockClient.EXPECT().
+		ContainerList(ctx, mock.Anything).
+		Return([]container.Summary{
+			{
+				Names:  []string{"/my-cluster-control-plane"},
+				Labels: map[string]string{"io.x-k8s.kind.role": "control-plane"},
+			},
+		}, nil)
+
+	mockClient.EXPECT().
+		CopyToContainer(ctx, "my-cluster-control-plane", "/root", mock.Anything, container.CopyToContainerOptions{}).
+		RunAndReturn(func(context.Context, string, string, io.Reader, container.CopyToContainerOptions) error {
+			cancel()
+
+			return context.Canceled
+		})
+	setupCleanupMockForImporter(t, mockClient, "my-cluster-control-plane", kindImportPath)
+
+	err := image.NewImporter(mockClient).Import(
+		ctx,
+		"my-cluster",
+		v1alpha1.DistributionVanilla,
+		v1alpha1.ProviderDocker,
+		image.ImportOptions{InputPath: inputPath},
+	)
+
+	require.ErrorIs(t, err, context.Canceled)
+}