New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully Implement New Allocator #2686

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dperny
Member

dperny commented Jul 5, 2018

The branch new-allocator has been pushed to this repo. It fully moves swarmkit to the new allocator introduced in #2615, and removes all code related to the old allocator.

That branch has been added so that developers can easily vendor in this new allocator for testing. If there are any problems with the new allocator, they can be discussed in the comments here.

When we're confident that the new allocator is no worse than the old one, we can merge this branch and delete it.

This branch and PR includes all of the commits in #2615, plus a new commit removing the old allocator. To view only the difference between this branch and #2615, see here: dperny/swarmkit-1@rewrite-allocator...docker:new-allocator. Please take care not to review on this PR changes that are in the other PR. I'm sorry for the trouble that might cause.

@dperny dperny force-pushed the new-allocator branch from 5c978d5 to 62ab3a3 Jul 5, 2018

@codecov

This comment has been minimized.

codecov bot commented Jul 5, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@496d19b). Click here to learn what that means.
The diff coverage is 72.78%.

@@            Coverage Diff            @@
##             master    #2686   +/-   ##
=========================================
  Coverage          ?   62.19%           
=========================================
  Files             ?      136           
  Lines             ?    21614           
  Branches          ?        0           
=========================================
  Hits              ?    13442           
  Misses            ?     6820           
  Partials          ?     1352

@dperny dperny force-pushed the new-allocator branch from 62ab3a3 to 4b32032 Aug 3, 2018

dperny added some commits Apr 17, 2018

Add ginkgo
Adds the ginkgo BDD testing framework.

Signed-off-by: Drew Erny <drew.erny@docker.com>
Add new allocator
Rewrites the manager/allocator component to be more readable,
documented, tested, and correct. The new allocator is tested with the
Ginkgo BDD testing framework

Allocator shares one set of errors across the whole implementation,
defined in the manager/allocator/network/errors package

Adds a new, more robust implementation of assigning ports. Tested with
the ginkgo testing framework.

Adds a subcomponent to the network allocator with the purpose of
interfacing with the IPAM drivers and allocating IP addresses for
swarmkit objects.

Adds a subcomponent for the network allocator that has the
responsibility of allocating network resources and driver state from
network drivers.

Adds the highest-level component charged with allocating network
resources from from the underlying libraries. The network allocator
provides an interface between the higher level swarmkit objects
(services, tasks and nodes), and the lower level swarmkit objects
(endpoints, attachments) that the lower level-allocator components
operate on.

Adds mocks, and the makefile rules to generate them. Readds a vendoring
of the mocking library.

Adds the top-level allocator component, (called NewAllocator while this
PR is still WIP), which is the component that interacts with the store
and event stream. Copies the tests from the old allocator, and starts
adapting them to the new one. Changes some tests from the old allocator
that fail because of assumptions that no longer hold.

Adds prometheus metrics collection to the new allocator.

Signed-off-by: Drew Erny <drew.erny@docker.com>
Remove deprecated ServiceSpec.Networks field
This field has, in various forms, been deprecated since at least 2016,
and we should be well clear to remove it.

Signed-off-by: Drew Erny <drew.erny@docker.com>
Continue fixing allocator
Continues improving the new allocator, after having removed the Networks
field from the Service object

Signed-off-by: Drew Erny <drew.erny@docker.com>
removed all of the old allocator
Signed-off-by: Drew Erny <drew.erny@docker.com>
Update vendoring
Some kind of update, possibly to vndr, means a revendoring is required
to pass CI.

Signed-off-by: Drew Erny <drew.erny@docker.com>

@dperny dperny force-pushed the new-allocator branch from 9dbbb08 to bc6aa61 Aug 7, 2018

@dperny dperny referenced this pull request Aug 21, 2018

Open

Rewrite Allocator #2516

@@ -49,6 +51,7 @@ setup: ## install dependencies
@go get -u github.com/client9/misspell/cmd/misspell
@go get -u github.com/lk4d4/vndr
@go get -u github.com/stevvooe/protobuild
@go get -u github.com/golang/mock/mockgen

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

We didn't have any mocking lib before? Also, why is this not in a dedicated vendor.conf file instead of here?

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Also, if we never needed mocking till now... why do we need it now? :)

@@ -157,3 +161,18 @@ dep-validate:
(echo >&2 "+ inconsistent dependencies! what you have in vendor.conf does not match with what you have in vendor" && false)
@rm -Rf vendor
@mv .vendor.bak vendor
mocks: mock-ipam mock-driver mock-port
@echo "🐳 $@"

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

This is going to echo after generating the mocks are generated. Not the worst thing, but different behaviour from the other targets. Feel free to ignore :)

mock-port: manager/allocator/network/port/port.go
mkdir -p mocks/mock_port/
mockgen -source=$^ -destination=$(MOCKS_PACKAGE)/mock_port/mock_port.go

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Might be somewhat cleaner as follows:

ALLOCATOR_NETWORK_MOCKS = $(addprefix mock-, ipam driver port)
mocks: $(ALLOCATOR_NETWORK_MOCKS)

mock-%: manager/allocator/network/$*/$*.go
  mkdir -p mocks/mock_$*/
  mockgen -source=$^ -destination=$(MOCKS_PACKAGE)/mock_$*/mock_$*.go
@@ -61,6 +61,7 @@ message NodeSpec {
// strategy and restart policy, a number of application-level behaviors can be
// defined.
message ServiceSpec {
reserved 7;

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Nifty. Didn't know that was a thing, but makes sense when thinking about it for a sec!

// task is successfully allocated
AllocatedStatusMessage = "pending task scheduling"
maxBatchInterval = 500 * time.Millisecond

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Worth a comment? (do I understand correctly that it's the max interval the allocator will wait for before processing new requests, the idea being that processing in batches is less expensive?)

It("should not deallocate a task that is updated when its in a terminal state", func() {
})
})
})

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Wow. Nice tests, thanks!

// 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
// changes to other components that depend on it.

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Then shouldn't it belong in the integration dir?

package allocator
import (
. "github.com/onsi/ginkgo"

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Just a question: what's the philosophy going forward? Should new integration tests use gingko?

"github.com/docker/swarmkit/manager/state"
"github.com/docker/swarmkit/manager/state/store"
// "github.com/sirupsen/logrus"

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Then remove?

DesiredState: api.TaskStateRunning,
}
assert.NoError(t, store.CreateTask(tx, t2))
// Create the predefined node-local network with one service
assert.NoError(t, store.CreateNetwork(tx, p))
// assert.NoError(t, store.CreateNetwork(tx, p))

This comment has been minimized.

@wk8

wk8 Sep 20, 2018

Collaborator

Would be cleaner to actually remove all of these instead of just commenting out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment