Skip to content

Commit 1af1ff7

Browse files
thediveothediveo
committed
fixes the botched fix for Docker client race; introduces Preflighter engine interface; enables -race checks in tests
1 parent 82cf83f commit 1af1ff7

6 files changed

Lines changed: 44 additions & 21 deletions

File tree

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,4 @@ report: ## run goreportcard on this module
1414
@scripts/goreportcard.sh
1515

1616
test: ## run unit tests
17-
go test -v -p 1 -exec sudo ./... && go test -v -p 1 ./...
17+
go test -v -p=1 -race -exec sudo ./... && go test -v -p=1 -race ./...

‎engineclient/engineclient.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,19 @@ type EngineClient interface {
5656
Close()
5757
}
5858

59+
// Allow an engine client to do some final pre-flight operations right
60+
// before starting a watch, where the preflight ops might require
61+
// talking to a particular engine and thus should be controlled by a
62+
// context.
63+
//
64+
// The raison d'être at this time is to avoid a race condition in the Docker
65+
// client API version negotiation that otherwise trips our race-enabled
66+
// tests and thus makes us blind for race violations in our own code base.
67+
type Preflighter interface {
68+
// Run "preflight" tasks just right before starting a Watch.
69+
Preflight(ctx context.Context)
70+
}
71+
5972
// RucksackPacker optionally adds additional information to the tracked
6073
// container information, as kind of a Rucksack. It gets passed container
6174
// engine-specific inspection information so as to be able to pick and pack

‎engineclient/moby/moby.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,9 @@ type MobyWatcher struct {
5656
packer engineclient.RucksackPacker // optional Rucksack packer for app-specific container information.
5757
}
5858

59-
// Make sure that the EngineClient interface is fully implemented.
59+
// Make sure that the EngineClient and Preflighter interfaces are fully implemented.
6060
var _ (engineclient.EngineClient) = (*MobyWatcher)(nil)
61+
var _ (engineclient.Preflighter) = (*MobyWatcher)(nil)
6162

6263
// NewMobyWatcher returns a new MobyWatcher using the specified Docker engine
6364
// client; typically, you would want to use this lower-level constructor only in
@@ -129,12 +130,17 @@ func (mw *MobyWatcher) Close() {
129130
mw.moby.Close()
130131
}
131132

132-
// List all the currently alive and kicking containers, but do not list any
133-
// containers without any processes.
134-
func (mw *MobyWatcher) List(ctx context.Context) ([]*whalewatcher.Container, error) {
133+
// Allow an engine client to do some final pre-flight operations that might
134+
// require talking to a particular engine and thus should be controlled by a
135+
// context.
136+
func (mw *MobyWatcher) Preflight(ctx context.Context) {
135137
// https://github.com/moby/moby/pull/42379
136138
mw.moby.NegotiateAPIVersion(ctx)
139+
}
137140

141+
// List all the currently alive and kicking containers, but do not list any
142+
// containers without any processes.
143+
func (mw *MobyWatcher) List(ctx context.Context) ([]*whalewatcher.Container, error) {
138144
// Scan the currently available containers and take only the alive into
139145
// further consideration. This is a potentially lengthy operation, as we
140146
// need to inspect each potential candidate individually due to the way the
@@ -192,9 +198,6 @@ func (mw *MobyWatcher) Inspect(ctx context.Context, nameorid string) (*whalewatc
192198
// in the lifecycle of containers getting born (=alive, as opposed to, say,
193199
// "conceived") and die.
194200
func (mw *MobyWatcher) LifecycleEvents(ctx context.Context) (<-chan engineclient.ContainerEvent, <-chan error) {
195-
// https://github.com/moby/moby/pull/42379
196-
mw.moby.NegotiateAPIVersion(ctx)
197-
198201
cntreventstream := make(chan engineclient.ContainerEvent)
199202
cntrerrstream := make(chan error, 1)
200203

‎go.sum‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,6 @@ github.com/containerd/containerd v1.5.0-beta.4/go.mod h1:GmdgZd2zA2GYIBZ0w09Zvgq
157157
github.com/containerd/containerd v1.5.0-rc.0/go.mod h1:V/IXoMqNGgBlabz3tHD2TWDoTJseu1FGOKuoA4nNb2s=
158158
github.com/containerd/containerd v1.5.1/go.mod h1:0DOxVqwDy2iZvrZp2JUx/E+hS0UNTVn7dJnIOwtYR4g=
159159
github.com/containerd/containerd v1.5.7/go.mod h1:gyvv6+ugqY25TiXxcZC3L5yOeYgEw0QMhscqVp1AR9c=
160-
github.com/containerd/containerd v1.6.4 h1:SEDZBp10mhCp+hkO3Njz/YhGrI7ah3edNcUlRdUPOgg=
161-
github.com/containerd/containerd v1.6.4/go.mod h1:oWOqbuJUZmOVafhA0lj2NAXbiO1u7F0K5l1bUgdyo94=
162160
github.com/containerd/containerd v1.6.6 h1:xJNPhbrmz8xAMDNoVjHy9YHtWwEQNS+CDkcIRh7t8Y0=
163161
github.com/containerd/containerd v1.6.6/go.mod h1:ZoP1geJldzCVY3Tonoz7b1IXk8rIX0Nltt5QE4OMNk0=
164162
github.com/containerd/continuity v0.0.0-20190426062206-aaeac12a7ffc/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
@@ -261,8 +259,6 @@ github.com/docker/distribution v2.7.1+incompatible/go.mod h1:J2gT2udsDAN96Uj4Kfc
261259
github.com/docker/distribution v2.8.1+incompatible h1:Q50tZOPR6T/hjNsyc9g8/syEs6bk8XXApsHjKukMl68=
262260
github.com/docker/distribution v2.8.1+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
263261
github.com/docker/docker v1.4.2-0.20190924003213-a8608b5b67c7/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
264-
github.com/docker/docker v20.10.16+incompatible h1:2Db6ZR/+FUR3hqPMwnogOPHFn405crbpxvWzKovETOQ=
265-
github.com/docker/docker v20.10.16+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
266262
github.com/docker/docker v20.10.17+incompatible h1:JYCuMrWaVNophQTOrMMoSwudOVEfcegoZZrleKc1xwE=
267263
github.com/docker/docker v20.10.17+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
268264
github.com/docker/docker-credential-helpers v0.6.3/go.mod h1:WRaJzqw3CTB9bk10avuGsjVBZsD05qeibJ1/TYlvc0Y=
@@ -577,8 +573,6 @@ github.com/opencontainers/selinux v1.8.2/go.mod h1:MUIHuUEvKB1wtJjQdOyYRgOnLD2xA
577573
github.com/opencontainers/selinux v1.10.0/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI=
578574
github.com/opencontainers/selinux v1.10.1 h1:09LIPVRP3uuZGQvgR+SgMSNBd1Eb3vlRbGqQpoHsF8w=
579575
github.com/opencontainers/selinux v1.10.1/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI=
580-
github.com/ory/dockertest/v3 v3.9.0 h1:U7M9FfYEwF4uqEE6WUSFs7K+Hvb31CsCX5uZUZD3olI=
581-
github.com/ory/dockertest/v3 v3.9.0/go.mod h1:jgm0rnguArPXsVduy+oUjzFtD0Na+DDNbUl8W5v+ez8=
582576
github.com/ory/dockertest/v3 v3.9.1 h1:v4dkG+dlu76goxMiTT2j8zV7s4oPPEppKT8K8p2f1kY=
583577
github.com/ory/dockertest/v3 v3.9.1/go.mod h1:42Ir9hmvaAPm0Mgibk6mBPi7SFvTXxEcnztDYOJ//uM=
584578
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=

‎watcher/moby/moby_test.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package moby
1717
import (
1818
"context"
1919
"sync"
20+
"time"
2021

2122
"github.com/docker/docker/api/types"
2223
"github.com/docker/docker/client"
@@ -47,23 +48,32 @@ var _ = Describe("Moby watcher engine end-to-end test", func() {
4748
It("gets and uses the underlying Docker client", func() {
4849
mw, err := New("unix:///var/run/docker.sock", nil, moby.WithPID(123456))
4950
Expect(err).NotTo(HaveOccurred())
51+
5052
Expect(mw.PID()).To(Equal(123456))
5153
defer mw.Close()
5254

5355
ctx, cancel := context.WithCancel(context.Background())
5456
defer cancel()
5557
done := make(chan struct{})
58+
// While // https://github.com/moby/moby/pull/42379 is pending we need
59+
// to run any API additional API calls from the same goroutine as where
60+
// we start the Watch in order to not trigger the race detector.
61+
nchan := make(chan []types.NetworkResource, 1)
5662
go func() {
63+
defer GinkgoRecover()
64+
dc, ok := mw.Client().(client.APIClient)
65+
Expect(ok).To(BeTrue())
66+
Expect(dc).NotTo(BeNil())
67+
networks, err := dc.NetworkList(ctx, types.NetworkListOptions{})
68+
Expect(err).NotTo(HaveOccurred())
69+
nchan <- networks
70+
mw.Client().(client.CommonAPIClient).NegotiateAPIVersion(ctx)
5771
_ = mw.Watch(ctx)
5872
close(done)
5973
}()
60-
Consistently(done, "1s").ShouldNot(BeClosed())
61-
62-
dc, ok := mw.Client().(client.APIClient)
63-
Expect(ok).To(BeTrue())
64-
Expect(dc).NotTo(BeNil())
65-
networks, err := dc.NetworkList(ctx, types.NetworkListOptions{})
66-
Expect(err).NotTo(HaveOccurred())
74+
Consistently(done).WithTimeout(5 * time.Second).WithPolling(250 * time.Millisecond).
75+
ShouldNot(BeClosed())
76+
networks := <-nchan
6777
Expect(networks).To(ContainElement(And(
6878
HaveField("Name", Equal("bridge")),
6979
HaveField("Driver", Equal("bridge")),

‎watcher/watcher.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,9 @@ func (ww *watcher) Close() {
194194
// container engine, subject to the backoff (and thus optional throttling or
195195
// rate-limiting) specified when this watch was created.
196196
func (ww *watcher) Watch(ctx context.Context) error {
197+
if pf, ok := ww.engine.(engineclient.Preflighter); ok {
198+
pf.Preflight(ctx)
199+
}
197200
return backoff.Retry(func() error {
198201
// In case we have an existing and non-empty portfolio, keep that
199202
// visible to our users while we try to synchronize. If not, then simply

0 commit comments

Comments
 (0)