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.