Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,11 @@ func detectSharedMounts(ctx context.Context, reqs map[string][]*reqForNode) (_ m
m := map[string]map[fsKey]*fsTracker{}
for _, reqs := range reqs {
for _, req := range reqs {
nodeName := req.ResolvedNode.Node().Name
nodeName := req.Node().Name
// skip shared-session optimisation: targets may connect to different replicas.
if req.Node().Driver != nil && req.Node().Driver.NeedsNewClientPerBuild() {
continue
}
if _, ok := m[nodeName]; !ok {
m[nodeName] = map[fsKey]*fsTracker{}
}
Expand Down
8 changes: 8 additions & 0 deletions build/resolver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ func (dp ResolvedNode) Platforms() []ocispecs.Platform {
}

func (dp ResolvedNode) Client(ctx context.Context) (*client.Client, error) {
node := dp.resolver.nodes[dp.driverIndex]
// For per-build drivers (e.g. loadbalance=random), bootstrap then return a fresh connection.
if node.Driver != nil && node.Driver.NeedsNewClientPerBuild() {
if _, err := dp.resolver.boot(ctx, []int{dp.driverIndex}, nil); err != nil {
return nil, err
}
return node.Driver.NewClient(ctx)
}
clients, err := dp.resolver.boot(ctx, []int{dp.driverIndex}, nil)
if err != nil {
return nil, err
Expand Down
5 changes: 5 additions & 0 deletions driver/kubernetes/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type Driver struct {
// if you add fields, remember to update docs:
// https://github.com/docker/docs/blob/main/content/build/drivers/kubernetes.md
minReplicas int
loadbalance string
deployment *appsv1.Deployment
statefulSet *appsv1.StatefulSet
configMaps []*corev1.ConfigMap
Expand All @@ -61,6 +62,10 @@ func (d *Driver) IsMobyDriver() bool {
return false
}

func (d *Driver) NeedsNewClientPerBuild() bool {
return d.loadbalance == LoadbalanceRandom
}

func (d *Driver) Config() driver.InitConfig {
return d.InitConfig
}
Expand Down
1 change: 1 addition & 0 deletions driver/kubernetes/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func (f *factory) New(ctx context.Context, cfg driver.InitConfig) (driver.Driver
StatefulSet: d.statefulSet,
}
}
d.loadbalance = loadbalance
return d, nil
}

Expand Down
38 changes: 38 additions & 0 deletions driver/kubernetes/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -268,3 +268,41 @@ func TestFactory_processDriverOpts(t *testing.T) {
},
)
}

func TestNeedsNewClientPerBuild(t *testing.T) {
f := factory{
cc: &mockClientConfig{
clientConfig: &rest.Config{},
},
}
baseCfg := driver.InitConfig{
Name: driver.BuilderName("test"),
}

t.Run("RandomLoadbalance", func(t *testing.T) {
cfg := baseCfg
cfg.DriverOpts = map[string]string{"loadbalance": "random"}
d, err := f.New(t.Context(), cfg)
require.NoError(t, err)
require.True(t, d.(*Driver).NeedsNewClientPerBuild(),
"expected NeedsNewClientPerBuild=true for loadbalance=random")
})

t.Run("StickyLoadbalance", func(t *testing.T) {
cfg := baseCfg
cfg.DriverOpts = map[string]string{"loadbalance": "sticky"}
d, err := f.New(t.Context(), cfg)
require.NoError(t, err)
require.False(t, d.(*Driver).NeedsNewClientPerBuild(),
"expected NeedsNewClientPerBuild=false for loadbalance=sticky")
})

t.Run("DefaultLoadbalance", func(t *testing.T) {
cfg := baseCfg
cfg.DriverOpts = map[string]string{}
d, err := f.New(t.Context(), cfg)
require.NoError(t, err)
require.False(t, d.(*Driver).NeedsNewClientPerBuild(),
"expected NeedsNewClientPerBuild=false for default (sticky) loadbalance")
})
}
14 changes: 14 additions & 0 deletions driver/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,20 @@ func (d *DriverHandle) Client(ctx context.Context, opt ...client.ClientOpt) (*cl
return d.client, d.err
}

func (d *DriverHandle) NewClient(ctx context.Context) (*client.Client, error) {
return d.Driver.Client(ctx, d.getClientOptions()...)
}

func (d *DriverHandle) NeedsNewClientPerBuild() bool {
type perBuildClientDriver interface {
NeedsNewClientPerBuild() bool
}
if p, ok := d.Driver.(perBuildClientDriver); ok {
return p.NeedsNewClientPerBuild()
}
return false
}
Comment on lines +131 to +143

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.

Actually after new review this doesn't feel like the right shape for the driver API. NeedsNewClientPerBuild() is added as an optional hidden interface inside DriverHandle, but the behavior is broader than the name says. The build code also uses it to disable shared local mount sessions.

Can we make that contract explicit near the Driver interface instead of hiding it in manager.go? Something like an optional RequiresUncachedClient() interface would make the build-side behavior easier to reason about. I would also rename NewClient() to something like UncachedClient() so it is clear this is deliberately bypassing DriverHandle.Client() caching, not just another constructor.


func (d *DriverHandle) getClientOptions() []client.ClientOpt {
return []client.ClientOpt{
client.WithTracerDelegate(delegated.DefaultExporter),
Expand Down
Loading