Skip to content

Commit 1a0ebd4

Browse files
dpernydperny
authored
Merge pull request moby#2812 from wk8/wk8/revert_for_now
Revert "Actually starting to use the deallocator to clean up services"
2 parents cc6aa78 + 35234b8 commit 1a0ebd4

15 files changed

Lines changed: 124 additions & 369 deletions

File tree

‎cmd/swarmctl/network/inspect.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,6 @@ func printNetworkSummary(network *api.Network) {
5151
common.FprintfIfNotEmpty(w, "ID\t: %s\n", network.ID)
5252
common.FprintfIfNotEmpty(w, "Name\t: %s\n", spec.Annotations.Name)
5353

54-
if network.PendingDelete {
55-
common.FprintfIfNotEmpty(w, "[Network %s marked for removal]\n", spec.Annotations.Name)
56-
}
57-
5854
fmt.Fprintln(w, "Spec:\t")
5955
if len(spec.Annotations.Labels) > 0 {
6056
fmt.Fprintln(w, " Labels:\t")

‎cmd/swarmctl/service/inspect.go‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,17 +22,12 @@ func printServiceSummary(service *api.Service, running int) {
2222
w := tabwriter.NewWriter(os.Stdout, 8, 8, 8, ' ', 0)
2323
defer w.Flush()
2424

25-
spec := service.Spec
25+
task := service.Spec.Task
2626
common.FprintfIfNotEmpty(w, "ID\t: %s\n", service.ID)
27-
common.FprintfIfNotEmpty(w, "Name\t: %s\n", spec.Annotations.Name)
28-
29-
if service.PendingDelete {
30-
common.FprintfIfNotEmpty(w, "[Service %s marked for removal]\n", spec.Annotations.Name)
31-
}
32-
33-
if len(spec.Annotations.Labels) > 0 {
27+
common.FprintfIfNotEmpty(w, "Name\t: %s\n", service.Spec.Annotations.Name)
28+
if len(service.Spec.Annotations.Labels) > 0 {
3429
fmt.Fprintln(w, "Labels\t")
35-
for k, v := range spec.Annotations.Labels {
30+
for k, v := range service.Spec.Annotations.Labels {
3631
fmt.Fprintf(w, " %s\t: %s\n", k, v)
3732
}
3833
}
@@ -56,8 +51,7 @@ func printServiceSummary(service *api.Service, running int) {
5651

5752
fmt.Fprintln(w, "Template\t")
5853
fmt.Fprintln(w, " Container\t")
59-
task := spec.Task
60-
ctr := task.GetContainer()
54+
ctr := service.Spec.Task.GetContainer()
6155
common.FprintfIfNotEmpty(w, " Image\t: %s\n", ctr.Image)
6256
common.FprintfIfNotEmpty(w, " Command\t: %q\n", strings.Join(ctr.Command, " "))
6357
common.FprintfIfNotEmpty(w, " Args\t: [%s]\n", strings.Join(ctr.Args, ", "))
@@ -96,7 +90,7 @@ func printServiceSummary(service *api.Service, running int) {
9690
printResources(w, res.Limits)
9791
}
9892
}
99-
if len(spec.Task.Networks) > 0 {
93+
if len(service.Spec.Task.Networks) > 0 {
10094
fmt.Fprint(w, " Networks:")
10195
for _, n := range service.Spec.Task.Networks {
10296
fmt.Fprintf(w, " %s", n.Target)

‎manager/controlapi/network.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,8 @@ func (s *Server) removeNetwork(id string) error {
199199
return status.Errorf(codes.Internal, "could not find services using network %s: %v", id, err)
200200
}
201201

202-
for _, service := range services {
203-
if !service.PendingDelete {
204-
return status.Errorf(codes.FailedPrecondition, "network %s is in use by service %s", id, service.ID)
205-
}
202+
if len(services) != 0 {
203+
return status.Errorf(codes.FailedPrecondition, "network %s is in use by service %s", id, services[0].ID)
206204
}
207205

208206
tasks, err := store.FindTasks(tx, store.ByReferencedNetworkID(id))
@@ -216,12 +214,7 @@ func (s *Server) removeNetwork(id string) error {
216214
}
217215
}
218216

219-
network := store.GetNetwork(tx, id)
220-
if network == nil {
221-
return status.Errorf(codes.NotFound, "network %s not found", id)
222-
}
223-
network.PendingDelete = true
224-
return store.UpdateNetwork(tx, network)
217+
return store.DeleteNetwork(tx, id)
225218
})
226219
}
227220

‎manager/controlapi/network_test.go‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -191,30 +191,11 @@ func TestRemoveNetworkWithAttachedService(t *testing.T) {
191191
assert.NoError(t, err)
192192
assert.NotEqual(t, nr.Network, nil)
193193
assert.NotEqual(t, nr.Network.ID, "")
194-
createServiceInNetwork(t, ts, "service1", "image", nr.Network.ID, 1)
194+
createServiceInNetwork(t, ts, "name", "image", nr.Network.ID, 1)
195195
_, err = ts.Client.RemoveNetwork(context.Background(), &api.RemoveNetworkRequest{NetworkID: nr.Network.ID})
196196
assert.Error(t, err)
197197
}
198198

199-
func TestRemoveNetworkWithAttachedServiceMarkedForRemoval(t *testing.T) {
200-
ts := newTestServer(t)
201-
defer ts.Stop()
202-
nr, err := ts.Client.CreateNetwork(context.Background(), &api.CreateNetworkRequest{
203-
Spec: createNetworkSpec("testnet5"),
204-
})
205-
assert.NoError(t, err)
206-
assert.NotEqual(t, nr.Network, nil)
207-
assert.NotEqual(t, nr.Network.ID, "")
208-
service := createServiceInNetwork(t, ts, "service2", "image", nr.Network.ID, 1)
209-
// then let's delete the service
210-
r, err := ts.Client.RemoveService(context.Background(), &api.RemoveServiceRequest{ServiceID: service.ID})
211-
assert.NoError(t, err)
212-
assert.NotNil(t, r)
213-
// now we should be able to delete the network
214-
_, err = ts.Client.RemoveNetwork(context.Background(), &api.RemoveNetworkRequest{NetworkID: nr.Network.ID})
215-
assert.NoError(t, err)
216-
}
217-
218199
func TestCreateNetworkInvalidDriver(t *testing.T) {
219200
ts := newTestServer(t)
220201
defer ts.Stop()

‎manager/controlapi/service.go‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -722,7 +722,6 @@ func (s *Server) GetService(ctx context.Context, request *api.GetServiceRequest)
722722
// - Returns `NotFound` if the Service is not found.
723723
// - Returns `InvalidArgument` if the ServiceSpec is malformed.
724724
// - Returns `Unimplemented` if the ServiceSpec references unimplemented features.
725-
// - Returns `FailedPrecondition` if the Service is marked for removal
726725
// - Returns an error if the update fails.
727726
func (s *Server) UpdateService(ctx context.Context, request *api.UpdateServiceRequest) (*api.UpdateServiceResponse, error) {
728727
if request.ServiceID == "" || request.ServiceVersion == nil {
@@ -756,12 +755,6 @@ func (s *Server) UpdateService(ctx context.Context, request *api.UpdateServiceRe
756755
return status.Errorf(codes.NotFound, "service %s not found", request.ServiceID)
757756
}
758757

759-
// we couldn't do this any sooner, as we do need to be holding the lock
760-
// when checking for this flag
761-
if service.PendingDelete {
762-
return status.Errorf(codes.FailedPrecondition, "service %s is marked for removal", request.ServiceID)
763-
}
764-
765758
// It's not okay to update Service.Spec.Networks on its own.
766759
// However, if Service.Spec.Task.Networks is also being
767760
// updated, that's okay (for example when migrating from the
@@ -855,15 +848,12 @@ func (s *Server) RemoveService(ctx context.Context, request *api.RemoveServiceRe
855848
}
856849

857850
err := s.store.Update(func(tx store.Tx) error {
858-
service := store.GetService(tx, request.ServiceID)
859-
if service == nil {
860-
return status.Errorf(codes.NotFound, "service %s not found", request.ServiceID)
861-
}
862-
// mark service for removal
863-
service.PendingDelete = true
864-
return store.UpdateService(tx, service)
851+
return store.DeleteService(tx, request.ServiceID)
865852
})
866853
if err != nil {
854+
if err == store.ErrNotExist {
855+
return nil, status.Errorf(codes.NotFound, "service %s not found", request.ServiceID)
856+
}
867857
return nil, err
868858
}
869859
return &api.RemoveServiceResponse{}, nil

‎manager/controlapi/service_test.go‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -956,19 +956,6 @@ func TestUpdateService(t *testing.T) {
956956
assert.Equal(t, codes.InvalidArgument, testutils.ErrorCode(err))
957957
}
958958

959-
func TestUpdateServiceMarkedForRemoval(t *testing.T) {
960-
ts := newTestServer(t)
961-
defer ts.Stop()
962-
963-
service := createService(t, ts, "name", "image", 1)
964-
r, err := ts.Client.RemoveService(context.Background(), &api.RemoveServiceRequest{ServiceID: service.ID})
965-
assert.NoError(t, err)
966-
assert.NotNil(t, r)
967-
968-
_, err = ts.Client.UpdateService(context.Background(), &api.UpdateServiceRequest{ServiceID: service.ID, Spec: &service.Spec, ServiceVersion: &service.Meta.Version})
969-
assert.Equal(t, codes.FailedPrecondition, testutils.ErrorCode(err))
970-
}
971-
972959
func TestServiceUpdateRejectNetworkChange(t *testing.T) {
973960
ts := newTestServer(t)
974961
defer ts.Stop()

‎manager/deallocator/deallocator.go‎

Lines changed: 20 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package deallocator
22

33
import (
44
"context"
5-
"sync"
65

76
"github.com/docker/go-events"
87
"github.com/docker/swarmkit/api"
@@ -33,9 +32,6 @@ import (
3332
type Deallocator struct {
3433
store *store.MemoryStore
3534

36-
// closeOnce ensures that stopChan is closed only once
37-
closeOnce sync.Once
38-
3935
// for services that are shutting down, we keep track of how many
4036
// tasks still exist for them
4137
services map[string]*serviceWithTaskCounts
@@ -98,7 +94,6 @@ func (deallocator *Deallocator) Run(ctx context.Context) error {
9894

9995
return
10096
},
101-
api.EventUpdateTask{},
10297
api.EventDeleteTask{},
10398
api.EventUpdateService{},
10499
api.EventUpdateNetwork{})
@@ -134,7 +129,7 @@ func (deallocator *Deallocator) Run(ctx context.Context) error {
134129
for {
135130
select {
136131
case event := <-eventsChan:
137-
if updated, err := deallocator.handleEvent(ctx, event); err == nil {
132+
if updated, err := deallocator.processNewEvent(ctx, event); err == nil {
138133
deallocator.notifyEventChan(updated)
139134
} else {
140135
log.G(ctx).WithError(err).Errorf("error processing deallocator event %#v", event)
@@ -147,16 +142,11 @@ func (deallocator *Deallocator) Run(ctx context.Context) error {
147142
}
148143
}
149144

150-
// Stop stops the deallocator's routine and wait for the main loop to exit
151-
// Stop can be called in two cases. One when the manager is
152-
// shutting down, and the other when the manager (the leader) is
153-
// becoming a follower. Since these two instances could race with
154-
// each other, we use closeOnce here to ensure that TaskReaper.Stop()
155-
// is called only once to avoid a panic.
145+
// Stop stops the deallocator's routine
146+
// FIXME (jrouge): see the comment on TaskReaper.Stop() and see when to properly stop this
147+
// plus unit test on this!
156148
func (deallocator *Deallocator) Stop() {
157-
deallocator.closeOnce.Do(func() {
158-
close(deallocator.stopChan)
159-
})
149+
close(deallocator.stopChan)
160150
<-deallocator.doneChan
161151
}
162152

@@ -190,21 +180,11 @@ func (deallocator *Deallocator) processService(ctx context.Context, service *api
190180
// better to clean up resources that shouldn't be cleaned up yet
191181
// than ending up with a service and some resources lost in limbo forever
192182
return true, deallocator.deallocateService(ctx, service)
193-
}
194-
195-
remainingTasks := 0
196-
for _, task := range tasks {
197-
if isTaskStillAlive(task) {
198-
remainingTasks++
199-
}
200-
}
201-
202-
if remainingTasks == 0 {
183+
} else if len(tasks) == 0 {
203184
// no tasks remaining for this service, we can clean it up
204185
return true, deallocator.deallocateService(ctx, service)
205186
}
206-
207-
deallocator.services[service.ID] = &serviceWithTaskCounts{service: service, taskCount: remainingTasks}
187+
deallocator.services[service.ID] = &serviceWithTaskCounts{service: service, taskCount: len(tasks)}
208188
return false, nil
209189
}
210190

@@ -283,16 +263,24 @@ func (deallocator *Deallocator) processNetwork(ctx context.Context, tx store.Tx,
283263
return
284264
}
285265

286-
// Handles new events, and dispatches to the right method depending on what
266+
// Processes new events, and dispatches to the right method depending on what
287267
// type of event it is.
288268
// The boolean part of the return tuple indicates whether anything was actually
289269
// removed from the store
290-
func (deallocator *Deallocator) handleEvent(ctx context.Context, event events.Event) (bool, error) {
270+
func (deallocator *Deallocator) processNewEvent(ctx context.Context, event events.Event) (bool, error) {
291271
switch typedEvent := event.(type) {
292-
case api.EventUpdateTask:
293-
return deallocator.processTaskEvent(ctx, typedEvent.Task, typedEvent.OldTask)
294272
case api.EventDeleteTask:
295-
return deallocator.processTaskEvent(ctx, nil, typedEvent.Task)
273+
serviceID := typedEvent.Task.ServiceID
274+
275+
if serviceWithCount, present := deallocator.services[serviceID]; present {
276+
if serviceWithCount.taskCount <= 1 {
277+
delete(deallocator.services, serviceID)
278+
return deallocator.processService(ctx, serviceWithCount.service)
279+
}
280+
serviceWithCount.taskCount--
281+
}
282+
283+
return false, nil
296284
case api.EventUpdateService:
297285
return deallocator.processService(ctx, typedEvent.Service)
298286
case api.EventUpdateNetwork:
@@ -301,32 +289,3 @@ func (deallocator *Deallocator) handleEvent(ctx context.Context, event events.Ev
301289
return false, nil
302290
}
303291
}
304-
305-
// Common logic for handling task update/delete events
306-
// oldTask is the task object as it was before its update or deletion
307-
// newTask is nil for delete events, and the new object for updates
308-
func (deallocator *Deallocator) processTaskEvent(ctx context.Context, newTask, oldTask *api.Task) (bool, error) {
309-
serviceID := oldTask.ServiceID
310-
serviceWithCount, present := deallocator.services[serviceID]
311-
312-
if present && isTaskStillAlive(oldTask) && (newTask == nil || !isTaskStillAlive(newTask)) {
313-
// this task belongs to a service that's shutting down, and in addition,
314-
// prior to its update or deletion it was still alive, but now it's
315-
// not alive any more, so we decrement the counter of alive tasks for
316-
// this service
317-
318-
if serviceWithCount.taskCount <= 1 {
319-
delete(deallocator.services, serviceID)
320-
return deallocator.processService(ctx, serviceWithCount.service)
321-
}
322-
serviceWithCount.taskCount--
323-
}
324-
325-
return false, nil
326-
}
327-
328-
// simple helper function to distinguish tasks that are still running
329-
// from ones that are done
330-
func isTaskStillAlive(task *api.Task) bool {
331-
return task.Status.State <= api.TaskStateRunning
332-
}

0 commit comments

Comments
 (0)