Skip to content

driver: check history capability #1846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crazy-max merged 3 commits into from
May 30, 2023
Merged

driver: check history capability #1846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b723ec
driver: check history capability
crazy-max May 26, 2023
d196ac3
driver: cache features
crazy-max May 26, 2023
8c65e4f
driver: add context to Features interface
crazy-max May 26, 2023
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
8 changes: 4 additions & 4 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op
}

for _, e := range opt.CacheTo {
if e.Type != "inline" && !nodeDriver.Features()[driver.CacheExport] {
if e.Type != "inline" && !nodeDriver.Features(ctx)[driver.CacheExport] {
return nil, nil, notSupported(nodeDriver, driver.CacheExport)
}
}
Expand Down Expand Up @@ -527,7 +527,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op

// set up exporters
for i, e := range opt.Exports {
if e.Type == "oci" && !nodeDriver.Features()[driver.OCIExporter] {
if e.Type == "oci" && !nodeDriver.Features(ctx)[driver.OCIExporter] {
return nil, nil, notSupported(nodeDriver, driver.OCIExporter)
}
if e.Type == "docker" {
Expand All @@ -545,7 +545,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op
defers = append(defers, cancel)
opt.Exports[i].Output = wrapWriteCloser(w)
}
} else if !nodeDriver.Features()[driver.DockerExporter] {
} else if !nodeDriver.Features(ctx)[driver.DockerExporter] {
return nil, nil, notSupported(nodeDriver, driver.DockerExporter)
}
}
Expand Down Expand Up @@ -614,7 +614,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op
for i, p := range opt.Platforms {
pp[i] = platforms.Format(p)
}
if len(pp) > 1 && !nodeDriver.Features()[driver.MultiPlatform] {
if len(pp) > 1 && !nodeDriver.Features(ctx)[driver.MultiPlatform] {
return nil, nil, notSupported(nodeDriver, driver.MultiPlatform)
}
so.FrontendAttrs["platform"] = strings.Join(pp, ",")
Expand Down
14 changes: 10 additions & 4 deletions driver/docker-container/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,13 +387,19 @@ func (d *Driver) Factory() driver.Factory {
return d.factory
}

func (d *Driver) Features() map[driver.Feature]bool {
func (d *Driver) Features(ctx context.Context) map[driver.Feature]bool {
var historyAPI bool
c, err := d.Client(ctx)
Copy link
Member

@tonistiigi tonistiigi May 30, 2023

Choose a reason for hiding this comment

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

This call is expensive. It makes an new docker exec.

Copy link
Member Author

@crazy-max crazy-max Jun 5, 2023

Choose a reason for hiding this comment

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

The client is cached

buildx/driver/manager.go

Lines 157 to 162 in a906149

func (d *cachedDriver) Client(ctx context.Context) (*client.Client, error) {
d.once.Do(func() {
d.client, d.err = d.Driver.Client(ctx)
})
return d.client, d.err
}
so we are probably fine here no?

Copy link
Member

@tonistiigi tonistiigi Jun 5, 2023

Choose a reason for hiding this comment

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

That is not the struct that's get called in the line 392.

if err == nil {
historyAPI = driver.HistoryAPISupported(ctx, c)
c.Close()
}
return map[driver.Feature]bool{
driver.OCIExporter: true,
driver.DockerExporter: true,

driver.CacheExport: true,
driver.MultiPlatform: true,
driver.CacheExport: true,
driver.MultiPlatform: true,
driver.HistoryAPI: historyAPI,
}
}

Expand Down
6 changes: 4 additions & 2 deletions driver/docker/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
}))
}

func (d *Driver) Features() map[driver.Feature]bool {
func (d *Driver) Features(ctx context.Context) map[driver.Feature]bool {
var useContainerdSnapshotter bool
ctx := context.Background()
var historyAPI bool
c, err := d.Client(ctx)
if err == nil {
workers, _ := c.ListWorkers(ctx)
Expand All @@ -69,13 +69,15 @@ func (d *Driver) Features() map[driver.Feature]bool {
useContainerdSnapshotter = true
}
}
historyAPI = driver.HistoryAPISupported(ctx, c)
c.Close()
}
return map[driver.Feature]bool{
driver.OCIExporter: useContainerdSnapshotter,
driver.DockerExporter: useContainerdSnapshotter,
driver.CacheExport: useContainerdSnapshotter,
driver.MultiPlatform: useContainerdSnapshotter,
driver.HistoryAPI: historyAPI,
}
}

Expand Down
28 changes: 27 additions & 1 deletion driver/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ import (
"github.com/docker/buildx/store"
"github.com/docker/buildx/util/progress"
clitypes "github.com/docker/cli/cli/config/types"
controlapi "github.com/moby/buildkit/api/services/control"
"github.com/moby/buildkit/client"
"github.com/moby/buildkit/util/grpcerrors"
"github.com/pkg/errors"
"google.golang.org/grpc/codes"
)

var ErrNotRunning = errors.Errorf("driver not running")
Expand Down Expand Up @@ -57,7 +60,7 @@ type Driver interface {
Stop(ctx context.Context, force bool) error
Rm(ctx context.Context, force, rmVolume, rmDaemon bool) error
Client(ctx context.Context) (*client.Client, error)
Features() map[Feature]bool
Features(ctx context.Context) map[Feature]bool
IsMobyDriver() bool
Config() InitConfig
}
Expand Down Expand Up @@ -89,3 +92,26 @@ func Boot(ctx, clientContext context.Context, d Driver, pw progress.Writer) (*cl
return c, nil
}
}

func HistoryAPISupported(ctx context.Context, c *client.Client) (res bool) {
res = true
checkErrF := func(err error) {
if s, ok := grpcerrors.AsGRPCStatus(err); ok {
if s.Code() == codes.Unimplemented {
Copy link
Member

@tonistiigi tonistiigi May 30, 2023

Choose a reason for hiding this comment

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

What error except for EOF in here means there is support?

res = false
}
}
}
cl, err := c.ControlClient().ListenBuildHistory(ctx, &controlapi.BuildHistoryRequest{
ActiveOnly: true,
Ref: "buildx-dummy-ref", // dummy ref to check if the server supports the API
Copy link
Member

@tonistiigi tonistiigi May 30, 2023

Choose a reason for hiding this comment

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

"buildx-test-history-api-feature" so that if you look at it from server side you understand where the request is coming from.

EarlyExit: true,
})
if err != nil {
checkErrF(err)
return
}
_, err = cl.Recv()
Copy link
Member

@tonistiigi tonistiigi May 30, 2023

Choose a reason for hiding this comment

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

Client should be read until it is empty. It can be closed/canceled if there is an issue.

checkErrF(err)
return
}
2 changes: 2 additions & 0 deletions driver/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ const DockerExporter Feature = "Docker exporter"

const CacheExport Feature = "cache export"
const MultiPlatform Feature = "multiple platforms"

const HistoryAPI Feature = "history api"
14 changes: 10 additions & 4 deletions driver/kubernetes/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,12 +228,18 @@ func (d *Driver) Factory() driver.Factory {
return d.factory
}

func (d *Driver) Features() map[driver.Feature]bool {
func (d *Driver) Features(ctx context.Context) map[driver.Feature]bool {
var historyAPI bool
c, err := d.Client(ctx)
if err == nil {
historyAPI = driver.HistoryAPISupported(ctx, c)
c.Close()
}
return map[driver.Feature]bool{
driver.OCIExporter: true,
driver.DockerExporter: d.DockerAPI != nil,

driver.CacheExport: true,
driver.MultiPlatform: true, // Untested (needs multiple Driver instances)
driver.CacheExport: true,
driver.MultiPlatform: true, // Untested (needs multiple Driver instances)
Copy link
Collaborator

@jedevc jedevc May 26, 2023

Choose a reason for hiding this comment

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

Unrelated, but I think this comment is out-of-date now?

I think this is tested now, at least we have docs for this scenario?

Copy link
Member Author

@crazy-max crazy-max May 26, 2023

Choose a reason for hiding this comment

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

@AkihiroSuda @morlay

driver.HistoryAPI: historyAPI,
}
}
15 changes: 12 additions & 3 deletions driver/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,11 @@ func GetFactories(instanceRequired bool) []Factory {

type cachedDriver struct {
Driver
client *client.Client
err error
once sync.Once
client *client.Client
err error
once sync.Once
featuresOnce sync.Once
features map[Feature]bool
}

func (d *cachedDriver) Client(ctx context.Context) (*client.Client, error) {
Expand All @@ -158,3 +160,10 @@ func (d *cachedDriver) Client(ctx context.Context) (*client.Client, error) {
})
return d.client, d.err
}

func (d *cachedDriver) Features(ctx context.Context) map[Feature]bool {
d.featuresOnce.Do(func() {
d.features = d.Driver.Features(ctx)
})
return d.features
}
9 changes: 8 additions & 1 deletion driver/remote/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,19 @@ func (d *Driver) Client(ctx context.Context) (*client.Client, error) {
return client.New(ctx, d.InitConfig.EndpointAddr, opts...)
}

func (d *Driver) Features() map[driver.Feature]bool {
func (d *Driver) Features(ctx context.Context) map[driver.Feature]bool {
var historyAPI bool
c, err := d.Client(ctx)
if err == nil {
historyAPI = driver.HistoryAPISupported(ctx, c)
c.Close()
}
return map[driver.Feature]bool{
driver.OCIExporter: true,
driver.DockerExporter: true,
driver.CacheExport: true,
driver.MultiPlatform: true,
driver.HistoryAPI: historyAPI,
}
}

Expand Down