Repository: openllb/hlb
Pull request #336 (2022-09-10 18:17:03)

Remove SessionID from local op
This ends up referring to an ID that isn't used by the actual session, so it causes problems. For example, "local" ops take at least 5 seconds because they try to wait for a session with this ID that never shows up: https://github.com/moby/buildkit/blob/abde08a5531d809a395cf648a31bca932b009af0/source/local/local.go#L95-L101

Ideally, we should set `SessionID` to the actual session ID instead of leaving it unspecified, because this relies on buildkit to iterate over the sessions and find one that works, which can be nondeterministic. But it seems like this would take a pretty significant refactor to create the session in advance, and then add `FSSyncProvider`s as we go. The change here should be a strict improvement over the current behavior.


Patch:
diff --git a/codegen/builtin_fs.go b/codegen/builtin_fs.go
index c8e4a47..9d8b1d7 100644
--- a/codegen/builtin_fs.go
+++ b/codegen/builtin_fs.go
@@ -239,12 +239,10 @@ func (l Local) Call(ctx context.Context, cln *client.Client, val Value, opts Opt
 	if err != nil {
 		return nil, err
 	}
-	localOpts = append(localOpts, llb.SharedKeyHint(id))
-
-	sessionID := SessionID(ctx)
-	if sessionID != "" {
-		localOpts = append(localOpts, llb.SessionID(sessionID))
-	}
+	localOpts = append(localOpts,
+		llb.SharedKeyHint(id),
+		llb.LocalUniqueID(id),
+	)
 
 	fs := Filesystem{
 		State:    llb.Local(localPath, localOpts...),
@@ -338,6 +336,7 @@ func (f Frontend) Call(ctx context.Context, cln *client.Client, val Value, opts
 				if err != nil {
 					return
 				}
+				fs.SessionOpts = sessionOpts
 			}
 
 			imageSpec, ok := res.Metadata[llbutil.KeyContainerImageConfig]
diff --git a/codegen/context.go b/codegen/context.go
index 58fd562..fad507d 100644
--- a/codegen/context.go
+++ b/codegen/context.go
@@ -27,7 +27,6 @@ type (
 	returnTypeKey      struct{}
 	argKey             struct{ n int }
 	bindingKey         struct{}
-	sessionIDKey       struct{}
 	multiwriterKey     struct{}
 	imageResolverKey   struct{}
 	backtraceKey       struct{}
@@ -99,15 +98,6 @@ func Arg(ctx context.Context, n int) ast.Node {
 	return arg
 }
 
-func WithSessionID(ctx context.Context, sessionID string) context.Context {
-	return context.WithValue(ctx, sessionIDKey{}, sessionID)
-}
-
-func SessionID(ctx context.Context) string {
-	sessionID, _ := ctx.Value(sessionIDKey{}).(string)
-	return sessionID
-}
-
 func WithMultiWriter(ctx context.Context, mw *solver.MultiWriter) context.Context {
 	return context.WithValue(ctx, multiwriterKey{}, mw)
 }
diff --git a/hlb.go b/hlb.go
index 5f81494..cc17bb8 100644
--- a/hlb.go
+++ b/hlb.go
@@ -6,7 +6,6 @@ import (
 	"io"
 
 	"github.com/moby/buildkit/client"
-	"github.com/moby/buildkit/identity"
 	"github.com/openllb/hlb/builtin"
 	"github.com/openllb/hlb/checker"
 	"github.com/openllb/hlb/codegen"
@@ -56,7 +55,6 @@ func Compile(ctx context.Context, cln *client.Client, w io.Writer, mod *ast.Modu
 	}
 
 	cg := codegen.New(cln, resolver)
-	ctx = codegen.WithSessionID(ctx, identity.NewID())
 	if solver.ConcurrencyLimiter(ctx) == nil {
 		ctx = solver.WithConcurrencyLimiter(ctx, semaphore.NewWeighted(defaultMaxConcurrency))
 	}
diff --git a/pkg/llbutil/session.go b/pkg/llbutil/session.go
index 39c2ee5..e39270c 100644
--- a/pkg/llbutil/session.go
+++ b/pkg/llbutil/session.go
@@ -119,10 +119,6 @@ func NewSession(ctx context.Context, opts ...SessionOption) (*session.Session, e
 	// between `llb.SharedKeyHint` and a session's shared key atm. If anything
 	// needs to start leveraging the session's shared key in the future, we
 	// should probably use the codegen.Session(ctx) session id.
-	//
-	// For now, locals also have `llb.LocalUniqueID` that introduces a random
-	// unique ID if a session isn't provided, so regardless of session shared key
-	// being provided or not, we still need to use `llb.SharedKeyHint`.
 	s, err := session.NewSession(ctx, "hlb", "")
 	if err != nil {
 		return s, err


Test patch:
diff --git a/codegen/codegen_test.go b/codegen/codegen_test.go
index 8c9eb33..cb64025 100644
--- a/codegen/codegen_test.go
+++ b/codegen/codegen_test.go
@@ -11,7 +11,6 @@ import (
 
 	"github.com/lithammer/dedent"
 	"github.com/moby/buildkit/client/llb"
-	"github.com/moby/buildkit/identity"
 	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/entitlements"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
@@ -57,13 +56,9 @@ func LocalState(ctx context.Context, t *testing.T, localPath string, opts ...llb
 
 	opts = append([]llb.LocalOption{
 		llb.SharedKeyHint(id),
+		llb.LocalUniqueID(id),
 	}, opts...)
 
-	sessionID := codegen.SessionID(ctx)
-	if sessionID != "" {
-		opts = append(opts, llb.SessionID(sessionID))
-	}
-
 	return llb.Local(localPath, opts...)
 }
 
@@ -987,7 +982,6 @@ func TestCodeGen(t *testing.T) {
 			}
 
 			cg := codegen.New(nil, nil)
-			ctx = codegen.WithSessionID(ctx, identity.NewID())
 			request, err := cg.Generate(ctx, mod, targets)
 			require.NoError(t, err, tc.name)
 
@@ -1057,7 +1051,6 @@ func TestCodegenError(t *testing.T) {
 			}
 
 			cg := codegen.New(nil, nil)
-			ctx = codegen.WithSessionID(ctx, identity.NewID())
 			_, err = cg.Generate(ctx, mod, targets)
 			var expected error
 			if tc.fn != nil {