From 4a526e7029fde06f06d182abcfb7a283d04ba0a2 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Mon, 4 Jun 2018 10:39:21 -0700 Subject: [PATCH] Continue fixing allocator Continues improving the new allocator, after having removed the Networks field from the Service object Signed-off-by: Drew Erny --- manager/allocator/allocator_new.go | 40 +++- .../allocator_new_integration_test.go | 175 ++++++++++++------ manager/allocator/network/allocator.go | 50 +++-- manager/allocator/network/allocator_test.go | 24 ++- manager/allocator/network/driver/driver.go | 20 ++ .../network/driver/driver_suite_test.go | 13 ++ .../allocator/network/driver/driver_test.go | 34 ++++ manager/allocator/network/ipam/ipam.go | 67 ++++--- manager/allocator/network/ipam/ipam_test.go | 36 ++++ manager/allocator/network/port/port.go | 4 +- .../onsi/ginkgo/extensions/table/table.go | 98 ++++++++++ .../ginkgo/extensions/table/table_entry.go | 81 ++++++++ 12 files changed, 531 insertions(+), 111 deletions(-) create mode 100644 manager/allocator/network/driver/driver_suite_test.go create mode 100644 manager/allocator/network/driver/driver_test.go create mode 100644 vendor/github.com/onsi/ginkgo/extensions/table/table.go create mode 100644 vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go diff --git a/manager/allocator/allocator_new.go b/manager/allocator/allocator_new.go index 790a354a00..716301812f 100644 --- a/manager/allocator/allocator_new.go +++ b/manager/allocator/allocator_new.go @@ -458,24 +458,34 @@ func (a *NewAllocator) handleEvent(ctx context.Context, event events.Event) { } case api.EventCreateTask, api.EventUpdateTask: var t *api.Task - if e, ok := ev.(api.EventCreateTask); ok { + if e, ok := ev.(api.EventUpdateTask); ok { + // before we do any processing, check the old task. if the previous + // state was a terminal state, then the task must necessarily have + // already been deallocated. + if e.OldTask != nil && !isTaskLive(e.OldTask) { + return + } t = e.Task } else { - t = ev.(api.EventUpdateTask).Task + t = ev.(api.EventCreateTask).Task } if t == nil { // if for some reason there is no task, nothing to do return } + // updating a task may mean the task has entered a terminal state. if // it has, we will free its network resources just as if we had deleted // it. - if t.Status.State >= api.TaskStateCompleted { + if !isTaskLive(t) { + // if the task was already in a terminal state, we should ignore + // this update, not try to deallocate it again if _, ok := a.deletedObjects[t.ID]; ok { delete(a.deletedObjects, t.ID) return } + log.G(ctx).WithField("task.id", t.ID).Debug("deallocating task") allocDone := metrics.StartTimer(allocatorActions.WithValues("task", "deallocate")) err := a.network.DeallocateTask(t) allocDone() @@ -492,7 +502,7 @@ func (a *NewAllocator) handleEvent(ctx context.Context, event events.Event) { if ev.Task == nil { return } - if ev.Task.Status.State >= api.TaskStateCompleted { + if !isTaskLive(ev.Task) { // if the task is being deleted from a terminal state, we will have // deallocated it when it moved into that terminal state, and do // not need to do so now. @@ -975,9 +985,7 @@ func (a *NewAllocator) processPendingAllocations(ctx context.Context) { for _, task := range allocatedTasks { if err := batch.Update(func(tx store.Tx) error { currentTask := store.GetTask(tx, task.ID) - if currentTask == nil || - currentTask.Status.State != api.TaskStateNew || - currentTask.DesiredState != api.TaskStateRunning { + if !isTaskLive(currentTask) { log.G(ctx).WithField("task.id", task.ID).Debug("task terminated or removed after allocation but before commit") a.network.DeallocateTask(task) a.deletedObjects[task.ID] = struct{}{} @@ -1005,3 +1013,21 @@ func (a *NewAllocator) processPendingAllocations(ctx context.Context) { } } } + +// isTaskLive returns true if the task is non-nil and in a non-terminal state. +// +// this function, though simple, has been factored out to avoid any errors that +// may arise from slightly different checks for liveness (for example, +// task.Status.State > api.TaskStateRunning vs task.Status.State >= api.TaskStateCompleted +func isTaskLive(t *api.Task) bool { + // a nil task is not live, of course + if t == nil { + return false + } + // a task past the RUNNING state is not live. + if t.Status.State > api.TaskStateRunning { + return false + } + // otherwise, it's live + return true +} diff --git a/manager/allocator/allocator_new_integration_test.go b/manager/allocator/allocator_new_integration_test.go index 9ddbf48748..58ca7587ad 100644 --- a/manager/allocator/allocator_new_integration_test.go +++ b/manager/allocator/allocator_new_integration_test.go @@ -3,26 +3,21 @@ package allocator // TODO(dperny): rename this file to allocator_test.go import ( - // testing imports . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" . "github.com/onsi/gomega/types" - // standard libraries "context" "fmt" "reflect" "time" - // the packages under in this integration test // imported for PortsMostlyEqual "github.com/docker/swarmkit/manager/allocator/network/port" - // external libraries "github.com/docker/libnetwork/ipamapi" "github.com/sirupsen/logrus" - // our libraries "github.com/docker/swarmkit/api" "github.com/docker/swarmkit/log" "github.com/docker/swarmkit/manager/state/store" @@ -38,6 +33,16 @@ const ( ingressID = "ingressNetID" ) +func init() { + // set some defaults specific to this test + // because batching is done every 500 ms, a polling interval shorter than + // that is useless + SetDefaultEventuallyPollingInterval(500 * time.Millisecond) + // allocation should be pretty quick, so use a 3 second timeout. + SetDefaultEventuallyTimeout(3 * time.Second) + +} + // This suite is an integration test of the allocator, which uses real, live // components. It should be a definitive test of the allocator's public API. If // these tests pass, then any changes to the allocator should not require @@ -54,6 +59,39 @@ var _ = Describe("allocator.NewAllocator", func() { allocatorExitResult chan error ) + // define some quick closures for getting a network, service, or task + // these return a function to be passed to Eventually. we define them here + // because the dataStore object only exists in this scope + getNetwork := func(id string) func() *api.Network { + return func() *api.Network { + var network *api.Network + dataStore.View(func(tx store.ReadTx) { + network = store.GetNetwork(tx, id) + }) + return network + } + } + + getService := func(id string) func() *api.Service { + return func() *api.Service { + var service *api.Service + dataStore.View(func(tx store.ReadTx) { + service = store.GetService(tx, id) + }) + return service + } + } + + getTask := func(id string) func() *api.Task { + return func() *api.Task { + var task *api.Task + dataStore.View(func(tx store.ReadTx) { + task = store.GetTask(tx, id) + }) + return task + } + } + // Before we start the tests, we need to create a store and an allocator. // We won't start the allocator until just before the tests, so that the // subspecs can add data to it before initialization @@ -195,45 +233,11 @@ var _ = Describe("allocator.NewAllocator", func() { JustBeforeEach(func() { // there are a lot of Eventually calls here. these calls poll the // enclosed function and check the matcher against their result - Eventually(func() *api.Network { - var nw *api.Network - dataStore.View(func(tx store.ReadTx) { - nw = store.GetNetwork(tx, nw1) - }) - return nw - }, 3*time.Second, 500*time.Millisecond).Should(BeAllocated()) - - Eventually(func() *api.Network { - var nw *api.Network - dataStore.View(func(tx store.ReadTx) { - nw = store.GetNetwork(tx, nw2) - }) - return nw - }, 3*time.Second, 500*time.Millisecond).Should(BeAllocated()) - - Eventually(func() *api.Network { - var nw *api.Network - dataStore.View(func(tx store.ReadTx) { - nw = store.GetNetwork(tx, nw3) - }) - return nw - }, 3*time.Second, 500*time.Millisecond).Should(BeAllocated()) - - Eventually(func() *api.Service { - var service *api.Service - dataStore.View(func(tx store.ReadTx) { - service = store.GetService(tx, serviceID) - }) - return service - }, 3*time.Second, 500*time.Millisecond).Should(BeAllocated()) - - Eventually(func() *api.Task { - var task *api.Task - dataStore.View(func(tx store.ReadTx) { - task = store.GetTask(tx, taskID) - }) - return task - }, 3*time.Second, 500*time.Millisecond).Should(BeAllocated()) + Eventually(getNetwork(nw1)).Should(BeAllocated()) + Eventually(getNetwork(nw2)).Should(BeAllocated()) + Eventually(getNetwork(nw3)).Should(BeAllocated()) + Eventually(getService(serviceID)).Should(BeAllocated()) + Eventually(getTask(taskID)).Should(BeAllocated()) }) It("should successfully update the service", func() { @@ -278,21 +282,76 @@ var _ = Describe("allocator.NewAllocator", func() { }) Expect(err).ToNot(HaveOccurred()) - Eventually(func() *api.Task { - var t *api.Task - dataStore.View(func(tx store.ReadTx) { - t = store.GetTask(tx, newTaskID) - }) - return t - }).Should(BeAllocated()) + Eventually(getTask(newTaskID)).Should(BeAllocated()) + Eventually(getService(serviceID)).Should(BeAllocated()) + }) + }) - Eventually(func() *api.Service { - var s *api.Service - dataStore.View(func(tx store.ReadTx) { - s = store.GetService(tx, serviceID) - }) - return s - }).Should(BeAllocated()) + // TODO(dperny): how do we determine that an object was correctly + // _deallocated_? deallocation is an action that is not exposed on the + // public interface of the allocator + PDescribe("Moving a task to a terminal state", func() { + var ( + serviceID = "termainalService" + taskID = "terminalTask" + ) + BeforeEach(func() { + // create a service + service := &api.Service{ + ID: serviceID, + Spec: api.ServiceSpec{ + Task: api.TaskSpec{}, + // if we just need to attach to some arbitrary network in + // a test, adding a port for ingress does the trick + Endpoint: &api.EndpointSpec{ + Ports: []*api.PortConfig{ + {TargetPort: 80, PublishedPort: 80}, + }, + }, + }, + } + + // create a task. + task := &api.Task{ + ID: taskID, + ServiceID: serviceID, + Spec: service.Spec.Task, + DesiredState: api.TaskStateRunning, + } + + // now add them in the store + err := dataStore.Update(func(tx store.Tx) error { + err := store.CreateService(tx, service) + Expect(err).ToNot(HaveOccurred()) + + err = store.CreateTask(tx, task) + Expect(err).ToNot(HaveOccurred()) + return nil + }) + Expect(err).ToNot(HaveOccurred()) + }) + + JustBeforeEach(func() { + // Make sure before we start the test that the service and task are + // allocated + Eventually(getService(serviceID)).Should(BeAllocated()) + Eventually(getTask(taskID)).Should(BeAllocated()) + }) + + It("should deallocate a task that moves to the terminal state", func() { + // TODO(dperny): how do we verify that a task has been or has not + // been deallocated...? + dataStore.Update(func(tx store.Tx) error { + // get the task and move it into a terminal state + task := store.GetTask(tx, taskID) + task.Status.State = api.TaskStateFailed + err := store.UpdateTask(tx, task) + Expect(err).ToNot(HaveOccurred()) + return nil + }) + }) + + It("should not deallocate a task that is updated when its in a terminal state", func() { }) }) }) diff --git a/manager/allocator/network/allocator.go b/manager/allocator/network/allocator.go index b923c59589..ce71a90a1b 100644 --- a/manager/allocator/network/allocator.go +++ b/manager/allocator/network/allocator.go @@ -116,7 +116,12 @@ func NewAllocator(pg plugingetter.PluginGetter) Allocator { // provided. // // If an error occurs during the restore, the local state may be inconsistent, -// and this allocator should be abandoned. +// and this allocator should be abandoned. Restore will only return errors when +// proceeding with the provided objects will lead to an inconsistent state in +// which the correct operation of the allocator cannot proceed. If there are +// other kinds of errors where swarmkit will work incorrectly but the state +// doesn't cause the allocator to, e.g., allocator duplicate IP addresses, we +// allow Restore to proceed. func (a *allocator) Restore(networks []*api.Network, services []*api.Service, tasks []*api.Task, nodes []*api.Node) error { // there is a problem with restoring nodes: because node deallocation // depends on network deallocation, it is possible for a network to be @@ -148,9 +153,12 @@ func (a *allocator) Restore(networks []*api.Network, services []*api.Service, ta // cannot be deleted while it is still in use by tasks. existingNetworks := make(map[string]struct{}, len(networks)) - // find if we have an ingress network in this list. if so, save its ID. we - // need it to correctly allocate tasks and services. there should only ever - // be 1 ingress network + // Find if we have an ingress network in this list. If so, save its ID. we + // need it to correctly allocate tasks and services. There should only ever + // be 1 ingress network, and it cannot be node local. We do not check if an + // ingress network is a node-local network, because while such a state + // would be invalid, it's not the kind of invalid state that will break the + // correct operation of the allocator. for _, nw := range networks { existingNetworks[nw.ID] = struct{}{} @@ -168,7 +176,12 @@ func (a *allocator) Restore(networks []*api.Network, services []*api.Service, ta // check if the network is node-local, and add it to our set if so. if local { - a.nodeLocalNetworks[nw.ID] = nw + // before adding to the set of node local networks, check if this + // network is allocated. only add allocated networks to the set of + // node local networks. + if driver.IsAllocated(nw) { + a.nodeLocalNetworks[nw.ID] = nw + } } } @@ -261,6 +274,8 @@ func (a *allocator) AllocateNetwork(n *api.Network) error { if err := a.ipam.AllocateNetwork(n); err != nil { return err } + } else { + a.nodeLocalNetworks[n.ID] = n } if err := a.driver.Allocate(n); err != nil { a.ipam.DeallocateNetwork(n) @@ -443,12 +458,14 @@ func (a *allocator) AllocateService(service *api.Service) error { // check the resolution mode of the new endpoint spec. If it's // ResolutionModeDNSRoundRobin, then we need to deallocate vips, not - // allocate them. + // allocate them. ResolutionModeDNSRoundRobin means that instead of + // addressing and load balancing a service based on a VIP, DNS queries + // round-robin between task IP addresses (returning a different task IP + // each time). if endpointSpec.Mode == api.ResolutionModeDNSRoundRobin { a.ipam.DeallocateVIPs(endpoint) endpoint.VirtualIPs = nil } else { - // we don't need to allocate vips if the publish mode is DNSRR if err := a.ipam.AllocateVIPs(endpoint, ids); err != nil { // if the error is a result of anything other than the fact that we're // already allocated, return it @@ -489,13 +506,13 @@ func (a *allocator) DeallocateService(service *api.Service) error { // with the Endpoint of its corresponding service. // // Before calling AllocateTask, the caller must make sure that the service is -// fully allocated. If the service's allocation state is out of data, the task +// fully allocated. If the service's allocation state is out of date, the task // wil inherit that out of date state. // // AllocateTask can only be called on New tasks, and should only be called // once. It cannot handle task updates. // -// If the return value if nil, then the task has been fully allocated. +// If the return value is nil, then the task has been fully allocated. // Otherwise, the task has not been allocated at all. This method will never // leave the task in a partially allocated state. func (a *allocator) AllocateTask(task *api.Task) (rerr error) { @@ -607,8 +624,10 @@ func (a *allocator) AllocateNode(node *api.Node, requestedNetworks map[string]st networks[nw] = struct{}{} } - // before we do anything, add the ingress network if it exists to the - // networks map. we always need an ingress network attachment. + // the node always needs a network attachment to the ingress network. if it + // exists, add the ingress network to the list of requested networks now. + // it may already be in the requested networks, but we if it is, then + // nothing has been altered. if a.ingressID != "" { // if for some reason, the caller has already added the ingress network // to the networks list, this will do nothing, which isn't a problem. @@ -632,12 +651,12 @@ func (a *allocator) AllocateNode(node *api.Node, requestedNetworks map[string]st } } - // TODO(dperny): code to support the singular network attachment. remove. + // TODO(dperny): this is code to support the singular network attachment. + // remove in a few releases so we don't have to support it. if node.Attachment != nil && node.Attachment.Network != nil { if _, ok := networks[node.Attachment.Network.ID]; ok { keep = append(keep, node.Attachment) delete(networks, node.Attachment.Network.ID) - // we can go ahead and nil out the attachment } else { remove = append(remove, node.Attachment) } @@ -697,8 +716,9 @@ func (a *allocator) AllocateNode(node *api.Node, requestedNetworks map[string]st } node.Attachments = append(keep, finalAttachments...) - // TODO(dperny): code to support the singular network attachment nil out - // the singular node attachment, so we never have to think of it again. + // TODO(dperny): code to support the singular network attachment, remove in + // a few releases. nil out the singular node attachment, so we never have + // to think of it again. node.Attachment = nil return nil } diff --git a/manager/allocator/network/allocator_test.go b/manager/allocator/network/allocator_test.go index 1a830b24f3..3969bc4bdb 100644 --- a/manager/allocator/network/allocator_test.go +++ b/manager/allocator/network/allocator_test.go @@ -95,19 +95,30 @@ var _ = Describe("network.Allocator", func() { Expect(err).ToNot(HaveOccurred()) }) }) - Context("when there are some networks allocated", func() { + Context("when there are some networks present", func() { BeforeEach(func() { initNetworks = []*api.Network{ { ID: "fooNet", }, { + // this local network is allocated and should be added + // to the nodeLocalNetworks list ID: "localnet", + DriverState: &api.Driver{ + Name: "local", + }, + }, + { + // this local network is not allocated and should not + // be added to nodeLocalNetworks on Restore + ID: "localnet2", }, } // fooNet will be global, but localnet will be local mockDriver.EXPECT().IsNetworkNodeLocal(initNetworks[0]).Return(false, nil) mockDriver.EXPECT().IsNetworkNodeLocal(initNetworks[1]).Return(true, nil) + mockDriver.EXPECT().IsNetworkNodeLocal(initNetworks[2]).Return(true, nil) mockPort.EXPECT().Restore([]*api.Endpoint{}) mockIpam.EXPECT().Restore(initNetworks, []*api.Endpoint{}, []*api.NetworkAttachment{}).Return(nil) mockDriver.EXPECT().Restore(initNetworks).Return(nil) @@ -121,9 +132,11 @@ var _ = Describe("network.Allocator", func() { // networks. this spec has been left here for documentation // purposes }) - It("should keep track of node-local networks", func() { - Expect(a.nodeLocalNetworks).To(HaveKey("localnet")) - Expect(a.nodeLocalNetworks["localnet"]).To(Equal(initNetworks[1])) + It("should keep track of allocated node-local networks", func() { + Expect(a.nodeLocalNetworks).To(HaveKeyWithValue( + "localnet", initNetworks[1], + )) + Expect(a.nodeLocalNetworks).ToNot(HaveKey("localnet2")) }) }) Context("when objects that depend on networks are allocated", func() { @@ -507,6 +520,9 @@ var _ = Describe("network.Allocator", func() { It("should return no error", func() { Expect(err).ToNot(HaveOccurred()) }) + It("should add the network to the map of node-local networks", func() { + Expect(a.nodeLocalNetworks).To(HaveKeyWithValue(net.ID, net)) + }) }) Context("when the network driver is invalid", func() { rerr := errors.ErrInvalidSpec("invalid driver") diff --git a/manager/allocator/network/driver/driver.go b/manager/allocator/network/driver/driver.go index f797ce802e..2976dfa13b 100644 --- a/manager/allocator/network/driver/driver.go +++ b/manager/allocator/network/driver/driver.go @@ -257,3 +257,23 @@ func getIpamData(n *api.Network) ([]driverapi.IPAMData, error) { } return ipamData, nil } + +// IsAllocated returns true if driver state of the provided network is fully +// allocated. +func IsAllocated(n *api.Network) bool { + // driver state should not be nil + // NOTE(dperny): really, the only vital portion of this is DriverState is + // non-nil and name is not empty. however, including the other checks + // future-proofs this function a bit. + return n.DriverState != nil && + // driver name should not be empty + n.DriverState.Name != "" && + // either there is a DriverConfig in the spec... + ((n.Spec.DriverConfig != nil && + // and the name either matches + (n.Spec.DriverConfig.Name == n.DriverState.Name || + // or is emptystring + n.Spec.DriverConfig.Name == "")) || + // ... or there is no DriverConfig at all at all + n.Spec.DriverConfig == nil) +} diff --git a/manager/allocator/network/driver/driver_suite_test.go b/manager/allocator/network/driver/driver_suite_test.go new file mode 100644 index 0000000000..fda1523db8 --- /dev/null +++ b/manager/allocator/network/driver/driver_suite_test.go @@ -0,0 +1,13 @@ +package driver + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "testing" +) + +func TestDriver(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Driver Suite") +} diff --git a/manager/allocator/network/driver/driver_test.go b/manager/allocator/network/driver/driver_test.go new file mode 100644 index 0000000000..eca41b3130 --- /dev/null +++ b/manager/allocator/network/driver/driver_test.go @@ -0,0 +1,34 @@ +package driver + +import ( + . "github.com/onsi/ginkgo/extensions/table" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/docker/swarmkit/api" +) + +var _ = Describe("driver package", func() { + DescribeTable("driver.IsAllocated", func(n *api.Network, expected bool) { + Expect(n).To(WithTransform(IsAllocated, Equal(expected))) + }, + Entry("nil driver", &api.Network{ + DriverState: nil, + }, false), + Entry("empty driver name", &api.Network{ + DriverState: &api.Driver{Name: ""}, + }, false), + Entry("empty spec", &api.Network{ + DriverState: &api.Driver{Name: "overlay"}, + Spec: api.NetworkSpec{DriverConfig: &api.Driver{Name: ""}}, + }, true), + Entry("matching spec", &api.Network{ + DriverState: &api.Driver{Name: "fooName"}, + Spec: api.NetworkSpec{DriverConfig: &api.Driver{Name: "fooName"}}, + }, true), + Entry("nil spec", &api.Network{ + DriverState: &api.Driver{Name: "overlay"}, + }, true), + ) +}) diff --git a/manager/allocator/network/ipam/ipam.go b/manager/allocator/network/ipam/ipam.go index 614a5bb654..dfbac6c8cc 100644 --- a/manager/allocator/network/ipam/ipam.go +++ b/manager/allocator/network/ipam/ipam.go @@ -49,6 +49,11 @@ type Allocator interface { AllocateVIPs(*api.Endpoint, map[string]struct{}) error DeallocateVIPs(*api.Endpoint) AllocateAttachment(*api.NetworkAttachmentConfig) (*api.NetworkAttachment, error) + // DeallocateAttachment is the only Deallocate method that returns an + // error. It is desirable for them all to do so, but the error handling + // flow for deallocation has not been fully designed yet, and its benefits + // are comparatively marginal, so in the interest of time, the other + // methods do not have error returns implemented yet. DeallocateAttachment(*api.NetworkAttachment) error } @@ -466,8 +471,20 @@ func (a *allocator) AllocateNetwork(n *api.Network) (rerr error) { } // add the option indicating that we're gonna request a gateway, and // remove it before we exit this function + // NOTE(dperny): I don't think there are alternate values of + // RequestAddressType, but just in case, we should save whatever + // previous value there may have been + prevReqAddrType, hasPrevReqAddrType := ipamOpts[ipamapi.RequestAddressType] ipamOpts[ipamapi.RequestAddressType] = netlabel.Gateway - defer delete(ipamOpts, ipamapi.RequestAddressType) + defer func() { + // if there was a value, set it back to that value. otherwise, just + // delete from the map + if hasPrevReqAddrType { + ipamOpts[ipamapi.RequestAddressType] = prevReqAddrType + } else { + delete(ipamOpts, ipamapi.RequestAddressType) + } + }() if config.Gateway != "" || gwIP == nil { gwIP, _, err = ipam.RequestAddress(poolID, net.ParseIP(config.Gateway), ipamOpts) if err != nil { @@ -559,35 +576,35 @@ func (a *allocator) AllocateVIPs(endpoint *api.Endpoint, networkIDs map[string]s // allocation by using this as a guess keep := make([]*api.Endpoint_VirtualIP, 0, len(endpoint.VirtualIPs)) deallocate := []*api.Endpoint_VirtualIP{} - // first, we need to figure out if any virtual IPs are being removed - // continues are bad and hard to follow, so here's the plain english - // explanation: - // for every VIP currently allocated - // go through the list of desired network IDs - // if a network ID matches the ID on the VIP - // then we're keeping this vip, so add it to the keep list and - // go to the next VIPs, skipping the bottom of this loop - // if we get to this point, then we have been through every desired - // network ID and not found one matching the one on this VIP, so we - // can add it to the list of VIPs to deallocate + // nwidsInVips is a set of all of network IDs with a VIP currently + // allocated, used to verify which vips we already have allocated. + nwidsInVips := make(map[string]struct{}, len(endpoint.VirtualIPs)) + + // go through all of the VIPs we have, and sort them into VIPs we're + // keeping and vips we're removing. in addition, make note of which + // networks have a VIP allocated already, so we can quickly figure out + // which networks we need to allocate for. + // + // NOTE(dperny): another possible optimization may be to copy the + // networkIDs map, and then delete each network ID found in the endpoint + // already from the map, leaving us after with a map containing only the + // network IDs we need to newly allocate. for _, vip := range endpoint.VirtualIPs { if _, ok := networkIDs[vip.NetworkID]; ok { keep = append(keep, vip) } else { deallocate = append(deallocate, vip) } + nwidsInVips[vip.NetworkID] = struct{}{} } - // now figure out which new network IDs we've added, which is the same loop - // above but swapped around to check network IDs against VIPs -newvips: + // now go through all the networks we desire to have allocated. If any of + // those networks does not already have an VIP allocated on the endpoint, + // add it to the list of networks we're allocating. for nwid := range networkIDs { - for _, vip := range endpoint.VirtualIPs { - if vip.NetworkID == nwid { - continue newvips - } + if _, ok := nwidsInVips[nwid]; !ok { + allocate = append(allocate, nwid) } - allocate = append(allocate, nwid) } // create a new slice to hold the vips we're allocating now @@ -654,7 +671,7 @@ allocateLoop: // network, and each network in turn has non-overlapping subnets, there is // no chance of IPs we're releasing to be reused in the allocation of new // VIPs. So, instead, we deallocate last, so that if any allocation fails, - // we only have to roll back incomplete allocation, not re-allocation a + // we only have to roll back incomplete allocation, not re-allocate a // release. We don't have to worry about re-allocating if deallocate fails, // because if deallocation fails we are in a world of hurt. a.deallocateVIPs(deallocate) @@ -676,12 +693,11 @@ func (a *allocator) deallocateVIPs(deallocate []*api.Endpoint_VirtualIP) { // because the higher levels won't allow the deletion of a network // which still has resources attached, so no need to check ok local := a.networks[vip.NetworkID] - // get the IPAM driver for this network. no need to check that the fi - ipam, _ := a.drvRegistry.IPAM(local.nw.IPAM.Driver.Name) // we don't need to check that the IPAM driver is non-nil because the // network being successfully allocated indicates that it is not. If it // is nil, we should probably crash the program anyway cause that's not // right + ipam, _ := a.drvRegistry.IPAM(local.nw.IPAM.Driver.Name) // we don't need to check err, because we set this value to begin with. // if we inherited some bogus value from an old iteration of the @@ -766,8 +782,9 @@ func (a *allocator) AllocateAttachment(config *api.NetworkAttachmentConfig) (att if rerr != nil { for _, addr := range attach.Addresses { poolID := local.endpoints[addr] - addr := net.ParseIP(addr) - ipam.ReleaseAddress(poolID, addr) + ip := net.ParseIP(addr) + ipam.ReleaseAddress(poolID, ip) + delete(local.endpoints, addr) } } }() diff --git a/manager/allocator/network/ipam/ipam_test.go b/manager/allocator/network/ipam/ipam_test.go index 9068f36762..4d7d91acb8 100644 --- a/manager/allocator/network/ipam/ipam_test.go +++ b/manager/allocator/network/ipam/ipam_test.go @@ -542,6 +542,7 @@ var _ = Describe("ipam.Allocator", func() { } reg.ipams["default"] = ipamAndCaps{mock, nil} }) + Context("when the user has specified no settings", func() { BeforeEach(func() { network = &api.Network{ @@ -577,7 +578,9 @@ var _ = Describe("ipam.Allocator", func() { Expect(poolsRequested).To(Equal(1)) Expect(addressesRequested).To(Equal(0)) }) + }) + Context("when the IPAM driver returns no gateway address", func() { BeforeEach(func() { mock.requestPoolFunc = func(_, _, _ string, _ map[string]string, _ bool) (string, *net.IPNet, map[string]string, error) { @@ -602,6 +605,7 @@ var _ = Describe("ipam.Allocator", func() { Expect(network.IPAM.Configs[0].Gateway).To(Equal("192.168.2.2")) }) }) + Context("when a gateway address is specified by the user", func() { var ( addressRequested string @@ -636,8 +640,24 @@ var _ = Describe("ipam.Allocator", func() { Expect(network.IPAM.Configs[0]).ToNot(BeNil()) Expect(network.IPAM.Configs[0].Subnet).To(Equal("192.168.2.0/24")) Expect(network.IPAM.Configs[0].Gateway).To(Equal("192.168.2.99")) + Expect(network.IPAM.Driver.Options).NotTo(HaveKey(ipamapi.RequestAddressType)) + }) + + Context("when a value for ipamapi.RequestAddressType is set", func() { + BeforeEach(func() { + network.Spec.IPAM.Driver = &api.Driver{ + Options: map[string]string{ipamapi.RequestAddressType: "nondefaultvalue"}, + } + }) + + It("should restore the value set by the user", func() { + Expect(network.IPAM.Driver.Options).To(HaveKeyWithValue( + ipamapi.RequestAddressType, "nondefaultvalue", + )) + }) }) }) + Context("when specifying an IPAM driver", func() { var ( err error @@ -1237,6 +1257,7 @@ var _ = Describe("ipam.Allocator", func() { }) }) }) + // TODO(dperny): write these tests. // honestly i'm so tired of writing tests jfc // please no more @@ -1249,6 +1270,7 @@ var _ = Describe("ipam.Allocator", func() { JustBeforeEach(func() { attachment, err = a.AllocateAttachment(spec) }) + Context("when an attachment has no addresses specified", func() { BeforeEach(func() { spec = &api.NetworkAttachmentConfig{ @@ -1272,6 +1294,7 @@ var _ = Describe("ipam.Allocator", func() { Expect(spec.DriverAttachmentOpts).To(Equal(attachment.DriverAttachmentOpts)) }) }) + Context("when an attachment has addresses specified", func() { BeforeEach(func() { spec = &api.NetworkAttachmentConfig{ @@ -1292,6 +1315,19 @@ var _ = Describe("ipam.Allocator", func() { )) }) }) + + // TODO(dperny): this test case is trivial, but the code is a pain + // in the butt to write because of the really ugly fake IPAM i use, + // so I'm leaving the test case here for some enterprising + // contributor to finish out later. + PContext("when allocating the attachment addresses fails partway through", func() { + It("should release any addresses already allocated", func() { + + }) + + It("should remove those addresses from the endpoints map", func() { + }) + }) }) }) }) diff --git a/manager/allocator/network/port/port.go b/manager/allocator/network/port/port.go index bc884b0832..bbd43ce624 100644 --- a/manager/allocator/network/port/port.go +++ b/manager/allocator/network/port/port.go @@ -226,7 +226,7 @@ func (pa *allocator) Allocate(endpoint *api.Endpoint, spec *api.EndpointSpec) (P // user's spec wants a new (dynamically allocated) port, just like if we // had dynamically assigned the port. // - // Luckily for use, we keep a copy of the spec on the Endpoint object, + // Luckily for us, we keep a copy of the spec on the Endpoint object, // which we own, which means we have the old endpoint spec around and can // compare the user's specs. Then, all we have to do is see if the // published port changed. @@ -236,7 +236,7 @@ func (pa *allocator) Allocate(endpoint *api.Endpoint, spec *api.EndpointSpec) (P // So, basically, here's what we're going to do: we're going to create a // new list of PortConfigs. This will be the final list that gets put into - // the object endpoint object + // the endpoint object finalPorts := make([]*api.PortConfig, len(spec.Ports)) // first, we need to "recover" any dynamically assigned publish ports. to diff --git a/vendor/github.com/onsi/ginkgo/extensions/table/table.go b/vendor/github.com/onsi/ginkgo/extensions/table/table.go new file mode 100644 index 0000000000..ae8ab7d248 --- /dev/null +++ b/vendor/github.com/onsi/ginkgo/extensions/table/table.go @@ -0,0 +1,98 @@ +/* + +Table provides a simple DSL for Ginkgo-native Table-Driven Tests + +The godoc documentation describes Table's API. More comprehensive documentation (with examples!) is available at http://onsi.github.io/ginkgo#table-driven-tests + +*/ + +package table + +import ( + "fmt" + "reflect" + + "github.com/onsi/ginkgo" +) + +/* +DescribeTable describes a table-driven test. + +For example: + + DescribeTable("a simple table", + func(x int, y int, expected bool) { + Ω(x > y).Should(Equal(expected)) + }, + Entry("x > y", 1, 0, true), + Entry("x == y", 0, 0, false), + Entry("x < y", 0, 1, false), + ) + +The first argument to `DescribeTable` is a string description. +The second argument is a function that will be run for each table entry. Your assertions go here - the function is equivalent to a Ginkgo It. +The subsequent arguments must be of type `TableEntry`. We recommend using the `Entry` convenience constructors. + +The `Entry` constructor takes a string description followed by an arbitrary set of parameters. These parameters are passed into your function. + +Under the hood, `DescribeTable` simply generates a new Ginkgo `Describe`. Each `Entry` is turned into an `It` within the `Describe`. + +It's important to understand that the `Describe`s and `It`s are generated at evaluation time (i.e. when Ginkgo constructs the tree of tests and before the tests run). + +Individual Entries can be focused (with FEntry) or marked pending (with PEntry or XEntry). In addition, the entire table can be focused or marked pending with FDescribeTable and PDescribeTable/XDescribeTable. +*/ +func DescribeTable(description string, itBody interface{}, entries ...TableEntry) bool { + describeTable(description, itBody, entries, false, false) + return true +} + +/* +You can focus a table with `FDescribeTable`. This is equivalent to `FDescribe`. +*/ +func FDescribeTable(description string, itBody interface{}, entries ...TableEntry) bool { + describeTable(description, itBody, entries, false, true) + return true +} + +/* +You can mark a table as pending with `PDescribeTable`. This is equivalent to `PDescribe`. +*/ +func PDescribeTable(description string, itBody interface{}, entries ...TableEntry) bool { + describeTable(description, itBody, entries, true, false) + return true +} + +/* +You can mark a table as pending with `XDescribeTable`. This is equivalent to `XDescribe`. +*/ +func XDescribeTable(description string, itBody interface{}, entries ...TableEntry) bool { + describeTable(description, itBody, entries, true, false) + return true +} + +func describeTable(description string, itBody interface{}, entries []TableEntry, pending bool, focused bool) { + itBodyValue := reflect.ValueOf(itBody) + if itBodyValue.Kind() != reflect.Func { + panic(fmt.Sprintf("DescribeTable expects a function, got %#v", itBody)) + } + + if pending { + ginkgo.PDescribe(description, func() { + for _, entry := range entries { + entry.generateIt(itBodyValue) + } + }) + } else if focused { + ginkgo.FDescribe(description, func() { + for _, entry := range entries { + entry.generateIt(itBodyValue) + } + }) + } else { + ginkgo.Describe(description, func() { + for _, entry := range entries { + entry.generateIt(itBodyValue) + } + }) + } +} diff --git a/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go b/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go new file mode 100644 index 0000000000..5fa645bcee --- /dev/null +++ b/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go @@ -0,0 +1,81 @@ +package table + +import ( + "reflect" + + "github.com/onsi/ginkgo" +) + +/* +TableEntry represents an entry in a table test. You generally use the `Entry` constructor. +*/ +type TableEntry struct { + Description string + Parameters []interface{} + Pending bool + Focused bool +} + +func (t TableEntry) generateIt(itBody reflect.Value) { + if t.Pending { + ginkgo.PIt(t.Description) + return + } + + values := []reflect.Value{} + for i, param := range t.Parameters { + var value reflect.Value + + if param == nil { + inType := itBody.Type().In(i) + value = reflect.Zero(inType) + } else { + value = reflect.ValueOf(param) + } + + values = append(values, value) + } + + body := func() { + itBody.Call(values) + } + + if t.Focused { + ginkgo.FIt(t.Description, body) + } else { + ginkgo.It(t.Description, body) + } +} + +/* +Entry constructs a TableEntry. + +The first argument is a required description (this becomes the content of the generated Ginkgo `It`). +Subsequent parameters are saved off and sent to the callback passed in to `DescribeTable`. + +Each Entry ends up generating an individual Ginkgo It. +*/ +func Entry(description string, parameters ...interface{}) TableEntry { + return TableEntry{description, parameters, false, false} +} + +/* +You can focus a particular entry with FEntry. This is equivalent to FIt. +*/ +func FEntry(description string, parameters ...interface{}) TableEntry { + return TableEntry{description, parameters, false, true} +} + +/* +You can mark a particular entry as pending with PEntry. This is equivalent to PIt. +*/ +func PEntry(description string, parameters ...interface{}) TableEntry { + return TableEntry{description, parameters, true, false} +} + +/* +You can mark a particular entry as pending with XEntry. This is equivalent to XIt. +*/ +func XEntry(description string, parameters ...interface{}) TableEntry { + return TableEntry{description, parameters, true, false} +}