Browse code

volumes: only send "create" event when actually creating volume

The VolumesService did not have information wether or not a volume
was _created_ or if a volume already existed in the driver, and
the existing volume was used.

As a result, multiple "create" events could be generated for the
same volume. For example:

1. Run `docker events` in a shell to start listening for events
2. Create a volume:

docker volume create myvolume

3. Start a container that uses that volume:

docker run -dit -v myvolume:/foo busybox

4. Check the events that were generated:

2021-02-15T18:49:55.874621004+01:00 volume create myvolume (driver=local)
2021-02-15T18:50:11.442759052+01:00 volume create myvolume (driver=local)
2021-02-15T18:50:11.487104176+01:00 container create 45112157c8b1382626bf5e01ef18445a4c680f3846c5e32d01775dddee8ca6d1 (image=busybox, name=gracious_hypatia)
2021-02-15T18:50:11.519288102+01:00 network connect a19f6bb8d44ff84d478670fa4e34c5bf5305f42786294d3d90e790ac74b6d3e0 (container=45112157c8b1382626bf5e01ef18445a4c680f3846c5e32d01775dddee8ca6d1, name=bridge, type=bridge)
2021-02-15T18:50:11.526407799+01:00 volume mount myvolume (container=45112157c8b1382626bf5e01ef18445a4c680f3846c5e32d01775dddee8ca6d1, destination=/foo, driver=local, propagation=, read/write=true)
2021-02-15T18:50:11.864134043+01:00 container start 45112157c8b1382626bf5e01ef18445a4c680f3846c5e32d01775dddee8ca6d1 (image=busybox, name=gracious_hypatia)

5. Notice that a "volume create" event is created twice;

- once when `docker volume create` was ran
- once when `docker run ...` was ran

This patch moves the generation of (most) events to the volume _store_, and only
generates an event if the volume did not yet exist.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2021/02/16 02:45:15
Showing 4 changed files
... ...
@@ -182,15 +182,14 @@ func (s *DockerSuite) TestVolumeEvents(c *testing.T) {
182 182
 	until := daemonUnixTime(c)
183 183
 	out, _ := dockerCmd(c, "events", "--since", since, "--until", until)
184 184
 	events := strings.Split(strings.TrimSpace(out), "\n")
185
-	assert.Assert(c, len(events) > 4)
185
+	assert.Assert(c, len(events) > 3)
186 186
 
187 187
 	volumeEvents := eventActionsByIDAndType(c, events, "test-event-volume-local", "volume")
188
-	assert.Equal(c, len(volumeEvents), 5)
188
+	assert.Equal(c, len(volumeEvents), 4)
189 189
 	assert.Equal(c, volumeEvents[0], "create")
190
-	assert.Equal(c, volumeEvents[1], "create")
191
-	assert.Equal(c, volumeEvents[2], "mount")
192
-	assert.Equal(c, volumeEvents[3], "unmount")
193
-	assert.Equal(c, volumeEvents[4], "destroy")
190
+	assert.Equal(c, volumeEvents[1], "mount")
191
+	assert.Equal(c, volumeEvents[2], "unmount")
192
+	assert.Equal(c, volumeEvents[3], "destroy")
194 193
 }
195 194
 
