diff --git a/pkg/common/progress.go b/pkg/common/progress.go index ff7df18b1..d3729491c 100644 --- a/pkg/common/progress.go +++ b/pkg/common/progress.go @@ -34,6 +34,10 @@ func DisplayProgress(ctx context.Context, eg *errgroup.Group, buildChannel chan // ForwardProgressWithPrefix forwards progress from src to dst, prefixing vertex names with the given prefix. // This allows multiplexing multiple build progress streams into a single display. func ForwardProgressWithPrefix(ctx context.Context, src <-chan *client.SolveStatus, dst chan<- *client.SolveStatus, prefix string) { + namePrefix := "[" + prefix + "] " + digestPrefix := prefix + ":" + remappedDigests := make(map[digest.Digest]digest.Digest) + for { select { case status, ok := <-src: @@ -44,35 +48,33 @@ func ForwardProgressWithPrefix(ctx context.Context, src <-chan *client.SolveStat continue } - // Clone and prefix vertex names + // Clone and prefix vertex names. prefixed := &client.SolveStatus{ Vertexes: make([]*client.Vertex, len(status.Vertexes)), - Statuses: status.Statuses, // Statuses reference vertices by digest, no need to modify - Logs: status.Logs, // Logs reference vertices by digest, no need to modify Warnings: status.Warnings, } for i, v := range status.Vertexes { - // Clone the vertex and prefix its name + // Clone the vertex and prefix its name. cloned := *v - cloned.Name = "[" + prefix + "] " + v.Name - // Create a new digest that includes the prefix to avoid collisions - cloned.Digest = digest.FromString(prefix + ":" + v.Digest.String()) + cloned.Name = namePrefix + v.Name + // Create a new digest that includes the prefix to avoid collisions. + cloned.Digest = prefixedDigest(remappedDigests, digestPrefix, v.Digest) prefixed.Vertexes[i] = &cloned } - // Also update status and log references to use new digests + // Also update status and log references to use new digests. prefixed.Statuses = make([]*client.VertexStatus, len(status.Statuses)) for i, s := range status.Statuses { cloned := *s - cloned.Vertex = digest.FromString(prefix + ":" + s.Vertex.String()) + cloned.Vertex = prefixedDigest(remappedDigests, digestPrefix, s.Vertex) prefixed.Statuses[i] = &cloned } prefixed.Logs = make([]*client.VertexLog, len(status.Logs)) for i, l := range status.Logs { cloned := *l - cloned.Vertex = digest.FromString(prefix + ":" + l.Vertex.String()) + cloned.Vertex = prefixedDigest(remappedDigests, digestPrefix, l.Vertex) prefixed.Logs[i] = &cloned } @@ -87,3 +89,13 @@ func ForwardProgressWithPrefix(ctx context.Context, src <-chan *client.SolveStat } } } + +func prefixedDigest(remappedDigests map[digest.Digest]digest.Digest, digestPrefix string, original digest.Digest) digest.Digest { + if remapped, ok := remappedDigests[original]; ok { + return remapped + } + + remapped := digest.FromString(digestPrefix + original.String()) + remappedDigests[original] = remapped + return remapped +} diff --git a/pkg/common/progress_benchmark_test.go b/pkg/common/progress_benchmark_test.go new file mode 100644 index 000000000..17e8a920e --- /dev/null +++ b/pkg/common/progress_benchmark_test.go @@ -0,0 +1,88 @@ +package common + +import ( + "context" + "testing" + "time" + + "github.com/moby/buildkit/client" + "github.com/opencontainers/go-digest" +) + +func BenchmarkForwardProgressWithPrefixRepeatedDigests(b *testing.B) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + src := make(chan *client.SolveStatus) + dst := make(chan *client.SolveStatus) + done := make(chan struct{}) + go func() { + defer close(done) + ForwardProgressWithPrefix(ctx, src, dst, "linux/amd64") + }() + + status := repeatedDigestSolveStatus() + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + src <- status + prefixed := <-dst + if len(prefixed.Statuses) != len(status.Statuses) || len(prefixed.Logs) != len(status.Logs) { + b.Fatalf("unexpected forwarded status sizes: statuses=%d logs=%d", len(prefixed.Statuses), len(prefixed.Logs)) + } + } + b.StopTimer() + + close(src) + <-done +} + +func repeatedDigestSolveStatus() *client.SolveStatus { + now := time.Unix(1_700_000_000, 0) + digests := []digest.Digest{ + digest.FromString("vertex-0"), + digest.FromString("vertex-1"), + digest.FromString("vertex-2"), + digest.FromString("vertex-3"), + } + + vertexes := make([]*client.Vertex, len(digests)) + for i, d := range digests { + vertexes[i] = &client.Vertex{ + Digest: d, + Name: forwardProgressTestVertexName, + Started: &now, + } + } + + statuses := make([]*client.VertexStatus, 64) + logs := make([]*client.VertexLog, 64) + for i := range statuses { + d := digests[i%len(digests)] + statuses[i] = &client.VertexStatus{ + ID: "status", + Vertex: d, + Name: "status", + Total: 100, + Current: int64(i), + Timestamp: now, + Started: &now, + } + logs[i] = &client.VertexLog{ + Vertex: d, + Stream: 1, + Data: []byte("repeated log data"), + Timestamp: now, + } + } + + return &client.SolveStatus{ + Vertexes: vertexes, + Statuses: statuses, + Logs: logs, + Warnings: []*client.VertexWarning{ + {Vertex: digests[0], Short: []byte("warning")}, + }, + } +} diff --git a/pkg/common/progress_test.go b/pkg/common/progress_test.go index 470bd56ef..c13879bfb 100644 --- a/pkg/common/progress_test.go +++ b/pkg/common/progress_test.go @@ -13,6 +13,8 @@ import ( "golang.org/x/sync/errgroup" ) +const forwardProgressTestVertexName = "vertex" + func TestDisplayProgress(t *testing.T) { ctx := context.Background() eg, egCtx := errgroup.WithContext(ctx) @@ -172,3 +174,119 @@ func TestDisplayProgressWithDebugMode(t *testing.T) { err := eg.Wait() assert.NoError(t, err) } + +func TestForwardProgressWithPrefixRemapsRepeatedDigestsWithoutMutatingInput(t *testing.T) { + ctx := context.Background() + src := make(chan *client.SolveStatus) + dst := make(chan *client.SolveStatus) + done := make(chan struct{}) + go func() { + defer close(done) + ForwardProgressWithPrefix(ctx, src, dst, "linux/arm64") + }() + + now := time.Unix(1_700_000_001, 0) + vertexA := digest.FromString("vertex-a") + vertexB := digest.FromString("vertex-b") + status := &client.SolveStatus{ + Vertexes: []*client.Vertex{ + {Digest: vertexA, Name: "install packages", Started: &now}, + {Digest: vertexB, Name: "copy files", Completed: &now}, + }, + Statuses: []*client.VertexStatus{ + {ID: "status-a-1", Vertex: vertexA, Name: "download", Timestamp: now}, + {ID: "status-a-2", Vertex: vertexA, Name: "extract", Timestamp: now}, + {ID: "status-b", Vertex: vertexB, Name: "copy", Timestamp: now}, + }, + Logs: []*client.VertexLog{ + {Vertex: vertexA, Stream: 1, Data: []byte("log a"), Timestamp: now}, + {Vertex: vertexA, Stream: 2, Data: []byte("log a again"), Timestamp: now}, + {Vertex: vertexB, Stream: 1, Data: []byte("log b"), Timestamp: now}, + }, + Warnings: []*client.VertexWarning{ + {Vertex: vertexA, Short: []byte("warning")}, + }, + } + + src <- status + prefixed := <-dst + close(src) + <-done + + remappedA := digest.FromString("linux/arm64:" + vertexA.String()) + remappedB := digest.FromString("linux/arm64:" + vertexB.String()) + + assert.Equal(t, remappedA, prefixed.Vertexes[0].Digest) + assert.Equal(t, "[linux/arm64] install packages", prefixed.Vertexes[0].Name) + assert.Equal(t, remappedB, prefixed.Vertexes[1].Digest) + assert.Equal(t, "[linux/arm64] copy files", prefixed.Vertexes[1].Name) + + assert.Equal(t, remappedA, prefixed.Statuses[0].Vertex) + assert.Equal(t, remappedA, prefixed.Statuses[1].Vertex) + assert.Equal(t, remappedB, prefixed.Statuses[2].Vertex) + assert.Equal(t, remappedA, prefixed.Logs[0].Vertex) + assert.Equal(t, remappedA, prefixed.Logs[1].Vertex) + assert.Equal(t, remappedB, prefixed.Logs[2].Vertex) + + // Warnings are forwarded unchanged, matching the previous behavior. + assert.Equal(t, vertexA, prefixed.Warnings[0].Vertex) + + // The input status and nested vertex/status/log fields are not mutated. + assert.Equal(t, vertexA, status.Vertexes[0].Digest) + assert.Equal(t, "install packages", status.Vertexes[0].Name) + assert.Equal(t, vertexB, status.Vertexes[1].Digest) + assert.Equal(t, "copy files", status.Vertexes[1].Name) + assert.Equal(t, vertexA, status.Statuses[0].Vertex) + assert.Equal(t, vertexA, status.Statuses[1].Vertex) + assert.Equal(t, vertexB, status.Statuses[2].Vertex) + assert.Equal(t, vertexA, status.Logs[0].Vertex) + assert.Equal(t, vertexA, status.Logs[1].Vertex) + assert.Equal(t, vertexB, status.Logs[2].Vertex) +} + +func TestForwardProgressWithPrefixSkipsNilStatus(t *testing.T) { + ctx := context.Background() + src := make(chan *client.SolveStatus) + dst := make(chan *client.SolveStatus) + done := make(chan struct{}) + go func() { + defer close(done) + ForwardProgressWithPrefix(ctx, src, dst, "target") + }() + + vertex := digest.FromString("vertex") + status := &client.SolveStatus{ + Vertexes: []*client.Vertex{{Digest: vertex, Name: forwardProgressTestVertexName}}, + } + + src <- nil + src <- status + prefixed := <-dst + close(src) + <-done + + assert.Len(t, prefixed.Vertexes, 1) + assert.Equal(t, digest.FromString("target:"+vertex.String()), prefixed.Vertexes[0].Digest) +} + +func TestForwardProgressWithPrefixReturnsOnCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + src := make(chan *client.SolveStatus) + dst := make(chan *client.SolveStatus) + done := make(chan struct{}) + go func() { + defer close(done) + ForwardProgressWithPrefix(ctx, src, dst, "target") + }() + + src <- &client.SolveStatus{ + Vertexes: []*client.Vertex{{Digest: digest.FromString(forwardProgressTestVertexName), Name: forwardProgressTestVertexName}}, + } + cancel() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("ForwardProgressWithPrefix did not return after context cancellation") + } +} diff --git a/pkg/generate/generate.go b/pkg/generate/generate.go index 127785608..e4fa39b56 100644 --- a/pkg/generate/generate.go +++ b/pkg/generate/generate.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + "sort" "strings" "time" @@ -420,6 +421,7 @@ LABEL sh.copa.image.patched="%s" // Extract and rewrite the patch layer tar with proper paths tr := tar.NewReader(bytes.NewReader(patchLayer)) hardlinks := make(map[string]*tar.Header) + hardlinkTargets := make(map[string]struct{}) for { hdr, err := tr.Next() @@ -434,6 +436,7 @@ LABEL sh.copa.image.patched="%s" if hdr.Typeflag == tar.TypeLink { // Store hardlink info for later hardlinks[hdr.Name] = hdr + hardlinkTargets[hdr.Linkname] = struct{}{} continue } @@ -461,7 +464,7 @@ LABEL sh.copa.image.patched="%s" if len(hardlinks) > 0 { // Re-read the tar to find hardlink targets tr = tar.NewReader(bytes.NewReader(patchLayer)) - fileContents := make(map[string][]byte) + fileContents := make(map[string][]byte, len(hardlinkTargets)) for { hdr, err := tr.Next() @@ -472,8 +475,10 @@ LABEL sh.copa.image.patched="%s" return errors.Wrap(err, "failed to read patch layer tar for hardlinks") } - // Store file contents for hardlink targets - if hdr.Typeflag == tar.TypeReg && hdr.Size > 0 { + // Store only file contents that are actual hardlink targets. The first + // pass has already copied every regular file into the output context; + // this second pass only needs bytes for materializing hardlinks as files. + if _, needed := hardlinkTargets[hdr.Name]; needed && hdr.Typeflag == tar.TypeReg && hdr.Size > 0 { // Limit file size to 1GB to prevent decompression bombs if hdr.Size > maxFileSize { return errors.Errorf("file %s exceeds maximum allowed size of 1GB", hdr.Name) @@ -486,8 +491,15 @@ LABEL sh.copa.image.patched="%s" } } - // Write hardlinks as regular files - for name, hdr := range hardlinks { + // Write hardlinks as regular files. Sort names so identical inputs produce + // deterministic build contexts instead of inheriting map iteration order. + hardlinkNames := make([]string, 0, len(hardlinks)) + for name := range hardlinks { + hardlinkNames = append(hardlinkNames, name) + } + sort.Strings(hardlinkNames) + for _, name := range hardlinkNames { + hdr := hardlinks[name] if content, ok := fileContents[hdr.Linkname]; ok { // Convert to regular file newHdr := &tar.Header{ diff --git a/pkg/generate/generate_benchmark_test.go b/pkg/generate/generate_benchmark_test.go new file mode 100644 index 000000000..64e35f0fe --- /dev/null +++ b/pkg/generate/generate_benchmark_test.go @@ -0,0 +1,77 @@ +package generate + +import ( + "archive/tar" + "bytes" + "fmt" + "io" + "os" + "path/filepath" + "testing" + "time" + + log "github.com/sirupsen/logrus" +) + +func BenchmarkCreateTarStreamWithSparseHardlinks(b *testing.B) { + patchLayer := benchmarkPatchLayer(b, 1000, 4096, 10) + outDir := b.TempDir() + originalOutput := log.StandardLogger().Out + log.SetOutput(io.Discard) + defer log.SetOutput(originalOutput) + + b.ReportAllocs() + b.SetBytes(int64(len(patchLayer))) + for i := 0; i < b.N; i++ { + outputPath := filepath.Join(outDir, fmt.Sprintf("context-%06d.tar", i)) + if err := createTarStream("debian:12", patchLayer, outputPath); err != nil { + b.Fatal(err) + } + if err := os.Remove(outputPath); err != nil { + b.Fatal(err) + } + } +} + +func benchmarkPatchLayer(b *testing.B, regularFiles, fileSize, hardlinks int) []byte { + b.Helper() + + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + modTime := time.Unix(1700000000, 0).UTC() + content := bytes.Repeat([]byte("x"), fileSize) + + for i := 0; i < regularFiles; i++ { + name := fmt.Sprintf("usr/lib/bench/file-%06d.dat", i) + hdr := &tar.Header{ + Name: name, + Mode: 0o644, + Size: int64(len(content)), + ModTime: modTime, + } + if err := tw.WriteHeader(hdr); err != nil { + b.Fatal(err) + } + if _, err := tw.Write(content); err != nil { + b.Fatal(err) + } + } + + for i := 0; i < hardlinks; i++ { + hdr := &tar.Header{ + Name: fmt.Sprintf("usr/bin/bench-link-%06d", i), + Mode: 0o755, + Linkname: fmt.Sprintf("usr/lib/bench/file-%06d.dat", i*(regularFiles/hardlinks)), + ModTime: modTime, + Typeflag: tar.TypeLink, + } + if err := tw.WriteHeader(hdr); err != nil { + b.Fatal(err) + } + } + + if err := tw.Close(); err != nil { + b.Fatal(err) + } + return buf.Bytes() +} diff --git a/pkg/tui/display.go b/pkg/tui/display.go index 45947ac91..60c721524 100644 --- a/pkg/tui/display.go +++ b/pkg/tui/display.go @@ -135,29 +135,20 @@ func (d *progrockDisplay) UpdateFrom(ctx context.Context, ch chan *client.SolveS // processBuildkitProgress processes buildkit progress events and translates them to progrock. func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan *client.SolveStatus, warnings *[]client.VertexWarning) error { - vertices := make(map[string]*progrock.VertexRecorder) - progressTasks := make(map[string]progrockTask) - - typeStatusKey := func(vtxID string, statusName string) string { - // BuildKit status updates don't always include a stable ID; vertex+name is good enough - // to dedupe task creation and reduce flicker. - return vtxID + ":" + statusName - } - cleanupVertexTasks := func(vtxID string) { - for k := range progressTasks { - if strings.HasPrefix(k, vtxID+":") { - delete(progressTasks, k) - } - } + type vertexState struct { + rec *progrock.VertexRecorder + tasks map[string]progrockTask } + vertices := make(map[string]*vertexState) + for { select { case status, ok := <-ch: if !ok { // Channel closed, mark remaining vertices as done and exit for _, vtx := range vertices { - vtx.Done(nil) + vtx.rec.Done(nil) } return nil } @@ -165,6 +156,7 @@ func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan * if status == nil { continue } + completedVertices := make([]string, 0, len(status.Vertexes)) // Process vertex information for _, vertex := range status.Vertexes { @@ -177,24 +169,30 @@ func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan * if name == "" { name = fmt.Sprintf("Step %s", vtxID[:8]) } - vtx = d.rec.Vertex(vertex.Digest, name) + vtx = &vertexState{ + rec: d.rec.Vertex(vertex.Digest, name), + tasks: make(map[string]progrockTask), + } vertices[vtxID] = vtx } // Update vertex status based on buildkit vertex state if vertex.Cached { - vtx.Cached() + vtx.rec.Cached() } if vertex.Completed != nil { // Vertex completed if vertex.Error != "" { - vtx.Done(errors.New(vertex.Error)) + vtx.rec.Done(errors.New(vertex.Error)) } else { - vtx.Done(nil) + vtx.rec.Done(nil) } - delete(vertices, vtxID) - cleanupVertexTasks(vtxID) + // Drop this vertex's progress tasks with the vertex state instead of + // scanning unrelated task bookkeeping. Defer the deletion until this + // entire SolveStatus batch is processed so final status/log records for + // this vertex in the same BuildKit message are not dropped. + completedVertices = append(completedVertices, vtxID) } } @@ -203,21 +201,20 @@ func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan * vtxID := statusUpdate.Vertex.String() if vtx, exists := vertices[vtxID]; exists { // Create/update progress task if we have total progress. - // Dedupe by vertex+name so we don't spam new tasks on every update. + // Dedupe by status name within each vertex so we don't spam new tasks on every update. if statusUpdate.Total > 0 { - key := typeStatusKey(vtxID, statusUpdate.Name) - task, ok := progressTasks[key] + task, ok := vtx.tasks[statusUpdate.Name] if !ok || task.total != statusUpdate.Total { task = progrockTask{ - rec: vtx.ProgressTask(statusUpdate.Total, "%s", statusUpdate.Name), + rec: vtx.rec.ProgressTask(statusUpdate.Total, "%s", statusUpdate.Name), total: statusUpdate.Total, } - progressTasks[key] = task + vtx.tasks[statusUpdate.Name] = task } task.rec.Current(statusUpdate.Current) if statusUpdate.Completed != nil { task.rec.Done(nil) - delete(progressTasks, key) + delete(vtx.tasks, statusUpdate.Name) } } } @@ -235,9 +232,9 @@ func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan * // Send to appropriate stream if log.Stream == 2 { // stderr - fmt.Fprint(vtx.Stderr(), msg) + fmt.Fprint(vtx.rec.Stderr(), msg) } else { // stdout - fmt.Fprint(vtx.Stdout(), msg) + fmt.Fprint(vtx.rec.Stdout(), msg) } } } @@ -249,10 +246,14 @@ func (d *progrockDisplay) processBuildkitProgress(ctx context.Context, ch chan * } } + for _, vtxID := range completedVertices { + delete(vertices, vtxID) + } + case <-ctx.Done(): // Context canceled (Ctrl+C), mark remaining vertices as done for _, vtx := range vertices { - vtx.Done(nil) + vtx.rec.Done(nil) } return ctx.Err() } diff --git a/pkg/tui/display_benchmark_test.go b/pkg/tui/display_benchmark_test.go new file mode 100644 index 000000000..fb28e9333 --- /dev/null +++ b/pkg/tui/display_benchmark_test.go @@ -0,0 +1,178 @@ +package tui + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/moby/buildkit/client" + "github.com/opencontainers/go-digest" + "github.com/vito/progrock" +) + +const ( + benchVertexCount = 1500 + benchTasksPerVertex = 8 + benchVertexBatchSize = 100 + benchStatusBatchSize = 250 + benchLogBatchSize = 200 + benchWarningEveryN = 100 + benchCompletedTaskName = "task-00" +) + +func BenchmarkProcessBuildkitProgressTaskBookkeeping(b *testing.B) { + events, wantWarnings := syntheticBuildkitProgressEvents(benchVertexCount, benchTasksPerVertex) + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + tape := progrock.NewTape() + display := &progrockDisplay{ + tape: tape, + rec: progrock.NewRecorder(tape), + } + warnings := make([]client.VertexWarning, 0, wantWarnings) + ch := make(chan *client.SolveStatus) + + go func() { + defer close(ch) + for _, event := range events { + ch <- event + } + }() + + if err := display.processBuildkitProgress(context.Background(), ch, &warnings); err != nil { + b.Fatal(err) + } + if got := len(warnings); got != wantWarnings { + b.Fatalf("warnings length = %d, want %d", got, wantWarnings) + } + if err := display.rec.Close(); err != nil { + b.Fatal(err) + } + } +} + +func syntheticBuildkitProgressEvents(vertexCount, tasksPerVertex int) ([]*client.SolveStatus, int) { + now := time.Unix(1_704_067_200, 0) + vertices := make([]digest.Digest, vertexCount) + for i := range vertices { + vertices[i] = digest.FromString(fmt.Sprintf("benchmark vertex %05d", i)) + } + taskNames := make([]string, tasksPerVertex) + for i := range taskNames { + taskNames[i] = fmt.Sprintf("task-%02d", i) + } + + events := make([]*client.SolveStatus, 0, vertexCount/benchVertexBatchSize+vertexCount*tasksPerVertex/benchStatusBatchSize+vertexCount/benchLogBatchSize) + + for start := 0; start < vertexCount; start += benchVertexBatchSize { + end := min(start+benchVertexBatchSize, vertexCount) + vertexes := make([]*client.Vertex, 0, end-start) + for i := start; i < end; i++ { + vertexes = append(vertexes, &client.Vertex{ + Digest: vertices[i], + Name: fmt.Sprintf("benchmark vertex %05d", i), + Started: &now, + }) + } + events = append(events, &client.SolveStatus{Vertexes: vertexes}) + } + + statuses := make([]*client.VertexStatus, 0, benchStatusBatchSize) + flushStatuses := func() { + if len(statuses) == 0 { + return + } + events = append(events, &client.SolveStatus{Statuses: statuses}) + statuses = make([]*client.VertexStatus, 0, benchStatusBatchSize) + } + for i, vtx := range vertices { + for taskID, taskName := range taskNames { + statuses = append(statuses, &client.VertexStatus{ + Vertex: vtx, + Name: taskName, + Current: int64((i + taskID) % 100), + Total: int64(100 + taskID), + }) + if len(statuses) == benchStatusBatchSize { + flushStatuses() + } + } + } + flushStatuses() + + // Complete one task on every vertex while leaving the rest active for vertex cleanup. + for _, vtx := range vertices { + statuses = append(statuses, &client.VertexStatus{ + Vertex: vtx, + Name: benchCompletedTaskName, + Current: 100, + Total: 100, + Completed: &now, + }) + if len(statuses) == benchStatusBatchSize { + flushStatuses() + } + } + flushStatuses() + + logs := make([]*client.VertexLog, 0, benchLogBatchSize) + flushLogs := func() { + if len(logs) == 0 { + return + } + events = append(events, &client.SolveStatus{Logs: logs}) + logs = make([]*client.VertexLog, 0, benchLogBatchSize) + } + for i, vtx := range vertices { + logs = append(logs, &client.VertexLog{ + Vertex: vtx, + Stream: 1, + Data: []byte(fmt.Sprintf("stdout log line %05d", i)), + }) + if len(logs) == benchLogBatchSize { + flushLogs() + } + logs = append(logs, &client.VertexLog{ + Vertex: vtx, + Stream: 2, + Data: []byte(fmt.Sprintf("stderr log line %05d\n", i)), + }) + if len(logs) == benchLogBatchSize { + flushLogs() + } + } + flushLogs() + + wantWarnings := 0 + warnings := make([]*client.VertexWarning, 0, vertexCount/benchWarningEveryN) + for i, vtx := range vertices { + if i%benchWarningEveryN != 0 { + continue + } + warnings = append(warnings, &client.VertexWarning{ + Vertex: vtx, + Short: []byte(fmt.Sprintf("synthetic warning %05d", i)), + }) + wantWarnings++ + } + events = append(events, &client.SolveStatus{Warnings: warnings}) + + for start := 0; start < vertexCount; start += benchVertexBatchSize { + end := min(start+benchVertexBatchSize, vertexCount) + vertexes := make([]*client.Vertex, 0, end-start) + for i := start; i < end; i++ { + vertexes = append(vertexes, &client.Vertex{ + Digest: vertices[i], + Name: fmt.Sprintf("benchmark vertex %05d", i), + Started: &now, + Completed: &now, + }) + } + events = append(events, &client.SolveStatus{Vertexes: vertexes}) + } + + return events, wantWarnings +} diff --git a/pkg/tui/display_test.go b/pkg/tui/display_test.go index 5041de617..caa3e89b3 100644 --- a/pkg/tui/display_test.go +++ b/pkg/tui/display_test.go @@ -353,6 +353,69 @@ func TestProcessBuildkitProgress_ProgressUpdates(t *testing.T) { assert.NoError(t, err) } +func TestProcessBuildkitProgress_ProcessesFinalStatusAndLogInCompletedVertexBatch(t *testing.T) { + tmpFile, err := os.CreateTemp("", "test") + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + defer tmpFile.Close() + + display, err := NewProgrockDisplay(tmpFile) + require.NoError(t, err) + + progrockDisp, ok := display.(*progrockDisplay) + require.True(t, ok, "expected *progrockDisplay") + ch := make(chan *client.SolveStatus) + warnings := []client.VertexWarning{} + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + now := time.Now() + vtxDigest := digest.FromString("completed-same-batch-vertex") + + go func() { + defer close(ch) + ch <- &client.SolveStatus{ + Vertexes: []*client.Vertex{ + { + Digest: vtxDigest, + Name: "completed same batch vertex", + Started: &now, + Completed: &now, + }, + }, + Statuses: []*client.VertexStatus{ + { + Vertex: vtxDigest, + Name: "final download", + Current: 100, + Total: 100, + Completed: &now, + }, + }, + Logs: []*client.VertexLog{ + { + Vertex: vtxDigest, + Stream: 1, + Data: []byte("final log line"), + }, + }, + } + }() + + err = progrockDisp.processBuildkitProgress(ctx, ch, &warnings) + assert.NoError(t, err) + + vertices := progrockDisp.tape.Vertices() + require.Len(t, vertices, 1) + activity := progrockDisp.tape.Activity(vertices[0]) + assert.Equal(t, int64(1), activity.TasksTotal) + assert.Equal(t, int64(1), activity.TasksCompleted) + assert.Equal(t, int64(100), activity.TaskBarsCurrent) + assert.Equal(t, int64(100), activity.TaskBarsTotal) + assert.Contains(t, activity.LastLine, "final log line") +} + func TestProcessBuildkitProgress_LogOutput(t *testing.T) { tmpFile, err := os.CreateTemp("", "test") require.NoError(t, err) diff --git a/pkg/vex/openvex.go b/pkg/vex/openvex.go index a6f87f228..09f50c6a8 100644 --- a/pkg/vex/openvex.go +++ b/pkg/vex/openvex.go @@ -26,6 +26,24 @@ func (o *OpenVex) CreateVEXDocument( patchedImageName string, pkgType string, ) (string, error) { + doc, err := o.createVEXDocument(updates, patchedImageName, pkgType) + if err != nil { + return "", err + } + + var buf bytes.Buffer + if err := doc.ToJSON(&buf); err != nil { + return "", err + } + + return buf.String(), nil +} + +func (o *OpenVex) createVEXDocument( + updates *unversioned.UpdateManifest, + patchedImageName string, + pkgType string, +) (*vex.VEX, error) { t := now() // construct a fresh VEX document per invocation (thread-safe, no shared state) doc := &vex.VEX{Metadata: vex.Metadata{ @@ -44,7 +62,7 @@ func (o *OpenVex) CreateVEXDocument( id, err := generateID(doc) if err != nil { - return "", err + return nil, err } doc.ID = id @@ -53,6 +71,14 @@ func (o *OpenVex) CreateVEXDocument( ID: "pkg:oci/" + patchedImageName, }, } + pt := utils.CanonicalPkgManagerType(pkgType) // base OS package manager type (apk, deb, rpm) + osPackagePrefix := "pkg:" + pt + "/" + updates.Metadata.OS.Type + "/" + osPackageQualifierSuffix := osPackageQualifiers(updates) + totalUpdates := len(updates.OSUpdates) + len(updates.LangUpdates) + initialStatementCapacity := min(totalUpdates, 512) + doc.Statements = make([]vex.Statement, 0, initialStatementCapacity) + statementByVulnerability := make(map[string]int, initialStatementCapacity) + seenSubcomponents := make(map[vexSubcomponentKey]struct{}, totalUpdates) // helper closure to add a single update (OS or language) to the VEX doc addUpdate := func(u unversioned.UpdatePackage) { @@ -71,9 +97,6 @@ func (o *OpenVex) CreateVEXDocument( log.Debugf("skipping update %s: fixed version equals installed version (%s)", u.Name, u.FixedVersion) return } - // Derive canonical package manager type (apk, deb, rpm) from the OS-level pkgType. - // For language packages (e.g. python-pkg), u.Type triggers a separate PURL scheme below. - pt := utils.CanonicalPkgManagerType(pkgType) // base OS package manager type (apk, deb, rpm) langType := u.Type // Use InstalledVersion (vulnerable) for BOM-VEX correlation: the subcomponent @@ -89,36 +112,31 @@ func (o *OpenVex) CreateVEXDocument( // Standard PyPI purl form: pkg:pypi/@ componentID = "pkg:pypi/" + u.Name + "@" + purlVersion } else { - // Build PURL qualifiers: arch is always present, distro added when OS version is known. - qualifiers := url.Values{} - qualifiers.Set("arch", updates.Metadata.Config.Arch) - if updates.Metadata.OS.Version != "" { - qualifiers.Set("distro", updates.Metadata.OS.Type+"-"+updates.Metadata.OS.Version) - } - componentID = "pkg:" + pt + "/" + updates.Metadata.OS.Type + "/" + u.Name + "@" + purlVersion + "?" + qualifiers.Encode() + componentID = osPackagePrefix + u.Name + "@" + purlVersion + osPackageQualifierSuffix } subComponent := vex.Subcomponent{Component: vex.Component{ID: componentID}} // if vulnerability id already exists, append subcomponent - for i := range doc.Statements { - if doc.Statements[i].Vulnerability.ID == u.VulnerabilityID { - // deduplicate identical subcomponent IDs - for _, existing := range doc.Statements[i].Products[0].Subcomponents { - if existing.ID == subComponent.ID { - log.Debugf("duplicate subcomponent %s ignored", subComponent.ID) - return - } - } - doc.Statements[i].Products[0].Subcomponents = append(doc.Statements[i].Products[0].Subcomponents, subComponent) + if statementIndex, ok := statementByVulnerability[u.VulnerabilityID]; ok { + // deduplicate identical subcomponent IDs + key := vexSubcomponentKey{vulnerabilityID: u.VulnerabilityID, componentID: subComponent.ID} + if _, ok := seenSubcomponents[key]; ok { + log.Debugf("duplicate subcomponent %s ignored", subComponent.ID) return } + seenSubcomponents[key] = struct{}{} + doc.Statements[statementIndex].Products[0].Subcomponents = append(doc.Statements[statementIndex].Products[0].Subcomponents, subComponent) + return } // otherwise create new statement - imageProduct.Subcomponents = []vex.Subcomponent{subComponent} + product := imageProduct + product.Subcomponents = []vex.Subcomponent{subComponent} doc.Statements = append(doc.Statements, vex.Statement{ Vulnerability: vex.Vulnerability{ID: u.VulnerabilityID}, - Products: []vex.Product{imageProduct}, + Products: []vex.Product{product}, Status: "fixed", }) + statementByVulnerability[u.VulnerabilityID] = len(doc.Statements) - 1 + seenSubcomponents[vexSubcomponentKey{vulnerabilityID: u.VulnerabilityID, componentID: subComponent.ID}] = struct{}{} } for _, u := range updates.OSUpdates { @@ -128,11 +146,19 @@ func (o *OpenVex) CreateVEXDocument( addUpdate(u) } - var buf bytes.Buffer - err = doc.ToJSON(&buf) - if err != nil { - return "", err - } + return doc, nil +} - return buf.String(), nil +type vexSubcomponentKey struct { + vulnerabilityID string + componentID string +} + +func osPackageQualifiers(updates *unversioned.UpdateManifest) string { + // Build PURL qualifiers: arch is always present, distro added when OS version is known. + qualifiers := "?arch=" + url.QueryEscape(updates.Metadata.Config.Arch) + if updates.Metadata.OS.Version != "" { + qualifiers += "&distro=" + url.QueryEscape(updates.Metadata.OS.Type+"-"+updates.Metadata.OS.Version) + } + return qualifiers } diff --git a/pkg/vex/openvex_benchmark_test.go b/pkg/vex/openvex_benchmark_test.go new file mode 100644 index 000000000..459921b6a --- /dev/null +++ b/pkg/vex/openvex_benchmark_test.go @@ -0,0 +1,101 @@ +package vex + +import ( + "fmt" + "os" + "path/filepath" + "testing" + "time" + + openvex "github.com/openvex/go-vex/pkg/vex" + "github.com/project-copacetic/copacetic/pkg/types/unversioned" + "github.com/project-copacetic/copacetic/pkg/utils" +) + +func BenchmarkOpenVexCreateVEXDocumentLargeManifest(b *testing.B) { + updates := benchmarkVEXManifest(2400, 400) + o := &OpenVex{} + + backupNow := now + now = func() time.Time { return time.Unix(1700000000, 0).UTC() } + defer func() { now = backupNow }() + + backupID := generateID + generateID = func(_ *openvex.VEX) (string, error) { return "https://openvex.dev/benchmark", nil } + defer func() { generateID = backupID }() + + b.ReportAllocs() + b.SetBytes(int64(len(updates.OSUpdates) + len(updates.LangUpdates))) + for i := 0; i < b.N; i++ { + doc, err := o.CreateVEXDocument(updates, "registry.example.com/team/app:patched", "deb") + if err != nil { + b.Fatal(err) + } + if len(doc) == 0 { + b.Fatal("empty VEX document") + } + } +} + +func BenchmarkTryOutputVexDocumentLargeManifest(b *testing.B) { + updates := benchmarkVEXManifest(2400, 400) + outDir := b.TempDir() + + backupNow := now + now = func() time.Time { return time.Unix(1700000000, 0).UTC() } + defer func() { now = backupNow }() + + backupID := generateID + generateID = func(_ *openvex.VEX) (string, error) { return "https://openvex.dev/benchmark", nil } + defer func() { generateID = backupID }() + + b.ReportAllocs() + b.SetBytes(int64(len(updates.OSUpdates) + len(updates.LangUpdates))) + for i := 0; i < b.N; i++ { + outputPath := filepath.Join(outDir, fmt.Sprintf("vex-%06d.json", i)) + if err := TryOutputVexDocument(updates, "deb", "registry.example.com/team/app:patched", "openvex", outputPath); err != nil { + b.Fatal(err) + } + if err := os.Remove(outputPath); err != nil { + b.Fatal(err) + } + } +} + +func benchmarkVEXManifest(totalUpdates, uniqueVulns int) *unversioned.UpdateManifest { + updates := &unversioned.UpdateManifest{ + Metadata: unversioned.Metadata{ + OS: unversioned.OS{Type: "debian", Version: "12"}, + Config: unversioned.Config{Arch: "amd64"}, + }, + OSUpdates: make([]unversioned.UpdatePackage, 0, totalUpdates), + LangUpdates: make([]unversioned.UpdatePackage, 0, totalUpdates/8), + } + + for i := 0; i < totalUpdates; i++ { + vulnID := fmt.Sprintf("CVE-2024-%04d", i%uniqueVulns) + pkgName := fmt.Sprintf("libexample-%04d", i) + // Every tenth item intentionally duplicates an earlier package/vuln pair so + // the benchmark covers subcomponent de-duplication as well as grouping. + if i >= uniqueVulns && i%10 == 0 { + pkgName = fmt.Sprintf("libexample-%04d", i%uniqueVulns) + } + updates.OSUpdates = append(updates.OSUpdates, unversioned.UpdatePackage{ + Name: pkgName, + InstalledVersion: fmt.Sprintf("1.%d.%d", i%17, i%29), + FixedVersion: fmt.Sprintf("1.%d.%d", i%17, (i%29)+1), + VulnerabilityID: vulnID, + }) + } + + for i := 0; i < totalUpdates/8; i++ { + updates.LangUpdates = append(updates.LangUpdates, unversioned.UpdatePackage{ + Name: fmt.Sprintf("python-package-%04d", i), + InstalledVersion: fmt.Sprintf("0.%d.%d", i%11, i%13), + FixedVersion: fmt.Sprintf("0.%d.%d", i%11, (i%13)+1), + VulnerabilityID: fmt.Sprintf("PYSEC-2024-%04d", i%(uniqueVulns/4)), + Type: utils.PythonPackages, + }) + } + return updates +} diff --git a/pkg/vex/vex.go b/pkg/vex/vex.go index 73a6d985d..b344672b5 100644 --- a/pkg/vex/vex.go +++ b/pkg/vex/vex.go @@ -1,8 +1,11 @@ package vex import ( + "bytes" "fmt" + "io" "os" + "path/filepath" "github.com/project-copacetic/copacetic/pkg/pkgmgr" "github.com/project-copacetic/copacetic/pkg/types/unversioned" @@ -13,18 +16,57 @@ type Vex interface { } func TryOutputVexDocument(updates *unversioned.UpdateManifest, pkgType, patchedImageName, format, file string) error { - var doc string - var err error - switch format { case "openvex": ov := &OpenVex{} - doc, err = ov.CreateVEXDocument(updates, patchedImageName, pkgType) + doc, err := ov.createVEXDocument(updates, patchedImageName, pkgType) if err != nil { return err } + return writeVEXDocumentFile(file, doc.ToJSON) default: return fmt.Errorf("unsupported output format %s specified", format) } - return os.WriteFile(file, []byte(doc), 0o600) +} + +func writeVEXDocumentFile(file string, write func(io.Writer) error) error { + if _, err := os.Lstat(file); err == nil { + return writeBufferedVEXDocumentFile(file, write) + } else if !os.IsNotExist(err) { + return err + } + + dir := filepath.Dir(file) + tmpFile, err := os.CreateTemp(dir, "."+filepath.Base(file)+".tmp-*") + if err != nil { + return err + } + tmpName := tmpFile.Name() + succeeded := false + defer func() { + if !succeeded { + _ = os.Remove(tmpName) + } + }() + + if err := write(tmpFile); err != nil { + _ = tmpFile.Close() + return err + } + if err := tmpFile.Close(); err != nil { + return err + } + if err := os.Rename(tmpName, file); err != nil { + return err + } + succeeded = true + return nil +} + +func writeBufferedVEXDocumentFile(file string, write func(io.Writer) error) error { + var buf bytes.Buffer + if err := write(&buf); err != nil { + return err + } + return os.WriteFile(file, buf.Bytes(), 0o600) } diff --git a/pkg/vex/vex_test.go b/pkg/vex/vex_test.go index c3c0338f2..ac5cfaaa8 100644 --- a/pkg/vex/vex_test.go +++ b/pkg/vex/vex_test.go @@ -1,6 +1,10 @@ package vex import ( + "errors" + "io" + "os" + "path/filepath" "testing" "github.com/project-copacetic/copacetic/pkg/buildkit" @@ -9,6 +13,8 @@ import ( "github.com/project-copacetic/copacetic/pkg/utils" ) +const existingVEXContent = "existing vex content" + func TestTryOutputVexDocument(t *testing.T) { config := &buildkit.Config{} alpineManager, _ := pkgmgr.GetPackageManager("alpine", "", config, utils.DefaultTempWorkingFolder) @@ -61,3 +67,152 @@ func TestTryOutputVexDocument(t *testing.T) { }) } } + +func TestWriteVEXDocumentFilePreservesExistingFileOnWriteError(t *testing.T) { + dir := t.TempDir() + outputPath := filepath.Join(dir, "vex.json") + if err := os.WriteFile(outputPath, []byte(existingVEXContent), 0o600); err != nil { + t.Fatalf("failed to seed output file: %v", err) + } + + writeErr := errors.New("write failed") + err := writeVEXDocumentFile(outputPath, func(w io.Writer) error { + if _, err := w.Write([]byte("partial")); err != nil { + return err + } + return writeErr + }) + if !errors.Is(err, writeErr) { + t.Fatalf("writeVEXDocumentFile() error = %v, want %v", err, writeErr) + } + + got, err := os.ReadFile(outputPath) + if err != nil { + t.Fatalf("failed to read output file: %v", err) + } + if string(got) != existingVEXContent { + t.Fatalf("output file content = %q, want %q", got, existingVEXContent) + } + + tmpFiles, err := filepath.Glob(filepath.Join(dir, ".vex.json.tmp-*")) + if err != nil { + t.Fatalf("failed to glob temp files: %v", err) + } + if len(tmpFiles) != 0 { + t.Fatalf("temporary files were not cleaned up: %v", tmpFiles) + } +} + +func TestWriteVEXDocumentFileReplacesOutputOnSuccess(t *testing.T) { + dir := t.TempDir() + outputPath := filepath.Join(dir, "vex.json") + if err := os.WriteFile(outputPath, []byte(existingVEXContent), 0o600); err != nil { + t.Fatalf("failed to seed output file: %v", err) + } + + const updatedContent = "updated vex content" + if err := writeVEXDocumentFile(outputPath, func(w io.Writer) error { + _, err := w.Write([]byte(updatedContent)) + return err + }); err != nil { + t.Fatalf("writeVEXDocumentFile() error = %v", err) + } + + got, err := os.ReadFile(outputPath) + if err != nil { + t.Fatalf("failed to read output file: %v", err) + } + if string(got) != updatedContent { + t.Fatalf("output file content = %q, want %q", got, updatedContent) + } +} + +func TestWriteVEXDocumentFilePreservesExistingFileMode(t *testing.T) { + dir := t.TempDir() + outputPath := filepath.Join(dir, "vex.json") + const existingMode = 0o640 + if err := os.WriteFile(outputPath, []byte(existingVEXContent), existingMode); err != nil { + t.Fatalf("failed to seed output file: %v", err) + } + + if err := writeVEXDocumentFile(outputPath, func(w io.Writer) error { + _, err := w.Write([]byte("updated")) + return err + }); err != nil { + t.Fatalf("writeVEXDocumentFile() error = %v", err) + } + + info, err := os.Stat(outputPath) + if err != nil { + t.Fatalf("failed to stat output file: %v", err) + } + if got := info.Mode().Perm(); got != existingMode { + t.Fatalf("output file mode = %v, want %v", got, os.FileMode(existingMode)) + } +} + +func TestWriteVEXDocumentFileFollowsExistingSymlink(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "target-vex.json") + linkPath := filepath.Join(dir, "vex-link.json") + if err := os.WriteFile(targetPath, []byte(existingVEXContent), 0o600); err != nil { + t.Fatalf("failed to seed target file: %v", err) + } + if err := os.Symlink(targetPath, linkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + const updatedContent = "updated through symlink" + if err := writeVEXDocumentFile(linkPath, func(w io.Writer) error { + _, err := w.Write([]byte(updatedContent)) + return err + }); err != nil { + t.Fatalf("writeVEXDocumentFile() error = %v", err) + } + + linkInfo, err := os.Lstat(linkPath) + if err != nil { + t.Fatalf("failed to lstat symlink: %v", err) + } + if linkInfo.Mode()&os.ModeSymlink == 0 { + t.Fatalf("output path is no longer a symlink: mode=%v", linkInfo.Mode()) + } + + got, err := os.ReadFile(targetPath) + if err != nil { + t.Fatalf("failed to read symlink target: %v", err) + } + if string(got) != updatedContent { + t.Fatalf("target content = %q, want %q", got, updatedContent) + } +} + +func TestWriteVEXDocumentFileUpdatesExistingFileWithoutDirectoryWrite(t *testing.T) { + dir := t.TempDir() + outputPath := filepath.Join(dir, "vex.json") + if err := os.WriteFile(outputPath, []byte(existingVEXContent), 0o600); err != nil { + t.Fatalf("failed to seed output file: %v", err) + } + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatalf("failed to remove directory write permission: %v", err) + } + t.Cleanup(func() { + _ = os.Chmod(dir, 0o700) + }) + + const updatedContent = "updated without directory write" + if err := writeVEXDocumentFile(outputPath, func(w io.Writer) error { + _, err := w.Write([]byte(updatedContent)) + return err + }); err != nil { + t.Fatalf("writeVEXDocumentFile() error = %v", err) + } + + got, err := os.ReadFile(outputPath) + if err != nil { + t.Fatalf("failed to read output file: %v", err) + } + if string(got) != updatedContent { + t.Fatalf("output file content = %q, want %q", got, updatedContent) + } +}