Skip to content

history api: support for logs of completed builds#3339

Merged
tonistiigi merged 11 commits intomoby:masterfrom
tonistiigi:history-api-logs
Dec 13, 2022
Merged

history api: support for logs of completed builds#3339
tonistiigi merged 11 commits intomoby:masterfrom
tonistiigi:history-api-logs

Conversation

@tonistiigi
Copy link
Copy Markdown
Member

buildctl debug logs <refid> now also works for completed builds. The logs are kept in contentstore with a simple length-based encoding of the proto format.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

@tonistiigi tonistiigi force-pushed the history-api-logs branch 2 times, most recently from b45a3a9 to f427054 Compare November 30, 2022 14:44
Comment thread client/status.go
@tonistiigi tonistiigi added this to the v0.11.0 milestone Dec 1, 2022
@tonistiigi tonistiigi force-pushed the history-api-logs branch 2 times, most recently from 602e785 to dbfbbcd Compare December 6, 2022 05:54
@tonistiigi tonistiigi marked this pull request as ready for review December 7, 2022 06:06
@tonistiigi tonistiigi force-pushed the history-api-logs branch 2 times, most recently from e557e2e to f7ddfe1 Compare December 7, 2022 07:52
Comment thread solver/llbsolver/history.go Outdated
Comment on lines +519 to +553
if req.NoWait {
return nil
}
Copy link
Copy Markdown
Member

@crazy-max crazy-max Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see OpenBlobWriter has been introduced to save tracing. Is the purpose of NoWait to avoid waiting for the history to be loaded in case it's being deleted/gc?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoWait is if the client only wants to get a list of builds once and keep waiting on updates for them. Currently, the only use case is in the tests where because some tests check for cleaned contentstore the full build history also needs to be cleaned.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename it to Exit or EarlyExit or similar?