196 195
 func (s *DockerSuite) TestNetworkEvents(c *testing.T) {
... ...
@@ -3,6 +3,7 @@ package system // import "github.com/docker/docker/integration/system"
3 3
 import (
4 4
 	"context"
5 5
 	"encoding/json"
6
+	"errors"
6 7
 	"io"
7 8
 	"net/http"
8 9
 	"net/url"
... ...
@@ -11,9 +12,12 @@ import (
11 11
 	"time"
12 12
 
13 13
 	"github.com/docker/docker/api/types"
14
+	"github.com/docker/docker/api/types/events"
14 15
 	"github.com/docker/docker/api/types/filters"
16
+	"github.com/docker/docker/api/types/mount"
15 17
 	"github.com/docker/docker/api/types/strslice"
16 18
 	"github.com/docker/docker/api/types/versions"
19
+	"github.com/docker/docker/api/types/volume"
17 20
 	"github.com/docker/docker/integration/internal/container"
18 21
 	"github.com/docker/docker/pkg/jsonmessage"
19 22
 	"github.com/docker/docker/testutil/request"
... ...
@@ -122,3 +126,69 @@ func TestEventsBackwardsCompatible(t *testing.T) {
122 122
 	assert.Check(t, is.Equal(cID, containerCreateEvent.ID))
123 123
 	assert.Check(t, is.Equal("busybox", containerCreateEvent.From))
124 124
 }
125
+
126
+// TestEventsVolumeCreate verifies that volume create events are only fired
127
+// once: when creating the volume, and not when attaching to a container.
128
+func TestEventsVolumeCreate(t *testing.T) {
129
+	skip.If(t, testEnv.OSType == "windows", "FIXME: Windows doesn't trigger the events? Could be a race")
130
+
131
+	defer setupTest(t)()
132
+	ctx, cancel := context.WithCancel(context.Background())
133
+	defer cancel()
134
+
135
+	client := testEnv.APIClient()
136
+
137
+	since := request.DaemonUnixTime(ctx, t, client, testEnv)
138
+	volName := t.Name()
139
+	getEvents := func(messages <-chan events.Message, errs <-chan error) ([]events.Message, error) {
140
+		var evts []events.Message
141
+
142
+		for {
143
+			select {
144
+			case m := <-messages:
145
+				evts = append(evts, m)
146
+			case err := <-errs:
147
+				if err == io.EOF {
148
+					return evts, nil
149
+				}
150
+				return nil, err
151
+			case <-time.After(time.Second * 3):
152
+				return nil, errors.New("timeout hit")
153
+			}
154
+		}
155
+	}
156
+
157
+	_, err := client.VolumeCreate(ctx, volume.VolumeCreateBody{Name: volName})
158
+	assert.NilError(t, err)
159
+
160
+	filter := filters.NewArgs(
161
+		filters.Arg("type", "volume"),
162
+		filters.Arg("event", "create"),
163
+		filters.Arg("volume", volName),
164
+	)
165
+	messages, errs := client.Events(ctx, types.EventsOptions{
166
+		Since:   since,
167
+		Until:   request.DaemonUnixTime(ctx, t, client, testEnv),
168
+		Filters: filter,
169
+	})
170
+
171
+	volEvents, err := getEvents(messages, errs)
172
+	assert.NilError(t, err)
173
+	assert.Equal(t, len(volEvents), 1, "expected volume create event when creating a volume")
174
+
175
+	container.Create(ctx, t, client, container.WithMount(mount.Mount{
176
+		Type:   mount.TypeVolume,
177
+		Source: volName,
178
+		Target: "/tmp/foo",
179
+	}))
180
+
181
+	messages, errs = client.Events(ctx, types.EventsOptions{
182
+		Since:   since,
183
+		Until:   request.DaemonUnixTime(ctx, t, client, testEnv),
184
+		Filters: filter,
185
+	})
186
+
187
+	volEvents, err = getEvents(messages, errs)
188
+	assert.NilError(t, err)
189
+	assert.Equal(t, len(volEvents), 1, "expected volume create event to be fired only once")
190
+}
... ...
@@ -23,7 +23,9 @@ type ds interface {
23 23
 	GetDriverList() []string
24 24
 }
25 25
 
26
-type volumeEventLogger interface {
26
+// VolumeEventLogger interface provides methods to log volume-related events
27
+type VolumeEventLogger interface {
28
+	// LogVolumeEvent generates an event related to a volume.
27 29
 	LogVolumeEvent(volumeID, action string, attributes map[string]string)
28 30
 }
29 31
 
... ...
@@ -33,17 +35,17 @@ type VolumesService struct {
33 33
 	vs           *VolumeStore
34 34
 	ds           ds
35 35
 	pruneRunning int32
36
-	eventLogger  volumeEventLogger
36
+	eventLogger  VolumeEventLogger
37 37
 }
38 38
 
39 39
 // NewVolumeService creates a new volume service
40
-func NewVolumeService(root string, pg plugingetter.PluginGetter, rootIDs idtools.Identity, logger volumeEventLogger) (*VolumesService, error) {
40
+func NewVolumeService(root string, pg plugingetter.PluginGetter, rootIDs idtools.Identity, logger VolumeEventLogger) (*VolumesService, error) {
41 41
 	ds := drivers.NewStore(pg)
42 42
 	if err := setupDefaultDriver(ds, root, rootIDs); err != nil {
43 43
 		return nil, err
44 44
 	}
45 45
 
46
-	vs, err := NewStore(root, ds)
46
+	vs, err := NewStore(root, ds, WithEventLogger(logger))
47 47
 	if err != nil {
48 48
 		return nil, err
49 49
 	}
... ...
@@ -71,7 +73,6 @@ func (s *VolumesService) Create(ctx context.Context, name, driverName string, op
71 71
 		return nil, err
72 72
 	}
73 73
 
74
-	s.eventLogger.LogVolumeEvent(v.Name(), "create", map[string]string{"driver": v.DriverName()})
75 74
 	apiV := volumeToAPIType(v)
76 75
 	return &apiV, nil
77 76
 }
... ...
@@ -161,10 +162,6 @@ func (s *VolumesService) Remove(ctx context.Context, name string, rmOpts ...opts
161 161
 	} else if IsNotExist(err) && cfg.PurgeOnError {
162 162
 		err = nil
163 163
 	}
164
-
165
-	if err == nil {
166
-		s.eventLogger.LogVolumeEvent(v.Name(), "destroy", map[string]string{"driver": v.DriverName()})
167
-	}
168 164
 	return err
169 165
 }
170 166
 
... ...
@@ -68,8 +68,11 @@ func (v volumeWrapper) CachedPath() string {
68 68
 	return v.Volume.Path()
69 69
 }
70 70
 
71
+// StoreOpt sets options for a VolumeStore
72
+type StoreOpt func(store *VolumeStore) error
73
+
71 74
 // NewStore creates a new volume store at the given path
72
-func NewStore(rootPath string, drivers *drivers.Store) (*VolumeStore, error) {
75
+func NewStore(rootPath string, drivers *drivers.Store, opts ...StoreOpt) (*VolumeStore, error) {
73 76
 	vs := &VolumeStore{
74 77
 		locks:   &locker.Locker{},
75 78
 		names:   make(map[string]volume.Volume),
... ...
@@ -79,6 +82,12 @@ func NewStore(rootPath string, drivers *drivers.Store) (*VolumeStore, error) {
79 79
 		drivers: drivers,
80 80
 	}
81 81
 
82
+	for _, o := range opts {
83
+		if err := o(vs); err != nil {
84
+			return nil, err
85
+		}
86
+	}
87
+
82 88
 	if rootPath != "" {
83 89
 		// initialize metadata store
84 90
 		volPath := filepath.Join(rootPath, volumeDataDir)
... ...
@@ -108,6 +117,14 @@ func NewStore(rootPath string, drivers *drivers.Store) (*VolumeStore, error) {
108 108
 	return vs, nil
109 109
 }
110 110
 
111
+// WithEventLogger configures the VolumeStore with the given VolumeEventLogger
112
+func WithEventLogger(logger VolumeEventLogger) StoreOpt {
113
+	return func(store *VolumeStore) error {
114
+		store.eventLogger = logger
115
+		return nil
116
+	}
117
+}
118
+
111 119
 func (s *VolumeStore) getNamed(name string) (volume.Volume, bool) {
112 120
 	s.globalLock.RLock()
113 121
 	v, exists := s.names[name]
... ...
@@ -198,7 +215,9 @@ type VolumeStore struct {
198 198
 	labels map[string]map[string]string
199 199
 	// options stores volume options for each volume
200 200
 	options map[string]map[string]string
201
-	db      *bolt.DB
201
+
202
+	db          *bolt.DB
203
+	eventLogger VolumeEventLogger
202 204
 }
203 205
 
204 206
 func filterByDriver(names []string) filterFunc {
... ...
@@ -464,7 +483,7 @@ func (s *VolumeStore) Create(ctx context.Context, name, driverName string, creat
464 464
 	default:
465 465
 	}
466 466
 
467
-	v, err := s.create(ctx, name, driverName, cfg.Options, cfg.Labels)
467
+	v, created, err := s.create(ctx, name, driverName, cfg.Options, cfg.Labels)
468 468
 	if err != nil {
469 469
 		if _, ok := err.(*OpErr); ok {
470 470
 			return nil, err
... ...
@@ -472,6 +491,9 @@ func (s *VolumeStore) Create(ctx context.Context, name, driverName string, creat
472 472
 		return nil, &OpErr{Err: err, Name: name, Op: "create"}
473 473
 	}
474 474
 
475
+	if created && s.eventLogger != nil {
476
+		s.eventLogger.LogVolumeEvent(v.Name(), "create", map[string]string{"driver": v.DriverName()})
477
+	}
475 478
 	s.setNamed(v, cfg.Reference)
476 479
 	return v, nil
477 480
 }
... ...
@@ -552,7 +574,7 @@ func volumeExists(ctx context.Context, store *drivers.Store, v volume.Volume) (b
552 552
 //  for the given volume name, an error is returned after checking if the reference is stale.
553 553
 // If the reference is stale, it will be purged and this create can continue.
554 554
 // It is expected that callers of this function hold any necessary locks.
555
-func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts, labels map[string]string) (volume.Volume, error) {
555
+func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts, labels map[string]string) (volume.Volume, bool, error) {
556 556
 	// Validate the name in a platform-specific manner
557 557
 
558 558
 	// volume name validation is specific to the host os and not on container image
... ...
@@ -560,19 +582,19 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts,
560 560
 	parser := volumemounts.NewParser(runtime.GOOS)
561 561
 	err := parser.ValidateVolumeName(name)
562 562
 	if err != nil {
563
-		return nil, err
563
+		return nil, false, err
564 564
 	}
565 565
 
566 566
 	v, err := s.checkConflict(ctx, name, driverName)
567 567
 	if err != nil {
568
-		return nil, err
568
+		return nil, false, err
569 569
 	}
570 570
 
571 571
 	if v != nil {
572 572
 		// there is an existing volume, if we already have this stored locally, return it.
573 573
 		// TODO: there could be some inconsistent details such as labels here
574 574
 		if vv, _ := s.getNamed(v.Name()); vv != nil {
575
-			return vv, nil
575
+			return vv, false, nil
576 576
 		}
577 577
 	}
578 578
 
... ...
@@ -580,7 +602,7 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts,
580 580
 	if driverName == "" {
581 581
 		v, _ = s.getVolume(ctx, name, "")
582 582
 		if v != nil {
583
-			return v, nil
583
+			return v, false, nil
584 584
 		}
585 585
 	}
586 586
 
... ...
@@ -589,7 +611,7 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts,
589 589
 	}
590 590
 	vd, err := s.drivers.CreateDriver(driverName)
591 591
 	if err != nil {
592
-		return nil, &OpErr{Op: "create", Name: name, Err: err}
592
+		return nil, false, &OpErr{Op: "create", Name: name, Err: err}
593 593
 	}
594 594
 
595 595
 	logrus.Debugf("Registering new volume reference: driver %q, name %q", vd.Name(), name)
... ...
@@ -599,7 +621,7 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts,
599 599
 			if _, err := s.drivers.ReleaseDriver(driverName); err != nil {
600 600
 				logrus.WithError(err).WithField("driver", driverName).Error("Error releasing reference to volume driver")
601 601
 			}
602
-			return nil, err
602
+			return nil, false, err
603 603
 		}
604 604
 	}
605 605
 
... ...
@@ -617,9 +639,9 @@ func (s *VolumeStore) create(ctx context.Context, name, driverName string, opts,
617 617
 	}
618 618
 
619 619
 	if err := s.setMeta(name, metadata); err != nil {
620
-		return nil, err
620
+		return nil, true, err
621 621
 	}
622
-	return volumeWrapper{v, labels, vd.Scope(), opts}, nil
622
+	return volumeWrapper{v, labels, vd.Scope(), opts}, true, nil
623 623
 }
624 624
 
625 625
 // Get looks if a volume with the given name exists and returns it if so
... ...
@@ -802,6 +824,9 @@ func (s *VolumeStore) Remove(ctx context.Context, v volume.Volume, rmOpts ...opt
802 802
 			err = e
803 803
 		}
804 804
 	}
805
+	if err == nil && s.eventLogger != nil {
806
+		s.eventLogger.LogVolumeEvent(v.Name(), "destroy", map[string]string{"driver": v.DriverName()})
807
+	}
805 808
 	return err
806 809
 }
807 810