From 8c61b5ae779a2b381de61cb154f921cadfc5cb54 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 09:49:43 +0000 Subject: [PATCH 1/2] Remove images from earlier deploys after a successful deploy (closes #216) After a successful deploy, upaas now removes the app's images other than the one the running container uses and the previous one, which rollback starts. Removing an image also removes the untagged images it was built on unless another image still needs them. Images left by other stages of a multi-stage build are not tied to the app and stay. Model: opus-5-5 --- TODO.md | 3 ++ internal/docker/client.go | 25 +++++++++ internal/service/deploy/deploy.go | 53 +++++++++++++++++++ internal/service/deploy/deploy_images_test.go | 48 +++++++++++++++++ internal/service/deploy/export_test.go | 5 ++ 5 files changed, 134 insertions(+) create mode 100644 internal/service/deploy/deploy_images_test.go diff --git a/TODO.md b/TODO.md index 27bd714..9a7114a 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,9 @@ regress. # Completed Steps +- 2026-09-23: After a successful deploy, upaas removes the app's images other + than the running one and the one rollback would use, together with the + untagged images they were built on (#216). - 2026-09-23: The git clone container is now removed together with its anonymous volume (the `alpine/git` image declares one at `/git`), so a deploy no longer leaves a Docker volume behind (#215). diff --git a/internal/docker/client.go b/internal/docker/client.go index 9e6abc5..eee6206 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -537,6 +537,31 @@ func (c *Client) RemoveImage(ctx context.Context, imageID ImageID) error { return nil } +// ListImageIDs returns the IDs of all images tagged in the given repository, +// such as "upaas-myapp". +func (c *Client) ListImageIDs( + ctx context.Context, + repository string, +) ([]ImageID, error) { + if c.docker == nil { + return nil, ErrNotConnected + } + + images, err := c.docker.ImageList(ctx, image.ListOptions{ + Filters: filters.NewArgs(filters.Arg("reference", repository)), + }) + if err != nil { + return nil, fmt.Errorf("failed to list images: %w", err) + } + + ids := make([]ImageID, 0, len(images)) + for _, img := range images { + ids = append(ids, ImageID(img.ID)) + } + + return ids, nil +} + func (c *Client) performBuild( ctx context.Context, opts BuildImageOptions, diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 79343e2..aa14790 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -534,6 +534,8 @@ func (svc *Service) runBuildAndDeploy( return err } + svc.removeUnusedImages(bgCtx, app, deployment) + // Use context.WithoutCancel to ensure health check completes even if // the parent context is cancelled (e.g., HTTP request ends). go svc.checkHealthAfterDelay(bgCtx, app, deployment) @@ -714,6 +716,57 @@ func (svc *Service) checkCancelled( return ErrDeployCancelled } +// removeUnusedImages removes the app's images (tagged upaas-: +// by buildImage) except the one the running container uses and the one +// Rollback would start. Removing an image also removes the untagged images +// it was built on, unless another image still needs them. +func (svc *Service) removeUnusedImages( + ctx context.Context, + app *models.App, + deployment *models.Deployment, +) { + images, err := svc.docker.ListImageIDs(ctx, "upaas-"+app.Name) + if err != nil { + svc.log.Error("failed to list app images", "error", err, "app", app.Name) + + return + } + + for _, imageID := range unusedImages(images, app) { + removeErr := svc.docker.RemoveImage(ctx, imageID) + if removeErr != nil { + svc.log.Error("failed to remove old image", + "error", removeErr, "app", app.Name, "image", imageID) + _ = deployment.AppendLog( + ctx, + "WARNING: failed to remove old image "+ + imageID.String()+": "+removeErr.Error(), + ) + + continue + } + + _ = deployment.AppendLog(ctx, "Removed old image: "+imageID.String()) + } +} + +// unusedImages returns the images that are neither the app's current image +// nor its previous image, which Rollback uses. +func unusedImages(images []docker.ImageID, app *models.App) []docker.ImageID { + var unused []docker.ImageID + + for _, imageID := range images { + if imageID.String() == app.ImageID.String || + imageID.String() == app.PreviousImageID.String { + continue + } + + unused = append(unused, imageID) + } + + return unused +} + // cleanupCancelledDeploy removes orphan resources left by a cancelled deployment. func (svc *Service) cleanupCancelledDeploy( ctx context.Context, diff --git a/internal/service/deploy/deploy_images_test.go b/internal/service/deploy/deploy_images_test.go new file mode 100644 index 0000000..0ece5d0 --- /dev/null +++ b/internal/service/deploy/deploy_images_test.go @@ -0,0 +1,48 @@ +package deploy_test + +import ( + "database/sql" + "testing" + + "github.com/stretchr/testify/assert" + + "sneak.berlin/go/upaas/internal/docker" + "sneak.berlin/go/upaas/internal/models" + "sneak.berlin/go/upaas/internal/service/deploy" +) + +const currentImage = "sha256:current" + +func TestUnusedImages_KeepsCurrentAndRollbackImage(t *testing.T) { + t.Parallel() + + app := &models.App{ + ImageID: sql.NullString{String: currentImage, Valid: true}, + PreviousImageID: sql.NullString{String: "sha256:previous", Valid: true}, + } + images := []docker.ImageID{ + "sha256:oldest", + "sha256:previous", + "sha256:older", + currentImage, + } + + assert.Equal(t, + []docker.ImageID{"sha256:oldest", "sha256:older"}, + deploy.UnusedImages(images, app), + ) +} + +func TestUnusedImages_FirstDeployHasNoRollbackImage(t *testing.T) { + t.Parallel() + + app := &models.App{ + ImageID: sql.NullString{String: currentImage, Valid: true}, + } + images := []docker.ImageID{currentImage, "sha256:failed"} + + assert.Equal(t, + []docker.ImageID{"sha256:failed"}, + deploy.UnusedImages(images, app), + ) +} diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index 1d86a25..dca8cfe 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -90,6 +90,11 @@ func (svc *Service) GetBuildDirExported(appName string) string { return svc.GetBuildDir(appName) } +// UnusedImages exposes unusedImages for testing. +func UnusedImages(images []docker.ImageID, app *models.App) []docker.ImageID { + return unusedImages(images, app) +} + // BuildContainerOptionsExported exposes buildContainerOptions for testing. func (svc *Service) BuildContainerOptionsExported( ctx context.Context, -- 2.54.0 From 4c6d3f464d12401c20f30031ecb809f20b10c183 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 10:09:47 +0000 Subject: [PATCH 2/2] Remove old images by tag, and test the step after a deploy (closes #216) Old images are now removed by their upaas-: tag, without force, so Docker deletes an image only when no other tag (such as another app's build of the same source) and no container still uses it. Removing by ID with force could delete another app's rollback image. The step after a deploy (record the new image, keep the replaced one for rollback, remove the rest) is its own function, tested against a fake Docker API, so the test fails if the removal is dropped or runs before the images are updated. Model: opus-5-5 --- internal/docker/client.go | 38 +++++- internal/service/deploy/deploy.go | 78 ++++++------ internal/service/deploy/deploy_images_test.go | 120 +++++++++++++----- internal/service/deploy/export_test.go | 11 +- 4 files changed, 170 insertions(+), 77 deletions(-) diff --git a/internal/docker/client.go b/internal/docker/client.go index eee6206..f348d33 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -537,12 +537,13 @@ func (c *Client) RemoveImage(ctx context.Context, imageID ImageID) error { return nil } -// ListImageIDs returns the IDs of all images tagged in the given repository, -// such as "upaas-myapp". -func (c *Client) ListImageIDs( +// ListImageTags returns the tags in the given repository, such as +// "upaas-myapp:12" in "upaas-myapp", each with the ID of its image. +// Tags the same image has in other repositories are left out. +func (c *Client) ListImageTags( ctx context.Context, repository string, -) ([]ImageID, error) { +) (map[string]ImageID, error) { if c.docker == nil { return nil, ErrNotConnected } @@ -554,12 +555,35 @@ func (c *Client) ListImageIDs( return nil, fmt.Errorf("failed to list images: %w", err) } - ids := make([]ImageID, 0, len(images)) + tags := make(map[string]ImageID) + for _, img := range images { - ids = append(ids, ImageID(img.ID)) + for _, tag := range img.RepoTags { + if strings.HasPrefix(tag, repository+":") { + tags[tag] = ImageID(img.ID) + } + } } - return ids, nil + return tags, nil +} + +// RemoveImageTag removes a tag such as "upaas-myapp:12", without force. +// Docker then deletes the image, and the untagged images it was built on, +// only if no other tag and no container still uses it. +func (c *Client) RemoveImageTag(ctx context.Context, tag string) error { + if c.docker == nil { + return ErrNotConnected + } + + _, err := c.docker.ImageRemove(ctx, tag, image.RemoveOptions{ + PruneChildren: true, + }) + if err != nil && !client.IsErrNotFound(err) { + return fmt.Errorf("failed to remove image tag %s: %w", tag, err) + } + + return nil } func (c *Client) performBuild( diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index aa14790..e2e1d30 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -9,8 +9,10 @@ import ( "errors" "fmt" "log/slog" + "maps" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -524,18 +526,11 @@ func (svc *Service) runBuildAndDeploy( return err } - // Save current image as previous before updating to new one - if app.ImageID.Valid && app.ImageID.String != "" { - app.PreviousImageID = app.ImageID - } - - err = svc.updateAppRunning(bgCtx, app, imageID) + err = svc.recordDeployedImage(bgCtx, app, deployment, imageID) if err != nil { return err } - svc.removeUnusedImages(bgCtx, app, deployment) - // Use context.WithoutCancel to ensure health check completes even if // the parent context is cancelled (e.g., HTTP request ends). go svc.checkHealthAfterDelay(bgCtx, app, deployment) @@ -716,57 +711,68 @@ func (svc *Service) checkCancelled( return ErrDeployCancelled } -// removeUnusedImages removes the app's images (tagged upaas-: -// by buildImage) except the one the running container uses and the one -// Rollback would start. Removing an image also removes the untagged images -// it was built on, unless another image still needs them. +// recordDeployedImage runs once the new container has started: it makes +// imageID the app's current image, keeps the replaced one as the previous +// image for Rollback, and then removes the app's other images. +func (svc *Service) recordDeployedImage( + ctx context.Context, + app *models.App, + deployment *models.Deployment, + imageID docker.ImageID, +) error { + // Save current image as previous before updating to new one + if app.ImageID.Valid && app.ImageID.String != "" { + app.PreviousImageID = app.ImageID + } + + err := svc.updateAppRunning(ctx, app, imageID) + if err != nil { + return err + } + + svc.removeUnusedImages(ctx, app, deployment) + + return nil +} + +// removeUnusedImages removes the app's tags (upaas-:, set by +// buildImage) except those of the image the running container uses and the +// one Rollback would start. Docker deletes an image only once no other tag, +// such as another app's, and no container still uses it. func (svc *Service) removeUnusedImages( ctx context.Context, app *models.App, deployment *models.Deployment, ) { - images, err := svc.docker.ListImageIDs(ctx, "upaas-"+app.Name) + tags, err := svc.docker.ListImageTags(ctx, "upaas-"+app.Name) if err != nil { svc.log.Error("failed to list app images", "error", err, "app", app.Name) return } - for _, imageID := range unusedImages(images, app) { - removeErr := svc.docker.RemoveImage(ctx, imageID) + for _, tag := range slices.Sorted(maps.Keys(tags)) { + imageID := tags[tag].String() + if imageID == app.ImageID.String || imageID == app.PreviousImageID.String { + continue + } + + removeErr := svc.docker.RemoveImageTag(ctx, tag) if removeErr != nil { svc.log.Error("failed to remove old image", - "error", removeErr, "app", app.Name, "image", imageID) + "error", removeErr, "app", app.Name, "tag", tag) _ = deployment.AppendLog( ctx, - "WARNING: failed to remove old image "+ - imageID.String()+": "+removeErr.Error(), + "WARNING: failed to remove old image "+tag+": "+removeErr.Error(), ) continue } - _ = deployment.AppendLog(ctx, "Removed old image: "+imageID.String()) + _ = deployment.AppendLog(ctx, "Removed old image: "+tag) } } -// unusedImages returns the images that are neither the app's current image -// nor its previous image, which Rollback uses. -func unusedImages(images []docker.ImageID, app *models.App) []docker.ImageID { - var unused []docker.ImageID - - for _, imageID := range images { - if imageID.String() == app.ImageID.String || - imageID.String() == app.PreviousImageID.String { - continue - } - - unused = append(unused, imageID) - } - - return unused -} - // cleanupCancelledDeploy removes orphan resources left by a cancelled deployment. func (svc *Service) cleanupCancelledDeploy( ctx context.Context, diff --git a/internal/service/deploy/deploy_images_test.go b/internal/service/deploy/deploy_images_test.go index 0ece5d0..086ccb3 100644 --- a/internal/service/deploy/deploy_images_test.go +++ b/internal/service/deploy/deploy_images_test.go @@ -1,48 +1,106 @@ package deploy_test import ( + "context" "database/sql" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/fx/fxtest" + "sneak.berlin/go/upaas/internal/config" + "sneak.berlin/go/upaas/internal/database" "sneak.berlin/go/upaas/internal/docker" + "sneak.berlin/go/upaas/internal/logger" "sneak.berlin/go/upaas/internal/models" "sneak.berlin/go/upaas/internal/service/deploy" ) -const currentImage = "sha256:current" - -func TestUnusedImages_KeepsCurrentAndRollbackImage(t *testing.T) { +// TestRecordDeployedImageRemovesOldImages runs the step after a deploy +// against a fake Docker API. Image one is also tagged for another app, +// image two was the previous image, three the current one, four is new. +func TestRecordDeployedImageRemovesOldImages(t *testing.T) { t.Parallel() - app := &models.App{ - ImageID: sql.NullString{String: currentImage, Valid: true}, - PreviousImageID: sql.NullString{String: "sha256:previous", Valid: true}, - } - images := []docker.ImageID{ - "sha256:oldest", - "sha256:previous", - "sha256:older", - currentImage, - } - - assert.Equal(t, - []docker.ImageID{"sha256:oldest", "sha256:older"}, - deploy.UnusedImages(images, app), - ) -} - -func TestUnusedImages_FirstDeployHasNoRollbackImage(t *testing.T) { - t.Parallel() - - app := &models.App{ - ImageID: sql.NullString{String: currentImage, Valid: true}, - } - images := []docker.ImageID{currentImage, "sha256:failed"} - - assert.Equal(t, - []docker.ImageID{"sha256:failed"}, - deploy.UnusedImages(images, app), + var ( + mu sync.Mutex + removed []string + forced bool ) + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case r.Method == http.MethodDelete: + _, name, _ := strings.Cut(r.URL.Path, "/images/") + + mu.Lock() + + removed = append(removed, name) + forced = forced || r.URL.Query().Get("force") != "" + mu.Unlock() + + _, _ = w.Write([]byte(`[]`)) + case strings.HasSuffix(r.URL.Path, "/images/json"): + _, _ = w.Write([]byte(`[ + {"Id":"sha256:one","RepoTags":["upaas-myapp:1","upaas-otherapp:7"]}, + {"Id":"sha256:two","RepoTags":["upaas-myapp:2"]}, + {"Id":"sha256:three","RepoTags":["upaas-myapp:3"]}, + {"Id":"sha256:four","RepoTags":["upaas-myapp:4"]} + ]`)) + default: + _, _ = w.Write([]byte(`{}`)) + } + }, + )) + t.Cleanup(srv.Close) + + log := slog.New(slog.NewTextHandler(os.Stderr, nil)) + lifecycle := fxtest.NewLifecycle(t) + + dockerClient, err := docker.New(lifecycle, docker.Params{ + Logger: logger.NewForTest(log), + Config: &config.Config{DockerHost: "tcp://" + srv.Listener.Addr().String()}, + }) + require.NoError(t, err) + + lifecycle.RequireStart() + t.Cleanup(lifecycle.RequireStop) + + db := database.NewTestDatabase(t) + ctx := context.Background() + + app := models.NewApp(db) + app.ID = "myapp-id" + app.Name = "myapp" + app.ImageID = sql.NullString{String: "sha256:three", Valid: true} + app.PreviousImageID = sql.NullString{String: "sha256:two", Valid: true} + require.NoError(t, app.Save(ctx)) + + deployment := models.NewDeployment(db) + deployment.AppID = app.ID + require.NoError(t, deployment.Save(ctx)) + + svc := deploy.NewTestServiceWithConfig(log, &config.Config{}, dockerClient) + + err = svc.RecordDeployedImage(ctx, app, deployment, "sha256:four") + require.NoError(t, err) + + assert.Equal(t, "sha256:four", app.ImageID.String) + assert.Equal(t, "sha256:three", app.PreviousImageID.String) + + mu.Lock() + defer mu.Unlock() + + assert.Equal(t, []string{"upaas-myapp:1", "upaas-myapp:2"}, removed) + assert.False(t, forced, "old image tags must be removed without force") } diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index dca8cfe..5b68733 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -90,9 +90,14 @@ func (svc *Service) GetBuildDirExported(appName string) string { return svc.GetBuildDir(appName) } -// UnusedImages exposes unusedImages for testing. -func UnusedImages(images []docker.ImageID, app *models.App) []docker.ImageID { - return unusedImages(images, app) +// RecordDeployedImage exposes recordDeployedImage for testing. +func (svc *Service) RecordDeployedImage( + ctx context.Context, + app *models.App, + deployment *models.Deployment, + imageID docker.ImageID, +) error { + return svc.recordDeployedImage(ctx, app, deployment, imageID) } // BuildContainerOptionsExported exposes buildContainerOptions for testing. -- 2.54.0