if s := trace.SpanFromContext(ctx); s.SpanContext().IsValid() {
if exp, err := detect.Exporter(); err == nil {
if rec, ok := exp.(*detect.TraceRecorder); ok {
stopTrace = rec.Record(s.SpanContext().TraceID())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are gathering spans for the build and save them to the history now. TraceID() is used to be sure we capture the right traces for the build right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We filter out the tracing info only for the same traceID that was set in the tracing context of the solve function. So two solves get their own individual traces.

@tonistiigi tonistiigi force-pushed the history-api-logs branch 6 times, most recently from 0f91453 to c61c8e6 Compare December 8, 2022 02:31
Comment thread cmd/buildctl/debug/ctl.go
Comment thread control/control.go
Comment thread solver/llbsolver/history.go Outdated
Comment on lines +519 to +553
if req.NoWait {
return nil
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename it to Exit or EarlyExit or similar?

Comment thread solver/llbsolver/history.go
Comment thread solver/llbsolver/history.go
Comment thread solver/llbsolver/history.go Outdated
Comment thread control/control.go Outdated
Comment thread solver/llbsolver/history.go
@thaJeztah
Copy link
Copy Markdown
Member

Will there be something on build to read back the logs? (see #1837)

@tonistiigi tonistiigi mentioned this pull request Dec 8, 2022
@tonistiigi
Copy link
Copy Markdown
Member Author

Will there be something on build to read back the logs?

buildctl debug logs <ref>

@tonistiigi tonistiigi force-pushed the history-api-logs branch 2 times, most recently from 0717a26 to d1129c3 Compare December 12, 2022 08:14
@jedevc
Copy link
Copy Markdown
Member

jedevc commented Dec 12, 2022

CI is red 👀

Copy link
Copy Markdown
Member

@jedevc jedevc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provenance refactor looks good to me, few minor comments, but happy for this to merge once CI is green and comments are fixed.

Comment thread cmd/buildkitd/config/config.go
Comment thread solver/llbsolver/solver.go Outdated
if err != nil {
return nil, err
}
func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend.SolveRequest, j *solver.Job) (func(*Result, exporter.DescriptorReference, error) error, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a tracking issue for SBOM support with build history if we're not doing it in this PR.

Comment thread solver/jobs.go Outdated
@tonistiigi tonistiigi force-pushed the history-api-logs branch 3 times, most recently from 93d336e to bbb57ea Compare December 12, 2022 22:22
@tonistiigi
Copy link
Copy Markdown
Member Author

TestIntegration/TestStargzLazyRegistryCacheImportExport/worker=containerd-snapshotter-stargz seems to be the only one left failing atm. @ktock Could you take a look as I don't really understand what it is testing and if the failing condition is valid. Otherwise, I suggest skipping this test for now, as it seems to be some stargz specific optimization behavior. The main difference with this PR is that ExportTo that was previously only called on --cache-to is now called with all builds.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@ktock
Copy link
Copy Markdown
Collaborator

ktock commented Dec 13, 2022

@tonistiigi Thank you for notifying me.

Is it an expected bahavior of this PR that BuildKit (with containerd worker configuration) doesn't cleanup containerd content store when pruning the cache all?
Master version (40bc1b3) cleans up the content store when doing buildctl prune --all but this PR doesn't seem to do so.
This is observed on overlayfs snapshotter as well.

  • buildkitd flags: --oci-worker=false --containerd-worker=true
# mkdir -p /tmp/ctx-a && cat <<EOF > /tmp/ctx-a/Dockerfile
FROM ubuntu:22.04
EOF
# buildctl build --progress=plain --frontend=dockerfile.v0 \
               --local context=/tmp/ctx-a --local dockerfile=/tmp/ctx-a \
               --output type=oci,dest=/tmp/test.tar
# buildctl prune --all
# buildctl du
ID	RECLAIMABLE	SIZE	LAST ACCESSED
Reclaimable:	0B
Total:		0B
# ctr --namespace=buildkit content ls
DIGEST									SIZE	AGE		LABELS
sha256:0585b60588963c4c76ac974991ddb8943760ea70c9e781b4d43ec023856dd37d	3.613kB	5 seconds	-
sha256:59329cca1b6c70654247470c3039b0076f3cffdc9ab32cce2f1006f404ab5642	481B	6 seconds	containerd.io/gc.ref.content.0=sha256:f35120b15f00b87e24dcf48439e45507e56802b121a7c8ebf7f1ddafe69d88e6,containerd.io/gc.ref.content.1=sha256:6e3729cf69e0ce2de9e779575a1fec8b7fb5efdfa822829290ab6d5d1bc3e797
sha256:5e44ffe94256b613961a33356a6710f668e01e66ffd00000f535b10b92c6bac0	3.585kB	5 seconds	-
sha256:6e3729cf69e0ce2de9e779575a1fec8b7fb5efdfa822829290ab6d5d1bc3e797	30.43MB	5 seconds	buildkit.io/blob/annotation.containerd.io/uncompressed=sha256:6515074984c6f8bb1b8a9962c8fb5f310fc85e70b04c88442a3939c026dbfad3,buildkit.io/blob/mediatype=application/vnd.docker.image.rootfs.diff.tar.gzip,containerd.io/gc.ref.content.blob-sha256:6e3729cf69e0ce2de9e779575a1fec8b7fb5efdfa822829290ab6d5d1bc3e797=sha256:6e3729cf69e0ce2de9e779575a1fec8b7fb5efdfa822829290ab6d5d1bc3e797
sha256:7f9e7e014d150b47278fef38fe32896eb1e73c6c43bfe6a4842ade77ede515e6	258.5kB	2 seconds	-
sha256:f35120b15f00b87e24dcf48439e45507e56802b121a7c8ebf7f1ddafe69d88e6	870B	6 seconds	-

TestIntegration/TestStargzLazyRegistryCacheImportExport/worker=containerd-snapshotter-stargz assumes prune API cleans up containerd content store as well. The test failure seems to related to this difference.

Previously this was not issue as manifest was deleted after
build completed but now the manifest is kept in build history API.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Copy Markdown
Member Author

@ktock Thanks. That case should be fixed with 2be7e54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants