Skip to content

Commit da5ee2a

Browse files
dpernydperny
authored
Merge pull request #2409 from dperny/workaround-attachments
Delete node attachments when node is removed
2 parents 28f91d8 + 0c7b2fc commit da5ee2a

File tree

2 files changed

+195
-0
lines changed

2 files changed

+195
-0
lines changed

‎manager/controlapi/node.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,29 @@ func (s *Server) UpdateNode(ctx context.Context, request *api.UpdateNodeRequest)
248248
}, nil
249249
}
250250

251+
func removeNodeAttachments(tx store.Tx, nodeID string) error {
252+
// orphan the node's attached containers. if we don't do this, the
253+
// network these attachments are connected to will never be removeable
254+
tasks, err := store.FindTasks(tx, store.ByNodeID(nodeID))
255+
if err != nil {
256+
return err
257+
}
258+
for _, task := range tasks {
259+
// if the task is an attachment, then we just delete it. the allocator
260+
// will do the heavy lifting. basically, GetAttachment will return the
261+
// attachment if that's the kind of runtime, or nil if it's not.
262+
if task.Spec.GetAttachment() != nil {
263+
// don't delete the task. instead, update it to `ORPHANED` so that
264+
// the taskreaper will clean it up.
265+
task.Status.State = api.TaskStateOrphaned
266+
if err := store.UpdateTask(tx, task); err != nil {
267+
return err
268+
}
269+
}
270+
}
271+
return nil
272+
}
273+
251274
// RemoveNode removes a Node referenced by NodeID with the given NodeSpec.
252275
// - Returns NotFound if the Node is not found.
253276
// - Returns FailedPrecondition if the Node has manager role (and is part of the memberlist) or is not shut down.
@@ -313,6 +336,10 @@ func (s *Server) RemoveNode(ctx context.Context, request *api.RemoveNodeRequest)
313336
return err
314337
}
315338

