diff --git a/TODO.md b/TODO.md index 1dc2403..7a0c2fe 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,15 @@ regress. # Completed Steps +- 2026-10-01: Built images are tagged `upaas-:`, git's short + form of the commit built, instead of the deployment number. A redeploy of a + commit gives its tag to the new image; the old one is kept while the app runs + it or Rollback would start it, then removed by its ID, found among the images + the app's deployments recorded. The removal of old images now keeps every + image any app runs or would roll back to. A deployment's commit is now saved + when it is updated, so manual deploys keep the commit read from the clone + (#239). + - 2026-10-01: Two database writes at the same moment no longer fail with "database is locked": each transaction now takes the write lock when it begins and waits up to 5 seconds for another writer to finish (#253). diff --git a/internal/docker/client.go b/internal/docker/client.go index a00f17c..1247e0d 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -505,6 +505,7 @@ type cloneConfig struct { type CloneResult struct { Output string // Combined stdout/stderr from git clone CommitSHA string // The HEAD commit SHA after clone/checkout + ShortSHA string // git's short form of CommitSHA (git rev-parse --short) } // CloneRepo clones a git repository using SSH and optionally checks out a @@ -567,7 +568,7 @@ func (c *Client) RemoveImage(ctx context.Context, imageID ImageID) error { } // ListImageTags returns the tags in the given repository, such as -// "upaas-myapp:12" in "upaas-myapp", each with the ID of its image. +// "upaas-myapp:1a2b3c4" 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, @@ -597,19 +598,44 @@ func (c *Client) ListImageTags( 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 { +// ListUntaggedImages returns the IDs of the images that have no tag, such +// as one whose tag a later build gave to the image it built. +func (c *Client) ListUntaggedImages(ctx context.Context) ([]ImageID, error) { + if c.docker == nil { + return nil, ErrNotConnected + } + + images, err := c.docker.ImageList(ctx, image.ListOptions{ + Filters: filters.NewArgs(filters.Arg("dangling", "true")), + }) + if err != nil { + return nil, fmt.Errorf("failed to list untagged images: %w", err) + } + + imageIDs := make([]ImageID, 0, len(images)) + + for _, img := range images { + imageIDs = append(imageIDs, ImageID(img.ID)) + } + + return imageIDs, nil +} + +// RemoveImageTag removes the tag name, such as "upaas-myapp:1a2b3c4", +// 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. If name +// is instead the ID of an untagged image, it removes that image unless a +// container uses it. +func (c *Client) RemoveImageTag(ctx context.Context, name string) error { if c.docker == nil { return ErrNotConnected } - _, err := c.docker.ImageRemove(ctx, tag, image.RemoveOptions{ + _, err := c.docker.ImageRemove(ctx, name, image.RemoveOptions{ PruneChildren: true, }) if err != nil && !client.IsErrNotFound(err) { - return fmt.Errorf("failed to remove image tag %s: %w", tag, err) + return fmt.Errorf("failed to remove image %s: %w", name, err) } return nil @@ -905,11 +931,13 @@ func (c *Client) createGitContainer( // Clone without depth limit so we can checkout any commit, then checkout specific SHA script = `git clone --branch "$CLONE_BRANCH" "$CLONE_URL" /repo` + ` && cd /repo && git checkout "$CLONE_SHA"` + - ` && echo COMMIT:$(git rev-parse HEAD)` + ` && echo COMMIT:$(git rev-parse HEAD)` + + ` && echo SHORT_SHA:$(git rev-parse --short HEAD)` } else { // Shallow clone of branch HEAD, then output commit SHA script = `git clone --depth 1 --branch "$CLONE_BRANCH" "$CLONE_URL" /repo` + - ` && cd /repo && echo COMMIT:$(git rev-parse HEAD)` + ` && cd /repo && echo COMMIT:$(git rev-parse HEAD)` + + ` && echo SHORT_SHA:$(git rev-parse --short HEAD)` } env := []string{ @@ -987,23 +1015,37 @@ func (c *Client) runGitClone( ) } - // Parse commit SHA from output (looks for "COMMIT:" line) - commitSHA := parseCommitSHA(logs) + // Parse the commit from the "COMMIT:" and "SHORT_SHA:" lines. + result := &CloneResult{ + Output: logs, + CommitSHA: parseCommitSHA(logs, commitMarker), + ShortSHA: parseCommitSHA(logs, shortSHAMarker), + } - return &CloneResult{Output: logs, CommitSHA: commitSHA}, nil + // The short hash names the image the deploy builds. + if result.ShortSHA == "" { + return nil, fmt.Errorf("%w: no short commit hash in its output: %s", + ErrGitCloneFailed, logs) + } + + return result, nil } } -// commitMarker is the prefix used to identify commit SHA in clone output. -const commitMarker = "COMMIT:" +// Prefixes of the lines in the clone output that carry the commit checked +// out, in full and in git's short form. +const ( + commitMarker = "COMMIT:" + shortSHAMarker = "SHORT_SHA:" +) -// parseCommitSHA extracts the commit SHA from git clone output. -// It looks for a line starting with "COMMIT:" and returns the SHA after it. -func parseCommitSHA(output string) string { +// parseCommitSHA extracts a commit SHA from git clone output. +// It looks for a line starting with marker and returns the SHA after it. +func parseCommitSHA(output, marker string) string { for line := range strings.SplitSeq(output, "\n") { line = strings.TrimSpace(line) - sha, found := strings.CutPrefix(line, commitMarker) + sha, found := strings.CutPrefix(line, marker) if found { return strings.TrimSpace(sha) } diff --git a/internal/docker/validation_test.go b/internal/docker/validation_test.go index 1e735b9..01a0a56 100644 --- a/internal/docker/validation_test.go +++ b/internal/docker/validation_test.go @@ -16,6 +16,7 @@ import ( "testing" "time" + "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/docker/docker/pkg/stdcopy" controlapi "github.com/moby/buildkit/api/services/control" @@ -205,6 +206,8 @@ func TestPerformCloneRemovesContainerVolumes(t *testing.T) { <-r.Context().Done() case strings.HasSuffix(r.URL.Path, "/wait"): _, _ = fmt.Fprintf(w, `{"StatusCode":%d}`, tt.exitCode) + case strings.HasSuffix(r.URL.Path, "/logs"): + writeCloneOutput(w) default: _, _ = w.Write([]byte(`{}`)) } @@ -221,18 +224,7 @@ func TestPerformCloneRemovesContainerVolumes(t *testing.T) { c := &Client{docker: dockerAPI, log: slog.Default()} - dir := t.TempDir() - cfg := &cloneConfig{ - repoURL: "git@example.com:repo.git", - branch: mainBranch, - sshPrivateKey: "fake-key", - containerDir: filepath.Join(dir, "repo"), - hostDir: filepath.Join(dir, "repo"), - keyFile: filepath.Join(dir, "deploy_key"), - hostKeyFile: filepath.Join(dir, "deploy_key"), - } - - _, _ = c.performClone(ctx, cfg) + _, _ = c.performClone(ctx, testCloneConfig(t)) select { case query := <-removeQuery: @@ -246,6 +238,38 @@ func TestPerformCloneRemovesContainerVolumes(t *testing.T) { } } +// testCloneConfig returns the settings of a clone in these tests, with its +// files in a temporary directory. +func testCloneConfig(t *testing.T) *cloneConfig { + t.Helper() + + dir := t.TempDir() + + return &cloneConfig{ + repoURL: "git@example.com:repo.git", + branch: mainBranch, + sshPrivateKey: "fake-key", + containerDir: filepath.Join(dir, "repo"), + hostDir: filepath.Join(dir, "repo"), + keyFile: filepath.Join(dir, "deploy_key"), + hostKeyFile: filepath.Join(dir, "deploy_key"), + } +} + +// cloneCommit is the commit the fake clones in these tests check out. +const cloneCommit = "1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b" + +// writeCloneOutput writes the output of a git clone of cloneCommit as +// Docker sends a container's log: each line after a header. +func writeCloneOutput(w io.Writer) { + stdout := stdcopy.NewStdWriter(w, stdcopy.Stdout) + stderr := stdcopy.NewStdWriter(w, stdcopy.Stderr) + + _, _ = stderr.Write([]byte("Cloning into '/repo'...\n")) + _, _ = stdout.Write([]byte("COMMIT:" + cloneCommit + "\n")) + _, _ = stdout.Write([]byte("SHORT_SHA:1a2b3c4\n")) +} + // TestPerformBuildUsesBuildKit runs a build against a fake Docker API and // checks that it asks for BuildKit and that BuildKit's progress reaches the // build log as plain text. @@ -501,12 +525,11 @@ func serveSession( // TestPerformCloneReadsFramedLogs runs a clone against a fake Docker API that // sends the clone container's output in frames, as Docker does for a // container without a terminal, and checks that the output comes back as -// plain text and that the commit is read from it. +// plain text and that the commit, in full and in git's short form, is read +// from it. func TestPerformCloneReadsFramedLogs(t *testing.T) { t.Parallel() - const commit = "1647b43aa6b211686719313bc6372c3693c54ca9" - srv := httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -517,10 +540,7 @@ func TestPerformCloneReadsFramedLogs(t *testing.T) { case strings.HasSuffix(r.URL.Path, "/wait"): _, _ = w.Write([]byte(`{"StatusCode":0}`)) case strings.HasSuffix(r.URL.Path, "/logs"): - _, _ = stdcopy.NewStdWriter(w, stdcopy.Stderr). - Write([]byte("Cloning into '/repo'...\n")) - _, _ = stdcopy.NewStdWriter(w, stdcopy.Stdout). - Write([]byte("COMMIT:" + commit + "\n")) + writeCloneOutput(w) default: _, _ = w.Write([]byte(`{}`)) } @@ -537,28 +557,126 @@ func TestPerformCloneReadsFramedLogs(t *testing.T) { c := &Client{docker: dockerAPI, log: slog.Default()} - dir := t.TempDir() - cfg := &cloneConfig{ - repoURL: "git@example.com:repo.git", - branch: mainBranch, - sshPrivateKey: "fake-key", - containerDir: filepath.Join(dir, "repo"), - hostDir: filepath.Join(dir, "repo"), - keyFile: filepath.Join(dir, "deploy_key"), - hostKeyFile: filepath.Join(dir, "deploy_key"), - } - - result, err := c.performClone(t.Context(), cfg) + result, err := c.performClone(t.Context(), testCloneConfig(t)) if err != nil { t.Fatal(err) } - want := "Cloning into '/repo'...\nCOMMIT:" + commit + "\n" + want := "Cloning into '/repo'...\nCOMMIT:" + cloneCommit + "\nSHORT_SHA:1a2b3c4\n" if result.Output != want { t.Errorf("got clone output %q, want %q", result.Output, want) } - if result.CommitSHA != commit { - t.Errorf("got commit %q, want %q", result.CommitSHA, commit) + if result.CommitSHA != cloneCommit || result.ShortSHA != "1a2b3c4" { + t.Errorf("got commit %q, short %q", result.CommitSHA, result.ShortSHA) + } +} + +// TestPerformCloneFailsWithoutShortSHA runs a clone against a fake Docker API +// whose clone succeeds but prints no "SHORT_SHA:" line, and checks that the +// clone fails, since the short hash names the image the deploy builds. +func TestPerformCloneFailsWithoutShortSHA(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case strings.HasSuffix(r.URL.Path, "/containers/create"): + _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/wait"): + _, _ = w.Write([]byte(`{"StatusCode":0}`)) + case strings.HasSuffix(r.URL.Path, "/logs"): + _, _ = stdcopy.NewStdWriter(w, stdcopy.Stdout). + Write([]byte("COMMIT:" + cloneCommit + "\n")) + default: + _, _ = w.Write([]byte(`{}`)) + } + }, + )) + t.Cleanup(srv.Close) + + dockerAPI, err := client.NewClientWithOpts( + client.WithHost("tcp://" + srv.Listener.Addr().String()), + ) + if err != nil { + t.Fatal(err) + } + + c := &Client{docker: dockerAPI, log: slog.Default()} + + _, err = c.performClone(t.Context(), testCloneConfig(t)) + if !errors.Is(err, ErrGitCloneFailed) { + t.Errorf("got error %v, want %v", err, ErrGitCloneFailed) + } +} + +// TestPerformCloneAsksGitForShortSHA runs a clone of a branch's last commit +// and a clone of a given commit against a fake Docker API, and checks that +// the command each clone container is created with prints git's own short +// form of the commit checked out. +func TestPerformCloneAsksGitForShortSHA(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + commitSHA string + }{ + {name: "branch", commitSHA: ""}, + {name: "commit", commitSHA: cloneCommit}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + created := make(chan container.Config, 1) + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case strings.HasSuffix(r.URL.Path, "/containers/create"): + var cfg container.Config + + _ = json.NewDecoder(r.Body).Decode(&cfg) + created <- cfg + + _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/wait"): + _, _ = w.Write([]byte(`{"StatusCode":0}`)) + case strings.HasSuffix(r.URL.Path, "/logs"): + writeCloneOutput(w) + default: + _, _ = w.Write([]byte(`{}`)) + } + }, + )) + t.Cleanup(srv.Close) + + dockerAPI, err := client.NewClientWithOpts( + client.WithHost("tcp://" + srv.Listener.Addr().String()), + ) + if err != nil { + t.Fatal(err) + } + + c := &Client{docker: dockerAPI, log: slog.Default()} + + cfg := testCloneConfig(t) + cfg.commitSHA = tt.commitSHA + + _, err = c.performClone(t.Context(), cfg) + if err != nil { + t.Fatal(err) + } + + cmd := strings.Join((<-created).Cmd, " ") + if !strings.Contains(cmd, "echo SHORT_SHA:$(git rev-parse --short HEAD)") { + t.Errorf("clone command %q does not print git's short hash", cmd) + } + }) } } diff --git a/internal/models/deployment.go b/internal/models/deployment.go index 874b8d4..a024c75 100644 --- a/internal/models/deployment.go +++ b/internal/models/deployment.go @@ -203,11 +203,12 @@ func (d *Deployment) insert(ctx context.Context) error { func (d *Deployment) update(ctx context.Context) error { query := ` UPDATE deployments SET - image_id = ?, container_id = ?, status = ?, logs = ?, finished_at = ? + commit_sha = ?, image_id = ?, container_id = ?, status = ?, logs = ?, + finished_at = ? WHERE id = ?` _, err := d.db.Exec(ctx, query, - d.ImageID, d.ContainerID, d.Status, d.Logs, d.FinishedAt, d.ID, + d.CommitSHA, d.ImageID, d.ContainerID, d.Status, d.Logs, d.FinishedAt, d.ID, ) return err @@ -295,6 +296,45 @@ func FindDeploymentsByAppID( return deployments, nil } +// FindDeploymentImageIDs returns the IDs of the images an app's +// deployments built or rolled back to. +func FindDeploymentImageIDs( + ctx context.Context, + deployDB *database.Database, + appID string, +) ([]string, error) { + rows, err := deployDB.Query(ctx, ` + SELECT DISTINCT image_id FROM deployments + WHERE app_id = ? AND image_id IS NOT NULL`, + appID, + ) + if err != nil { + return nil, fmt.Errorf("querying deployment image IDs: %w", err) + } + + defer func() { _ = rows.Close() }() + + var imageIDs []string + + for rows.Next() { + var imageID string + + scanErr := rows.Scan(&imageID) + if scanErr != nil { + return nil, fmt.Errorf("scanning deployment image ID: %w", scanErr) + } + + imageIDs = append(imageIDs, imageID) + } + + rowsErr := rows.Err() + if rowsErr != nil { + return nil, fmt.Errorf("iterating deployment image IDs: %w", rowsErr) + } + + return imageIDs, nil +} + // LatestDeploymentForApp finds the most recent deployment for an app. // //nolint:nilnil // returning nil,nil is idiomatic for "not found" in Active Record diff --git a/internal/models/models_test.go b/internal/models/models_test.go index a4ac37f..b6016f2 100644 --- a/internal/models/models_test.go +++ b/internal/models/models_test.go @@ -564,6 +564,33 @@ func TestDeploymentMarkFinished(t *testing.T) { assert.True(t, found.FinishedAt.Valid) } +// TestDeploymentSaveStoresCommitSetAfterInsert checks that a commit set on a +// deployment after it was first saved, as a manual deploy does once the clone +// reads it, is stored by the next save. +func TestDeploymentSaveStoresCommitSetAfterInsert(t *testing.T) { + t.Parallel() + + testDB, cleanup := setupTestDB(t) + defer cleanup() + + app := createTestApp(t, testDB) + + deployment := models.NewDeployment(testDB) + deployment.AppID = app.ID + + err := deployment.Save(context.Background()) + require.NoError(t, err) + + deployment.CommitSHA = sql.NullString{String: "abc123def456", Valid: true} + + err = deployment.Save(context.Background()) + require.NoError(t, err) + + found, err := models.FindDeployment(context.Background(), testDB, deployment.ID) + require.NoError(t, err) + assert.Equal(t, "abc123def456", found.CommitSHA.String) +} + func TestDeploymentFindByAppID(t *testing.T) { t.Parallel() diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 9af52d0..d6e7635 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -746,44 +746,103 @@ func (svc *Service) recordDeployedImage( 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. +// removeUnusedImages removes the app's images except those an app's running +// container uses or its Rollback would start. Docker deletes a tagged 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, ) { - tags, err := svc.docker.ListImageTags(ctx, "upaas-"+app.Name) + images, err := svc.findAppImages(ctx, app) if err != nil { svc.log.Error("failed to list app images", "error", err, "app", app.Name) return } - for _, tag := range slices.Sorted(maps.Keys(tags)) { - imageID := tags[tag].String() - if imageID == app.ImageID.String || imageID == app.PreviousImageID.String { + keep, err := svc.imagesToKeep(ctx) + if err != nil { + svc.log.Error("failed to list the images apps use", "error", err, "app", app.Name) + + return + } + + for _, name := range slices.Sorted(maps.Keys(images)) { + if keep[images[name].String()] { continue } - removeErr := svc.docker.RemoveImageTag(ctx, tag) + removeErr := svc.docker.RemoveImageTag(ctx, name) if removeErr != nil { svc.log.Error("failed to remove old image", - "error", removeErr, "app", app.Name, "tag", tag) + "error", removeErr, "app", app.Name, "image", name) _ = deployment.AppendLog( ctx, - "WARNING: failed to remove old image "+tag+": "+removeErr.Error(), + "WARNING: failed to remove old image "+name+": "+removeErr.Error(), ) continue } - _ = deployment.AppendLog(ctx, "Removed old image: "+tag) + _ = deployment.AppendLog(ctx, "Removed old image: "+name) } } +// findAppImages returns the app's images, each under the name it is removed +// by: its tag, upaas-: as set by buildImage, or its ID if +// it has none. A redeploy of a commit gives the commit's tag to the new +// image, so the old one is found among the images the app's deployments +// recorded. +func (svc *Service) findAppImages( + ctx context.Context, + app *models.App, +) (map[string]docker.ImageID, error) { + images, err := svc.docker.ListImageTags(ctx, "upaas-"+app.Name) + if err != nil { + return nil, fmt.Errorf("failed to list image tags: %w", err) + } + + untagged, err := svc.docker.ListUntaggedImages(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list untagged images: %w", err) + } + + recorded, err := models.FindDeploymentImageIDs(ctx, svc.db, app.ID) + if err != nil { + return nil, fmt.Errorf("failed to find deployment images: %w", err) + } + + for _, imageID := range untagged { + if slices.Contains(recorded, imageID.String()) { + images[imageID.String()] = imageID + } + } + + return images, nil +} + +// imagesToKeep returns the IDs of the image each app's running container +// uses and the one its Rollback would start. It covers every app because +// apps that build the same commit can share an image, and a redeploy can +// take the tag that kept the image for one of them. +func (svc *Service) imagesToKeep(ctx context.Context) (map[string]bool, error) { + apps, err := models.AllApps(ctx, svc.db) + if err != nil { + return nil, fmt.Errorf("failed to list apps: %w", err) + } + + keep := make(map[string]bool) + + for _, app := range apps { + keep[app.ImageID.String] = true + keep[app.PreviousImageID.String] = true + } + + return keep, nil +} + // cleanupCancelledDeploy removes orphan resources left by a cancelled deployment. func (svc *Service) cleanupCancelledDeploy( ctx context.Context, @@ -923,14 +982,14 @@ func (svc *Service) buildImage( app *models.App, deployment *models.Deployment, ) (docker.ImageID, error) { - workDir, cleanup, err := svc.cloneRepository(ctx, app, deployment) + workDir, shortSHA, cleanup, err := svc.cloneRepository(ctx, app, deployment) if err != nil { return "", err } defer cleanup() - imageTag := fmt.Sprintf("upaas-%s:%d", app.Name, deployment.ID) + imageTag := "upaas-" + app.Name + ":" + shortSHA // Create log writer that flushes build output to deployment logs every second logWriter := newDeploymentLogWriter(ctx, deployment) @@ -965,11 +1024,14 @@ func (svc *Service) buildImage( return imageID, nil } +// cloneRepository clones the app's repository for a build. It returns the +// directory of the clone, git's short form of the commit checked out, and a +// function that removes the clone. func (svc *Service) cloneRepository( ctx context.Context, app *models.App, deployment *models.Deployment, -) (string, func(), error) { +) (string, string, func(), error) { // Use a subdirectory of DataDir for builds since it's mounted from the host // and accessible to Docker for bind mounts (unlike /tmp inside the container). // Structure: builds//-/ @@ -986,7 +1048,7 @@ func (svc *Service) cloneRepository( fmt.Errorf("failed to create builds dir: %w", err), ) - return "", nil, fmt.Errorf("failed to create builds dir: %w", err) + return "", "", nil, fmt.Errorf("failed to create builds dir: %w", err) } buildDir, err := os.MkdirTemp(appBuildsDir, fmt.Sprintf("%d-*", deployment.ID)) @@ -998,7 +1060,7 @@ func (svc *Service) cloneRepository( fmt.Errorf("failed to create temp dir: %w", err), ) - return "", nil, fmt.Errorf("failed to create temp dir: %w", err) + return "", "", nil, fmt.Errorf("failed to create temp dir: %w", err) } cleanup := func() { _ = os.RemoveAll(buildDir) } @@ -1034,7 +1096,7 @@ func (svc *Service) cloneRepository( fmt.Errorf("failed to clone repo: %w", cloneErr), ) - return "", nil, fmt.Errorf("failed to clone repo: %w", cloneErr) + return "", "", nil, fmt.Errorf("failed to clone repo: %w", cloneErr) } svc.processCloneResult(ctx, app, deployment, cloneResult, commitSHA) @@ -1042,7 +1104,7 @@ func (svc *Service) cloneRepository( // Return the 'work' subdirectory where the repo was cloned workDir := filepath.Join(buildDir, "work") - return workDir, cleanup, nil + return workDir, cloneResult.ShortSHA, cleanup, nil } // processCloneResult handles the result of a git clone operation. diff --git a/internal/service/deploy/deploy_build_test.go b/internal/service/deploy/deploy_build_test.go index a2cf9f4..655db3d 100644 --- a/internal/service/deploy/deploy_build_test.go +++ b/internal/service/deploy/deploy_build_test.go @@ -34,6 +34,8 @@ func TestBuildImageLogsBuildErrorBeforeDeployError(t *testing.T) { switch { case strings.HasSuffix(r.URL.Path, "/containers/create"): _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/logs"): + writeCloneOutput(w, "abc1234") case strings.HasSuffix(r.URL.Path, "/version"): _, _ = w.Write([]byte(`{"Version":"27.3.1","ApiVersion":"1.47"}`)) case strings.HasSuffix(r.URL.Path, "/build"): @@ -77,7 +79,7 @@ func TestBuildImageLogsBuildErrorBeforeDeployError(t *testing.T) { // The service has no notify service: the app has no ntfy topic and no // Slack webhook, so the build failure notification sends nothing. - svc := deploy.NewTestServiceWithConfig(log, cfg, dockerClient) + svc := deploy.NewTestServiceWithConfig(log, cfg, db, dockerClient) _, err = svc.BuildImage(ctx, app, deployment) require.EqualError(t, err, "failed to build image: exit code: 1") diff --git a/internal/service/deploy/deploy_cleanup_test.go b/internal/service/deploy/deploy_cleanup_test.go index 42f7954..4742731 100644 --- a/internal/service/deploy/deploy_cleanup_test.go +++ b/internal/service/deploy/deploy_cleanup_test.go @@ -20,7 +20,7 @@ func TestCleanupCancelledDeploy_RemovesBuildDir(t *testing.T) { tmpDir := t.TempDir() cfg := &config.Config{DataDir: tmpDir} - svc := deploy.NewTestServiceWithConfig(slog.Default(), cfg, nil) + svc := deploy.NewTestServiceWithConfig(slog.Default(), cfg, nil, nil) // Create a fake build directory matching the deployment pattern appName := "test-app" @@ -59,7 +59,7 @@ func TestCleanupCancelledDeploy_NoBuildDir(t *testing.T) { tmpDir := t.TempDir() cfg := &config.Config{DataDir: tmpDir} - svc := deploy.NewTestServiceWithConfig(slog.Default(), cfg, nil) + svc := deploy.NewTestServiceWithConfig(slog.Default(), cfg, nil, nil) // Should not panic when build dir doesn't exist svc.CleanupCancelledDeploy(context.Background(), "nonexistent-app", 1, "") diff --git a/internal/service/deploy/deploy_images_test.go b/internal/service/deploy/deploy_images_test.go index 086ccb3..7e93849 100644 --- a/internal/service/deploy/deploy_images_test.go +++ b/internal/service/deploy/deploy_images_test.go @@ -3,14 +3,21 @@ package deploy_test import ( "context" "database/sql" + "encoding/json" + "fmt" + "io" "log/slog" + "maps" "net/http" "net/http/httptest" "os" + "slices" "strings" "sync" "testing" + "github.com/docker/docker/api/types/image" + "github.com/docker/docker/pkg/stdcopy" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/fx/fxtest" @@ -23,45 +30,139 @@ import ( "sneak.berlin/go/upaas/internal/service/deploy" ) -// 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() +// fakeImageAPI is a fake Docker API that keeps images and their tags as +// Docker does: a build gives its tag to the image it builds, and removing +// an image's last tag, or an untagged image by its ID, deletes the image. +// It also answers the steps of a git clone that reports shortSHA. +type fakeImageAPI struct { + mu sync.Mutex + images map[string][]string // image ID -> tags + shortSHA string // the commit's short hash the clone reports + nextID string // ID of the image the next build creates + removed []string // each tag or ID removed + forced bool // whether a removal was forced +} - var ( - mu sync.Mutex - removed []string - forced bool - ) +func (api *fakeImageAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) { + api.mu.Lock() + defer api.mu.Unlock() - srv := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Type", "application/json") - switch { - case r.Method == http.MethodDelete: - _, name, _ := strings.Cut(r.URL.Path, "/images/") + _, name, isImage := strings.Cut(r.URL.Path, "/images/") - mu.Lock() + switch { + case strings.HasSuffix(r.URL.Path, "/images/json"): + dangling := strings.Contains(r.URL.Query().Get("filters"), "dangling") + api.listImages(w, dangling) + case isImage && r.Method == http.MethodDelete: + api.forced = api.forced || r.URL.Query().Get("force") != "" + api.removeImage(w, name) + case isImage && strings.HasSuffix(name, "/json"): + api.inspectImage(w, strings.TrimSuffix(name, "/json")) + case strings.HasSuffix(r.URL.Path, "/build"): + tag := r.URL.Query().Get("t") + api.untag(tag) + api.images[api.nextID] = append(api.images[api.nextID], tag) + case strings.HasSuffix(r.URL.Path, "/version"): + _, _ = w.Write([]byte(`{"Version":"27.3.1","ApiVersion":"1.47"}`)) + case strings.HasSuffix(r.URL.Path, "/containers/create"): + _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) + case strings.HasSuffix(r.URL.Path, "/logs"): + writeCloneOutput(w, api.shortSHA) + default: + // The other steps of the git clone, which succeeds. + _, _ = w.Write([]byte(`{}`)) + } +} - removed = append(removed, name) - forced = forced || r.URL.Query().Get("force") != "" - mu.Unlock() +// listImages lists the untagged images, or else the tagged ones. +func (api *fakeImageAPI) listImages(w http.ResponseWriter, dangling bool) { + list := []image.Summary{} - _, _ = 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(`{}`)) - } - }, - )) + for _, id := range slices.Sorted(maps.Keys(api.images)) { + if (len(api.images[id]) == 0) == dangling { + list = append(list, image.Summary{ID: id, RepoTags: api.images[id]}) + } + } + + data, err := json.Marshal(list) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + + return + } + + _, _ = w.Write(data) +} + +func (api *fakeImageAPI) inspectImage(w http.ResponseWriter, name string) { + for id, tags := range api.images { + if id == name || slices.Contains(tags, name) { + _, _ = fmt.Fprintf(w, `{"Id":%q}`, id) + + return + } + } + + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"No such image"}`)) +} + +func (api *fakeImageAPI) removeImage(w http.ResponseWriter, name string) { + api.removed = append(api.removed, name) + + id := name + if _, isID := api.images[name]; !isID { + id = api.untag(name) + } + + if len(api.images[id]) == 0 { + delete(api.images, id) + } + + _, _ = w.Write([]byte(`[]`)) +} + +// untag removes tag from the image that has it, leaving the image, and +// returns the image's ID. +func (api *fakeImageAPI) untag(tag string) string { + for id, tags := range api.images { + if slices.Contains(tags, tag) { + api.images[id] = slices.DeleteFunc(tags, func(t string) bool { return t == tag }) + + return id + } + } + + return "" +} + +// state returns each image's tags and each tag or ID removed. +func (api *fakeImageAPI) state() (map[string][]string, []string) { + api.mu.Lock() + defer api.mu.Unlock() + + return maps.Clone(api.images), slices.Clone(api.removed) +} + +// writeCloneOutput writes the line of a git clone's output that gives the +// commit's short hash, as Docker sends a container's log: after a header. +func writeCloneOutput(w io.Writer, shortSHA string) { + out := stdcopy.NewStdWriter(w, stdcopy.Stdout) + + _, _ = fmt.Fprintf(out, "SHORT_SHA:%s\n", shortSHA) +} + +// newImageTestService returns a deploy service that uses api as its +// Docker API, and its database. +func newImageTestService( + t *testing.T, + api *fakeImageAPI, +) (*deploy.Service, *database.Database) { + t.Helper() + + srv := httptest.NewServer(api) t.Cleanup(srv.Close) log := slog.New(slog.NewTextHandler(os.Stderr, nil)) @@ -77,30 +178,203 @@ func TestRecordDeployedImageRemovesOldImages(t *testing.T) { t.Cleanup(lifecycle.RequireStop) db := database.NewTestDatabase(t) - ctx := context.Background() + dataDir := t.TempDir() + cfg := &config.Config{DataDir: dataDir, HostDataDir: dataDir} + + return deploy.NewTestServiceWithConfig(log, cfg, db, dockerClient), db +} + +// saveApp saves an app with the given current and previous image. +func saveApp( + t *testing.T, + db *database.Database, + name, imageID, previousImageID string, +) *models.App { + t.Helper() 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)) + app.ID = name + "-id" + app.Name = name + app.ImageID = sql.NullString{String: imageID, Valid: true} + app.PreviousImageID = sql.NullString{String: previousImageID, Valid: true} + require.NoError(t, app.Save(context.Background())) + + return app +} + +// TestRecordDeployedImageRemovesOldImages runs the step after a deploy +// against a fake Docker API. Image one has a tag from before images were +// tagged with their commit, and is also tagged for another app. Image two +// was the previous image, three the current one, four is new. Image five is +// the other app's previous image, which a redeploy of its commit left +// without the other app's tag. +func TestRecordDeployedImageRemovesOldImages(t *testing.T) { + t.Parallel() + + api := &fakeImageAPI{images: map[string][]string{ + "sha256:one": {"upaas-myapp:140", "upaas-otherapp:1a2b3c4"}, + "sha256:two": {"upaas-myapp:2b3c4d5"}, + "sha256:three": {"upaas-myapp:3c4d5e6"}, + "sha256:four": {"upaas-myapp:4d5e6f7"}, + "sha256:five": {"upaas-myapp:5e6f7a8"}, + }} + svc, db := newImageTestService(t, api) + ctx := context.Background() + + app := saveApp(t, db, "myapp", "sha256:three", "sha256:two") + saveApp(t, db, "otherapp", "sha256:other", "sha256:five") 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") + 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() + images, removed := api.state() - assert.Equal(t, []string{"upaas-myapp:1", "upaas-myapp:2"}, removed) - assert.False(t, forced, "old image tags must be removed without force") + assert.Equal(t, []string{"upaas-myapp:140", "upaas-myapp:2b3c4d5"}, removed) + assert.Equal(t, map[string][]string{ + "sha256:one": {"upaas-otherapp:1a2b3c4"}, + "sha256:three": {"upaas-myapp:3c4d5e6"}, + "sha256:four": {"upaas-myapp:4d5e6f7"}, + "sha256:five": {"upaas-myapp:5e6f7a8"}, + }, images) + assert.False(t, api.forced, "old images must be removed without force") +} + +// TestRecordDeployedImageRemovesOnlyTheAppsUntaggedImages runs the step after +// a deploy against a fake Docker API that holds three untagged images: one an +// earlier deployment of the app recorded, one a deployment of another app +// recorded, and one no deployment recorded, such as an image upaas never +// built. Only the app's own is removed. +func TestRecordDeployedImageRemovesOnlyTheAppsUntaggedImages(t *testing.T) { + t.Parallel() + + api := &fakeImageAPI{images: map[string][]string{ + "sha256:new": {"upaas-myapp:1a2b3c4"}, + "sha256:myapp": {}, + "sha256:otherapp": {}, + "sha256:unknown": {}, + }} + svc, db := newImageTestService(t, api) + ctx := context.Background() + + app := saveApp(t, db, "myapp", "", "") + otherApp := saveApp(t, db, "otherapp", "", "") + + saveDeployment := func(appID, imageID string) *models.Deployment { + t.Helper() + + deployment := models.NewDeployment(db) + deployment.AppID = appID + deployment.ImageID = sql.NullString{String: imageID, Valid: true} + require.NoError(t, deployment.Save(ctx)) + + return deployment + } + + saveDeployment(app.ID, "sha256:myapp") + saveDeployment(otherApp.ID, "sha256:otherapp") + deployment := saveDeployment(app.ID, "sha256:new") + + err := svc.RecordDeployedImage(ctx, app, deployment, "sha256:new") + require.NoError(t, err) + + images, removed := api.state() + + assert.Equal(t, []string{"sha256:myapp"}, removed) + assert.Equal(t, map[string][]string{ + "sha256:new": {"upaas-myapp:1a2b3c4"}, + "sha256:otherapp": {}, + "sha256:unknown": {}, + }, images) +} + +// TestRedeployRemovesImagesLeftWithoutTag deploys commits against a fake +// Docker API, some of them again. A build takes the commit's tag from the +// image an earlier build of it made. That image is kept, without a tag, +// while the app runs it or Rollback would start it, and is removed by its +// ID once neither does. +func TestRedeployRemovesImagesLeftWithoutTag(t *testing.T) { + t.Parallel() + + const ( + tagABC1234 = "upaas-myapp:abc1234" + tag0123ABC = "upaas-myapp:0123abc" + retriedImage = "sha256:retried-0123abc" + ) + + // The app runs commit abc1234 and would roll back to def5678. The last + // deploy, of commit 0123abc, failed after its build. + api := &fakeImageAPI{images: map[string][]string{ + "sha256:built-abc1234": {tagABC1234}, + "sha256:built-def5678": {"upaas-myapp:def5678"}, + "sha256:failed-0123abc": {tag0123ABC}, + }} + svc, db := newImageTestService(t, api) + ctx := context.Background() + + app := saveApp(t, db, "myapp", "sha256:built-abc1234", "sha256:built-def5678") + + for imageID := range api.images { + deployment := models.NewDeployment(db) + deployment.AppID = app.ID + deployment.ImageID = sql.NullString{String: imageID, Valid: true} + require.NoError(t, deployment.Save(ctx)) + } + + deployCommit := func(shortSHA, imageID string) { + t.Helper() + + api.mu.Lock() + api.shortSHA = shortSHA + api.nextID = imageID + api.mu.Unlock() + + deployment := models.NewDeployment(db) + deployment.AppID = app.ID + require.NoError(t, deployment.Save(ctx)) + + built, err := svc.BuildImage(ctx, app, deployment) + require.NoError(t, err) + require.NoError(t, svc.RecordDeployedImage(ctx, app, deployment, built)) + } + + // The failed deploy's image loses its tag, and nothing uses it. + deployCommit("0123abc", retriedImage) + + images, _ := api.state() + assert.Equal(t, map[string][]string{ + "sha256:built-abc1234": {tagABC1234}, + retriedImage: {tag0123ABC}, + }, images) + + // The running image loses its tag and becomes the one Rollback starts. + deployCommit("0123abc", "sha256:rebuilt-0123abc") + + images, _ = api.state() + assert.Equal(t, map[string][]string{ + "sha256:rebuilt-0123abc": {tag0123ABC}, + retriedImage: {}, + }, images) + assert.Equal(t, retriedImage, app.PreviousImageID.String) + + // Once Rollback no longer needs it, the untagged image is removed. + deployCommit("4567def", "sha256:built-4567def") + + images, removed := api.state() + assert.Equal(t, map[string][]string{ + "sha256:built-4567def": {"upaas-myapp:4567def"}, + "sha256:rebuilt-0123abc": {tag0123ABC}, + }, images) + assert.Equal(t, []string{ + "sha256:failed-0123abc", "upaas-myapp:def5678", // first deploy + tagABC1234, // second deploy + retriedImage, // third deploy + }, removed) + assert.False(t, api.forced, "old images must be removed without force") } diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index 95ce82e..8abfc34 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -9,6 +9,7 @@ import ( "strings" "sneak.berlin/go/upaas/internal/config" + "sneak.berlin/go/upaas/internal/database" "sneak.berlin/go/upaas/internal/docker" "sneak.berlin/go/upaas/internal/models" ) @@ -44,15 +45,18 @@ func (svc *Service) UnlockApp(appID string) { svc.unlockApp(appID) } -// NewTestServiceWithConfig creates a Service with config and docker client for testing. +// NewTestServiceWithConfig creates a Service with config, database and +// docker client for testing. func NewTestServiceWithConfig( log *slog.Logger, cfg *config.Config, + db *database.Database, dockerClient *docker.Client, ) *Service { return &Service{ log: log, config: cfg, + db: db, docker: dockerClient, } }