From 6bd2eb50468fa2e96c7be8895f1e72434ff6328d Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Sat, 18 Sep 2021 21:04:16 -0700 Subject: [PATCH 1/3] resolver: use different mutext for handlers and hosts hosts mutex is called on initialization, meaning `GetResolver` might block if it is in the middle of auth exchange. This is currently bad in the case where Job initialization needs to register a name before timeout is reached. Signed-off-by: Tonis Tiigi --- util/resolver/authorizer.go | 19 ++++++++++--------- util/resolver/pool.go | 12 ++++++------ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/util/resolver/authorizer.go b/util/resolver/authorizer.go index 3b029b245d69..2766f2157e07 100644 --- a/util/resolver/authorizer.go +++ b/util/resolver/authorizer.go @@ -25,11 +25,12 @@ import ( type authHandlerNS struct { counter int64 // needs to be 64bit aligned for 32bit systems - mu sync.Mutex - handlers map[string]*authHandler - hosts map[string][]docker.RegistryHost - sm *session.Manager - g flightcontrol.Group + handlers map[string]*authHandler + muHandlers sync.Mutex + hosts map[string][]docker.RegistryHost + muHosts sync.Mutex + sm *session.Manager + g flightcontrol.Group } func newAuthHandlerNS(sm *session.Manager) *authHandlerNS { @@ -118,8 +119,8 @@ func newDockerAuthorizer(client *http.Client, handlers *authHandlerNS, sm *sessi // Authorize handles auth request. func (a *dockerAuthorizer) Authorize(ctx context.Context, req *http.Request) error { - a.handlers.mu.Lock() - defer a.handlers.mu.Unlock() + a.handlers.muHandlers.Lock() + defer a.handlers.muHandlers.Unlock() // skip if there is no auth handler ah := a.handlers.get(ctx, req.URL.Host, a.sm, a.session) @@ -141,8 +142,8 @@ func (a *dockerAuthorizer) getCredentials(host string) (sessionID, username, sec } func (a *dockerAuthorizer) AddResponses(ctx context.Context, responses []*http.Response) error { - a.handlers.mu.Lock() - defer a.handlers.mu.Unlock() + a.handlers.muHandlers.Lock() + defer a.handlers.muHandlers.Unlock() last := responses[len(responses)-1] host := last.Request.URL.Host diff --git a/util/resolver/pool.go b/util/resolver/pool.go index 5706ddb7f37e..ad9725d064a8 100644 --- a/util/resolver/pool.go +++ b/util/resolver/pool.go @@ -40,7 +40,7 @@ func (p *Pool) gc() { defer p.mu.Unlock() for k, ns := range p.m { - ns.mu.Lock() + ns.muHandlers.Lock() for key, h := range ns.handlers { if time.Since(h.lastUsed) < 10*time.Minute { continue @@ -58,7 +58,7 @@ func (p *Pool) gc() { if len(ns.handlers) == 0 { delete(p.m, k) } - ns.mu.Unlock() + ns.muHandlers.Unlock() } time.AfterFunc(5*time.Minute, p.gc) @@ -128,9 +128,9 @@ func (r *Resolver) HostsFunc(host string) ([]docker.RegistryHost, error) { return func(domain string) ([]docker.RegistryHost, error) { v, err := r.handler.g.Do(context.TODO(), domain, func(ctx context.Context) (interface{}, error) { // long lock not needed because flightcontrol.Do - r.handler.mu.Lock() + r.handler.muHosts.Lock() v, ok := r.handler.hosts[domain] - r.handler.mu.Unlock() + r.handler.muHosts.Unlock() if ok { return v, nil } @@ -138,9 +138,9 @@ func (r *Resolver) HostsFunc(host string) ([]docker.RegistryHost, error) { if err != nil { return nil, err } - r.handler.mu.Lock() + r.handler.muHosts.Lock() r.handler.hosts[domain] = res - r.handler.mu.Unlock() + r.handler.muHosts.Unlock() return res, nil }) if err != nil || v == nil { From f62bb55245bc15b6fb79a4cf0cbe6c5792909222 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Sat, 18 Sep 2021 21:22:32 -0700 Subject: [PATCH 2/3] resolver: make sure authorizer is not overwritten on other resolvers Authorizer stores the current session.Group so if it is overwritten for another resolver it means that session might have been dropped and authentication will fail. Signed-off-by: Tonis Tiigi --- util/resolver/pool.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/util/resolver/pool.go b/util/resolver/pool.go index ad9725d064a8..242cb17ea7cd 100644 --- a/util/resolver/pool.go +++ b/util/resolver/pool.go @@ -146,10 +146,13 @@ func (r *Resolver) HostsFunc(host string) ([]docker.RegistryHost, error) { if err != nil || v == nil { return nil, err } - res := v.([]docker.RegistryHost) - if len(res) == 0 { + vv := v.([]docker.RegistryHost) + if len(vv) == 0 { return nil, nil } + // make a copy so authorizer is set on unique instance + res := make([]docker.RegistryHost, len(vv)) + copy(res, vv) auth := newDockerAuthorizer(res[0].Client, r.handler, r.sm, r.g) for i := range res { res[i].Authorizer = auth From 161b581e71c0ced8a8b8403e33614e6c00d1b696 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Sat, 18 Sep 2021 21:28:45 -0700 Subject: [PATCH 3/3] solver: increase timeout for job registration Signed-off-by: Tonis Tiigi --- control/control.go | 2 ++ solver/jobs.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/control/control.go b/control/control.go index 1e70843ab509..1eff83433abf 100644 --- a/control/control.go +++ b/control/control.go @@ -238,6 +238,8 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* atomic.AddInt64(&c.buildCount, 1) defer atomic.AddInt64(&c.buildCount, -1) + // This method registers job ID in solver.Solve. Make sure there are no blocking calls before that might delay this. + if err := translateLegacySolveRequest(req); err != nil { return nil, err } diff --git a/solver/jobs.go b/solver/jobs.go index 0533366b2248..82b29527ff00 100644 --- a/solver/jobs.go +++ b/solver/jobs.go @@ -455,7 +455,7 @@ func (jl *Solver) NewJob(id string) (*Job, error) { } func (jl *Solver) Get(id string) (*Job, error) { - ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 6*time.Second) defer cancel() go func() {