From a75f9776036e346a59232bab5e0dda2ffef2a45e Mon Sep 17 00:00:00 2001 From: Matthias Wessendorf Date: Fri, 12 Jun 2026 10:22:32 +0200 Subject: [PATCH 1/2] OCPBUGS-88450: Use fixed artifacts directory to prevent stale temp dir accumulation The downloads server previously created a new random temp directory via os.MkdirTemp on each startup. When Kubernetes restarts the container (without deleting the pod), the emptyDir volume persists and each restart leaves behind ~3.1G of orphaned artifacts. The defer-based cleanup in main() only runs on graceful exit, not on SIGKILL. Switch to a fixed, well-known path (/tmp/artifacts) and clean it on startup with os.RemoveAll before re-creating it. This guarantees at most one copy of the artifacts exists at any time. Signed-off-by: Matthias Wessendorf Co-Authored-By: Claude Opus 4.6 Signed-off-by: Matthias Wessendorf --- cmd/downloads/config/downloads_config.go | 12 ++++++++---- cmd/downloads/main.go | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/cmd/downloads/config/downloads_config.go b/cmd/downloads/config/downloads_config.go index 1719cbf7041..c0575873131 100644 --- a/cmd/downloads/config/downloads_config.go +++ b/cmd/downloads/config/downloads_config.go @@ -60,6 +60,7 @@ type DownloadsServerConfig struct { const indexFileName = "index.html" const pathToOCLicense = "/usr/share/openshift/LICENSE" const ocLicenseFile = "oc-license" +const defaultArtifactsDir = "/tmp/artifacts" // template used to generate the HTML file with links to artifacts const templateStringHTML = ` @@ -109,11 +110,14 @@ func loadArtifactsSpec(path string) ([]ArtifactSpec, error) { // NewDownloadsServerConfig creates a new ArtifactsConfig object func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerConfig, error) { - tempDir, err := os.MkdirTemp("", "artifacts") - if err != nil { - return nil, err + tempDir := defaultArtifactsDir + if err := os.RemoveAll(tempDir); err != nil { + return nil, fmt.Errorf("failed to clean up stale artifacts directory: %w", err) + } + if err := os.MkdirAll(tempDir, 0755); err != nil { + return nil, fmt.Errorf("failed to create artifacts directory: %w", err) } - klog.Info("Create temporary directory for artifacts") + klog.Info("Created artifacts directory (cleaned up any stale data from previous run)") specs, err := loadArtifactsSpec(specsFilePath) if err != nil { return nil, err diff --git a/cmd/downloads/main.go b/cmd/downloads/main.go index 53598a36d49..025c1b422fe 100644 --- a/cmd/downloads/main.go +++ b/cmd/downloads/main.go @@ -28,7 +28,7 @@ func main() { klog.Fatalf("Failed to configure downloads server config: %v", err) os.Exit(1) } - defer os.RemoveAll(downloadsServerConfig.TempDir) + downloadsServerConfig.CreateArchivesInBackground() // Listen for incoming connections klog.Infof("Server started. Listening on http://0.0.0.0:%s", downloadsServerConfig.Port) From 37923dd71cc04c37ba078dfe2074df2fa0c52a5a Mon Sep 17 00:00:00 2001 From: Matthias Wessendorf Date: Wed, 17 Jun 2026 11:46:48 +0200 Subject: [PATCH 2/2] OCPBUGS-88450: Clean up old random temp dirs and add unit tests Address review feedback: - Use filepath.Glob("/tmp/artifacts*") to also remove stale random temp directories left by the old os.MkdirTemp code (e.g. /tmp/artifacts1234567890). The first deployment with the fixed path would otherwise leave those orphaned dirs behind. - Add unit tests for NewDownloadsServerConfig directory handling: 1. Clean start creates the directory and populates it 2. Restart with stale data cleans up and recreates correctly 3. Old random temp dirs (/tmp/artifacts*) are also cleaned up Signed-off-by: Matthias Wessendorf --- cmd/downloads/config/downloads_config.go | 7 +- cmd/downloads/config/downloads_config_test.go | 110 ++++++++++++++++++ cmd/downloads/main.go | 2 - 3 files changed, 114 insertions(+), 5 deletions(-) create mode 100644 cmd/downloads/config/downloads_config_test.go diff --git a/cmd/downloads/config/downloads_config.go b/cmd/downloads/config/downloads_config.go index c0575873131..8e76e91f459 100644 --- a/cmd/downloads/config/downloads_config.go +++ b/cmd/downloads/config/downloads_config.go @@ -111,13 +111,14 @@ func loadArtifactsSpec(path string) ([]ArtifactSpec, error) { // NewDownloadsServerConfig creates a new ArtifactsConfig object func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerConfig, error) { tempDir := defaultArtifactsDir - if err := os.RemoveAll(tempDir); err != nil { - return nil, fmt.Errorf("failed to clean up stale artifacts directory: %w", err) + matches, _ := filepath.Glob("/tmp/artifacts*") + for _, match := range matches { + os.RemoveAll(match) } if err := os.MkdirAll(tempDir, 0755); err != nil { return nil, fmt.Errorf("failed to create artifacts directory: %w", err) } - klog.Info("Created artifacts directory (cleaned up any stale data from previous run)") + klog.Info("Created artifacts directory (cleaned up any stale data from previous runs)") specs, err := loadArtifactsSpec(specsFilePath) if err != nil { return nil, err diff --git a/cmd/downloads/config/downloads_config_test.go b/cmd/downloads/config/downloads_config_test.go new file mode 100644 index 00000000000..d6422f6ab66 --- /dev/null +++ b/cmd/downloads/config/downloads_config_test.go @@ -0,0 +1,110 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +func writeTestSpecsFile(t *testing.T, binaryPath string) string { + t.Helper() + specContent := []byte("defaultArtifactsConfig:\n - arch: amd64\n operatingSystem: linux\n path: " + binaryPath + "\n") + specFile := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(specFile, specContent, 0644); err != nil { + t.Fatalf("failed to write spec file: %v", err) + } + return specFile +} + +func TestNewDownloadsServerConfig_CreatesDirectory(t *testing.T) { + os.RemoveAll(defaultArtifactsDir) + t.Cleanup(func() { os.RemoveAll(defaultArtifactsDir) }) + + srcDir := t.TempDir() + binaryPath := filepath.Join(srcDir, "oc") + if err := os.WriteFile(binaryPath, []byte("fake-binary"), 0755); err != nil { + t.Fatalf("failed to write fake binary: %v", err) + } + + cfg, err := NewDownloadsServerConfig(0, writeTestSpecsFile(t, binaryPath)) + if err != nil { + t.Fatalf("NewDownloadsServerConfig() error: %v", err) + } + + if cfg.TempDir != defaultArtifactsDir { + t.Errorf("TempDir = %q, want %q", cfg.TempDir, defaultArtifactsDir) + } + if _, err := os.Stat(defaultArtifactsDir); err != nil { + t.Errorf("artifacts directory does not exist: %v", err) + } + if _, err := os.Stat(filepath.Join(defaultArtifactsDir, indexFileName)); err != nil { + t.Errorf("index.html not created: %v", err) + } +} + +func TestNewDownloadsServerConfig_CleansStaleData(t *testing.T) { + os.RemoveAll(defaultArtifactsDir) + t.Cleanup(func() { os.RemoveAll(defaultArtifactsDir) }) + + if err := os.MkdirAll(defaultArtifactsDir, 0755); err != nil { + t.Fatalf("failed to create stale dir: %v", err) + } + staleFile := filepath.Join(defaultArtifactsDir, "stale-file.txt") + if err := os.WriteFile(staleFile, []byte("stale"), 0644); err != nil { + t.Fatalf("failed to write stale file: %v", err) + } + + srcDir := t.TempDir() + binaryPath := filepath.Join(srcDir, "oc") + if err := os.WriteFile(binaryPath, []byte("fake-binary"), 0755); err != nil { + t.Fatalf("failed to write fake binary: %v", err) + } + + _, err := NewDownloadsServerConfig(0, writeTestSpecsFile(t, binaryPath)) + if err != nil { + t.Fatalf("NewDownloadsServerConfig() error: %v", err) + } + + if _, err := os.Stat(staleFile); !os.IsNotExist(err) { + t.Errorf("stale file should have been cleaned up, but still exists") + } +} + +func TestNewDownloadsServerConfig_CleansOldRandomTempDirs(t *testing.T) { + os.RemoveAll(defaultArtifactsDir) + t.Cleanup(func() { + os.RemoveAll(defaultArtifactsDir) + matches, _ := filepath.Glob("/tmp/artifacts*test*") + for _, m := range matches { + os.RemoveAll(m) + } + }) + + oldDirs := []string{ + "/tmp/artifacts1234test", + "/tmp/artifacts5678test", + } + for _, d := range oldDirs { + if err := os.MkdirAll(d, 0755); err != nil { + t.Fatalf("failed to create old temp dir %s: %v", d, err) + } + os.WriteFile(filepath.Join(d, "leftover"), []byte("data"), 0644) + } + + srcDir := t.TempDir() + binaryPath := filepath.Join(srcDir, "oc") + if err := os.WriteFile(binaryPath, []byte("fake-binary"), 0755); err != nil { + t.Fatalf("failed to write fake binary: %v", err) + } + + _, err := NewDownloadsServerConfig(0, writeTestSpecsFile(t, binaryPath)) + if err != nil { + t.Fatalf("NewDownloadsServerConfig() error: %v", err) + } + + for _, d := range oldDirs { + if _, err := os.Stat(d); !os.IsNotExist(err) { + t.Errorf("old temp dir %s should have been cleaned up, but still exists", d) + } + } +} diff --git a/cmd/downloads/main.go b/cmd/downloads/main.go index 025c1b422fe..9163ae3aa1b 100644 --- a/cmd/downloads/main.go +++ b/cmd/downloads/main.go @@ -28,8 +28,6 @@ func main() { klog.Fatalf("Failed to configure downloads server config: %v", err) os.Exit(1) } - downloadsServerConfig.CreateArchivesInBackground() - // Listen for incoming connections klog.Infof("Server started. Listening on http://0.0.0.0:%s", downloadsServerConfig.Port)