Skip to content

Commit

Permalink
Return committed readonly inputs and actives in exec error in MountIDs
Browse files Browse the repository at this point in the history
Signed-off-by: Edgar Lee <edgarl@netflix.com>
  • Loading branch information
hinshun committed Nov 16, 2020
1 parent 3ba6cd7 commit 1240dd7
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 37 deletions.
13 changes: 12 additions & 1 deletion client/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,17 @@ func testClientGatewayExecError(t *testing.T, sb integration.Sandbox) {
llb.AddMount("/readonly", llb.Scratch(), llb.Readonly),
).Root(),
2, []string{"/data"},
}, {
"rootfs and readwrite force no output mount",
llb.Image("busybox:latest").Run(
llb.Shlexf(`sh -c "echo %s > /data && echo %s > /rw/data && fail"`, id, id),
llb.AddMount(
"/rw",
llb.Scratch().File(llb.Mkfile("foo", 0700, []byte(id))),
llb.ForceNoOutput,
),
).Root(),
2, []string{"/data", "/rw/data", "/rw/foo"},
}}

for _, tt := range tests {
Expand Down Expand Up @@ -1216,7 +1227,7 @@ func testClientGatewayExecError(t *testing.T, sb integration.Sandbox) {
_, err = c.Build(ctx, SolveOpt{}, "buildkit_test", b, nil)
require.NoError(t, err)

// checkAllReleasable(t, c, sb, true)
checkAllReleasable(t, c, sb, true)
}

// testClientGatewaySlowCacheExecError is testing gateway exec into the ref
Expand Down
38 changes: 26 additions & 12 deletions frontend/gateway/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func NewContainer(ctx context.Context, w worker.Worker, sm *session.Manager, g s
})
if err != nil {
for i := len(p.Actives) - 1; i >= 0; i-- { // call in LIFO order
p.Actives[i].Release(context.TODO())
p.Actives[i].Ref.Release(context.TODO())
}
for _, o := range p.OutputRefs {
o.Ref.Release(context.TODO())
Expand All @@ -104,7 +104,7 @@ func NewContainer(ctx context.Context, w worker.Worker, sm *session.Manager, g s
for _, active := range p.Actives {
active := active
ctr.cleanup = append(ctr.cleanup, func() error {
return active.Release(context.TODO())
return active.Ref.Release(context.TODO())
})
}

Expand All @@ -115,15 +115,20 @@ type PreparedMounts struct {
Root executor.Mount
ReadonlyRootFS bool
Mounts []executor.Mount
OutputRefs []OutputRef
Actives []cache.MutableRef
OutputRefs []MountRef
Actives []MountMutableRef
}

type OutputRef struct {
type MountRef struct {
Ref cache.Ref
MountIndex int
}

type MountMutableRef struct {
Ref cache.MutableRef
MountIndex int
}

type MakeMutable func(m *opspb.Mount, ref cache.ImmutableRef) (cache.MutableRef, error)

func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manager, g session.Group, mnts []*opspb.Mount, refs []*worker.WorkerRef, makeMutable MakeMutable) (p PreparedMounts, err error) {
Expand All @@ -140,7 +145,7 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage

// if mount is based on input validate and load it
if m.Input != opspb.Empty {
if int(m.Input) > len(refs) {
if int(m.Input) >= len(refs) {
return p, errors.Errorf("missing input %d", m.Input)
}
ref = refs[int(m.Input)].ImmutableRef
Expand All @@ -153,7 +158,7 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage
if m.Output != opspb.SkipOutput {
// if it is readonly and not root then output is the input
if m.Readonly && ref != nil && m.Dest != opspb.RootMount {
p.OutputRefs = append(p.OutputRefs, OutputRef{
p.OutputRefs = append(p.OutputRefs, MountRef{
MountIndex: i,
Ref: ref.Clone(),
})
Expand All @@ -164,7 +169,7 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage
return p, err
}
mountable = active
p.OutputRefs = append(p.OutputRefs, OutputRef{
p.OutputRefs = append(p.OutputRefs, MountRef{
MountIndex: i,
Ref: active,
})
Expand All @@ -175,7 +180,10 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage
if err != nil {
return p, err
}
p.Actives = append(p.Actives, active)
p.Actives = append(p.Actives, MountMutableRef{
MountIndex: i,
Ref: active,
})
mountable = active
}

Expand All @@ -185,9 +193,12 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage
return p, err
}
mountable = active
p.Actives = append(p.Actives, active)
p.Actives = append(p.Actives, MountMutableRef{
MountIndex: i,
Ref: active,
})
if m.Output != opspb.SkipOutput && ref != nil {
p.OutputRefs = append(p.OutputRefs, OutputRef{
p.OutputRefs = append(p.OutputRefs, MountRef{
MountIndex: i,
Ref: ref.Clone(),
})
Expand Down Expand Up @@ -232,7 +243,10 @@ func PrepareMounts(ctx context.Context, mm *mounts.MountManager, cm cache.Manage
if err != nil {
return p, err
}
p.Actives = append(p.Actives, active)
p.Actives = append(p.Actives, MountMutableRef{
MountIndex: i,
Ref: active,
})
root = active
}
p.Root = mountWithSession(root, g)
Expand Down
7 changes: 6 additions & 1 deletion solver/jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/moby/buildkit/client"
"github.com/moby/buildkit/session"
"github.com/moby/buildkit/solver/errdefs"
"github.com/moby/buildkit/solver/pb"

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Nov 16, 2020

Member

This dependency should really be avoided as solver is not LLB specific. Could be follow-up if it is problematic.

This comment has been minimized.

Copy link
@hinshun

hinshun Nov 16, 2020

Author Collaborator

I removed the dependency here:
fa8a02c

"github.com/moby/buildkit/util/flightcontrol"
"github.com/moby/buildkit/util/progress"
"github.com/moby/buildkit/util/tracing"
Expand Down Expand Up @@ -621,7 +622,9 @@ func (s *sharedOp) LoadCache(ctx context.Context, rec *CacheRecord) (Result, err
// evaluated, hence "slow" cache.
func (s *sharedOp) CalcSlowCache(ctx context.Context, index Index, p PreprocessFunc, f ResultBasedCacheFunc, res Result) (dgst digest.Digest, err error) {
defer func() {
err = errdefs.WrapVertex(WrapSlowCache(err, index, NewSharedResult(res).Clone()), s.st.origDigest)
err = WrapSlowCache(err, index, NewSharedResult(res).Clone())
err = errdefs.WithOp(err, s.st.vtx.Sys().(*pb.Op))
err = errdefs.WrapVertex(err, s.st.origDigest)
}()
key, err := s.g.Do(ctx, fmt.Sprintf("slow-compute-%d", index), func(ctx context.Context) (interface{}, error) {
s.slowMu.Lock()
Expand Down Expand Up @@ -683,6 +686,7 @@ func (s *sharedOp) CalcSlowCache(ctx context.Context, index Index, p PreprocessF

func (s *sharedOp) CacheMap(ctx context.Context, index int) (resp *cacheMapResp, err error) {
defer func() {
err = errdefs.WithOp(err, s.st.vtx.Sys().(*pb.Op))
err = errdefs.WrapVertex(err, s.st.origDigest)
}()
op, err := s.getOp()
Expand Down Expand Up @@ -741,6 +745,7 @@ func (s *sharedOp) CacheMap(ctx context.Context, index int) (resp *cacheMapResp,

func (s *sharedOp) Exec(ctx context.Context, inputs []Result) (outputs []Result, exporters []ExportableCacheKey, err error) {
defer func() {
err = errdefs.WithOp(err, s.st.vtx.Sys().(*pb.Op))
err = errdefs.WrapVertex(err, s.st.origDigest)
}()
op, err := s.getOp()
Expand Down
15 changes: 0 additions & 15 deletions solver/llbsolver/bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,21 +198,6 @@ func (rp *resultProxy) wrapError(err error) error {
}
}
}

var op *pb.Op
for _, dt := range rp.def.Def {
var curr pb.Op
if err := (&curr).Unmarshal(dt); err != nil {
return errors.Wrap(err, "failed to parse llb proto op")
}
if ve.Digest == digest.FromBytes(dt).String() {
op = &curr
break
}
}
if op != nil {
err = errdefs.WithOp(err, op)
}
}
return err
}
Expand Down
27 changes: 19 additions & 8 deletions solver/llbsolver/ops/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,14 +229,6 @@ func (e *execOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
return e.cm.New(ctx, ref, g, cache.WithDescription(desc))
})
defer func() {
for i := len(p.Actives) - 1; i >= 0; i-- { // call in LIFO order
p.Actives[i].Release(context.TODO())
}
for _, o := range p.OutputRefs {
if o.Ref != nil {
o.Ref.Release(context.TODO())
}
}
if err != nil {
execInputs := make([]solver.Result, len(e.op.Mounts))
for i, m := range e.op.Mounts {
Expand All @@ -246,10 +238,29 @@ func (e *execOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
execInputs[i] = inputs[m.Input]
}
execMounts := make([]solver.Result, len(e.op.Mounts))
copy(execMounts, execInputs)
for i, res := range results {
execMounts[p.OutputRefs[i].MountIndex] = res
}
for _, active := range p.Actives {
ref, cerr := active.Ref.Commit(ctx)
if cerr != nil {
err = errors.Wrapf(err, "error committing %s: %s", active.Ref.ID(), cerr)
continue
}
execMounts[active.MountIndex] = worker.NewWorkerRefResult(ref, e.w)
}
err = errdefs.WithExecError(err, execInputs, execMounts)
} else {
// Only release actives if err is nil.
for i := len(p.Actives) - 1; i >= 0; i-- { // call in LIFO order
p.Actives[i].Ref.Release(context.TODO())
}
}
for _, o := range p.OutputRefs {
if o.Ref != nil {
o.Ref.Release(context.TODO())
}
}
}()
if err != nil {
Expand Down

0 comments on commit 1240dd7

Please sign in to comment.