339+
if err := removeNodeAttachments(tx, request.NodeID); err != nil {
340+
return err
341+
}
342+
316343
return store.DeleteNode(tx, request.NodeID)
317344
})
318345
if err != nil {

‎manager/controlapi/node_test.go‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,3 +731,171 @@ func TestUpdateNodeDemote(t *testing.T) {
731731
t.Parallel()
732732
testUpdateNodeDemote(t)
733733
}
734+
735+
// TestRemoveNodeAttachments tests the unexported removeNodeAttachments
736+
// function. This avoids us having to update the TestRemoveNodes function to
737+
// test all of this logic
738+
func TestRemoveNodeAttachments(t *testing.T) {
739+
// first, set up a store and all that
740+
ts := newTestServer(t)
741+
defer ts.Stop()
742+
743+
ts.Store.Update(func(tx store.Tx) error {
744+
store.CreateCluster(tx, &api.Cluster{
745+
ID: identity.NewID(),
746+
Spec: api.ClusterSpec{
747+
Annotations: api.Annotations{
748+
Name: store.DefaultClusterName,
749+
},
750+
},
751+
})
752+
return nil
753+
})
754+
755+
// make sure before we start that our server is in a good (empty) state
756+
r, err := ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{})
757+
assert.NoError(t, err)
758+
assert.Empty(t, r.Nodes)
759+
760+
// create a manager
761+
createNode(t, ts, "id1", api.NodeRoleManager, api.NodeMembershipAccepted, api.NodeStatus_READY)
762+
r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{})
763+
assert.NoError(t, err)
764+
assert.Len(t, r.Nodes, 1)
765+
766+
// create a worker. put it in the DOWN state, which is the state it will be
767+
// in to remove it anyway
768+
createNode(t, ts, "id2", api.NodeRoleWorker, api.NodeMembershipAccepted, api.NodeStatus_DOWN)
769+
r, err = ts.Client.ListNodes(context.Background(), &api.ListNodesRequest{})
770+
assert.NoError(t, err)
771+
assert.Len(t, r.Nodes, 2)
772+
773+
// create a network we can "attach" to
774+
err = ts.Store.Update(func(tx store.Tx) error {
775+
n := &api.Network{
776+
ID: "net1id",
777+
Spec: api.NetworkSpec{
778+
Annotations: api.Annotations{
779+
Name: "net1name",
780+
},
781+
Attachable: true,
782+
},
783+
}
784+
return store.CreateNetwork(tx, n)
785+
})
786+
require.NoError(t, err)
787+
788+
// create some tasks:
789+
err = ts.Store.Update(func(tx store.Tx) error {
790+
// 1.) A network attachment on the node we're gonna remove
791+
task1 := &api.Task{
792+
ID: "task1",
793+
NodeID: "id2",
794+
DesiredState: api.TaskStateRunning,
795+
Status: api.TaskStatus{
796+
State: api.TaskStateRunning,
797+
},
798+
Spec: api.TaskSpec{
799+
Runtime: &api.TaskSpec_Attachment{
800+
Attachment: &api.NetworkAttachmentSpec{
801+
ContainerID: "container1",
802+
},
803+
},
804+
Networks: []*api.NetworkAttachmentConfig{
805+
{
806+
Target: "net1id",
807+
Addresses: []string{}, // just leave this empty, we don't need it
808+
},
809+
},
810+
},
811+
// we probably don't care about the rest of the fields.
812+
}
813+
if err := store.CreateTask(tx, task1); err != nil {
814+
return err
815+
}
816+
817+
// 2.) A network attachment on the node we're not going to remove
818+
task2 := &api.Task{
819+
ID: "task2",
820+
NodeID: "id1",
821+
DesiredState: api.TaskStateRunning,
822+
Status: api.TaskStatus{
823+
State: api.TaskStateRunning,
824+
},
825+
Spec: api.TaskSpec{
826+
Runtime: &api.TaskSpec_Attachment{
827+
Attachment: &api.NetworkAttachmentSpec{
828+
ContainerID: "container2",
829+
},
830+
},
831+
Networks: []*api.NetworkAttachmentConfig{
832+
{
833+
Target: "net1id",
834+
Addresses: []string{}, // just leave this empty, we don't need it
835+
},
836+
},
837+
},
838+
// we probably don't care about the rest of the fields.
839+
}
840+
if err := store.CreateTask(tx, task2); err != nil {
841+
return err
842+
}
843+
844+
// 3.) A regular task on the node we're going to remove
845+
task3 := &api.Task{
846+
ID: "task3",
847+
NodeID: "id2",
848+
DesiredState: api.TaskStateRunning,
849+
Status: api.TaskStatus{
850+
State: api.TaskStateRunning,
851+
},
852+
Spec: api.TaskSpec{
853+
Runtime: &api.TaskSpec_Container{
854+
Container: &api.ContainerSpec{},
855+
},
856+
},
857+
}
858+
if err := store.CreateTask(tx, task3); err != nil {
859+
return err
860+
}
861+
862+
// 4.) A regular task on the node we're not going to remove
863+
task4 := &api.Task{
864+
ID: "task4",
865+
NodeID: "id1",
866+
DesiredState: api.TaskStateRunning,
867+
Status: api.TaskStatus{
868+
State: api.TaskStateRunning,
869+
},
870+
Spec: api.TaskSpec{
871+
Runtime: &api.TaskSpec_Container{
872+
Container: &api.ContainerSpec{},
873+
},
874+
},
875+
}
876+
return store.CreateTask(tx, task4)
877+
})
878+
require.NoError(t, err)
879+
880+
// Now, call the function with our nodeID. make sure it returns no error
881+
err = ts.Store.Update(func(tx store.Tx) error {
882+
return removeNodeAttachments(tx, "id2")
883+
})
884+
require.NoError(t, err)
885+
886+
// Now, make sure only task1, the network-attacahed task on id2, was
887+
// removed
888+
ts.Store.View(func(tx store.ReadTx) {
889+
tasks, err := store.FindTasks(tx, store.All)
890+
require.NoError(t, err)
891+
// should only be 3 tasks left
892+
require.Len(t, tasks, 4)
893+
// and the list should not contain task1
894+
for _, task := range tasks {
895+
require.NotNil(t, task)
896+
if task.ID == "task1" {
897+
require.Equal(t, task.Status.State, api.TaskStateOrphaned)
898+
}
899+
}
900+
})
901+
}

0 commit comments

Comments
 (0)