Add new PortAllocator #2579

dperny · 2018-03-28T23:28:47Z

This is the first commit in a series of commits rewriting the network allocator (#2516).

This is a rewrite of the PortAllocator component. The PortAllocator is a very simple component that keeps track of which ingress network ports are in use in the cluster.

This new PortAllocator is not yet wired up, because it exposes a completely different interface compared to the existing one. It will be wired up as the rewrite progresses. It has been put here for a review, and can be reviewed as if it is to be merged, but it should not be merged at this time.

Because the PortAllocator is a simple, serial object, with a well-defined interface, I've chosen to use Ginkgo to write tests, as it has helped me enumerate the test cases I need to write and focus on testing those cases. I'm not suggesting we use Ginkgo anywhere else, and I am especially not suggesting we port any existing tests to Ginkgo. It just happens to be the right tool for this job.

At this time, the main design of PortAllocator is finished, and it only needs some more tests written.

Includes a vendoring of Ginkgo, which makes this PR look bigger than it is.

stevvooe · 2018-03-28T23:38:28Z

manager/allocator/network/port/port.go

+const (
+	// DynamicPortStart is the start of the dynamic port range from which node
+	// ports will be allocated when the user did not specify a port.
+	DynamicPortStart uint32 = 30000


I think you might be able to get away with an int16, although I wouldn't recommend using an unsigned here.

Rather, you can just use a regular int.

I use uint32 because that's the type of PortConfig.PublishPort.

Why is it using a uint32 in the first place? TCP ports are limited to the uint16 range. I don't find any reason to use a higher capacity type.

stevvooe · 2018-03-28T23:40:54Z

manager/allocator/network/port/port.go

+}
+
+// Commit commits a change to the PortAllocator. There are no safety checks.
+func (pa *PortAllocator) Commit(p *Proposal) {


I would recommend moving the Commit method to the Proposal object itself. Checkout the bolt db api for an example of how this would work.

Can this method have an error?

I would like to avoid this method having an error. I'd like to avoid having this method. It's literally just because there's no easy way to roll back to a previous PortAllocator state if you abandon allocation.

stevvooe · 2018-03-28T23:44:58Z

manager/allocator/network/port/port.go

+// as a map key
+type port struct {
+	// protocol represented by this port space
+	protocol api.PortConfig_Protocol


You can save a little space by having a struct with a port vector for each protocol:

type PortAllocator struct { udp map[port]struct{} tcp map[port]struct{} sctp map[port]struct{} }

It's possible map overhead removes this benefit, however.

Like we discussed, this is probably sparse. I did some experimentation with using big.Int as a bit set and that is pretty minimal.

It's easier to reason in the code about ports-as-a-value than it is to reason different-protocols-different-spaces.

The overhead of the extra data in each port object is minimal compared to the overhead of literally all the other dumb inefficient crap I'm doing, so even if we optimized here, it would be the tiniest win.

dperny · 2018-03-29T18:20:55Z

Updated with more tests, changed the Proposal to have a Commit method.

anshulpundir · 2018-03-29T18:28:24Z

If we're agreed on adding Ginkgo, please add it as a separate commit (and maybe PR).

dperny · 2018-03-29T19:17:31Z

I realized that mutating the endpoint object in AllocateEndpoint was dumb as hell, so I changed the interface for that method to return (as part of the Proposal) a slice of PortConfigs, which the caller should stick into the endpoint. Less mutation is a good thing.

dperny · 2018-03-29T19:18:04Z

I don't think adding Ginkgo as a separate PR is helpful. I could agree that putting the vendoring into a separate commit might be though.

codecov · 2018-03-29T21:08:18Z

Codecov Report

Merging #2579 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2579      +/-   ##
==========================================
+ Coverage   61.74%   61.83%   +0.08%     
==========================================
  Files         134      135       +1     
  Lines       21763    21893     +130     
==========================================
+ Hits        13438    13537      +99     
- Misses       6869     6905      +36     
+ Partials     1456     1451       -5

dperny · 2018-03-29T22:25:43Z

It's always a good feeling when you're writing tests to catch an error in your code.

dperny · 2018-03-29T23:50:51Z

91.8% test coverage of the new code. I just need to figure out what currently isn't tested.

dperny · 2018-03-30T18:22:55Z

100.0% test coverage. My work here is done.

dperny · 2018-03-30T19:32:02Z

I wrote a utility to spit out the specs of what's being tested. Here's the summary:

port.Allocator
    adding a new endpoint
        to a clean port allocator
            - should succeed
            - should not modify the spec
            - should not modify the endpoint
            - should not modify the Allocator state before calling Commit
            - should be idempotent
        to an allocator with ports in use
            with colliding ports in the new endpoint
                - should return ErrPortInUse
            with no colliding ports in the new endpoint
                - should succeed
                - should return ports matching the spec
            with colliding port numbers, but different protocols
                - should succeed
                - should return ports matching the spec
        with invalid publish or target ports
            - should return ErrInvalidEndpoint
    updating an endpoint
        to add ports
            if ports collide
                - should fail with ErrPortInUse
            if ports do not collide
                - should succeed
                - should add return ports matching the spec's ports
        to remove ports
            - should succeed
            - should return ports matching the spec's ports
            - should free the removed ports for another object to use
            - should not free any ports not removed
        to change name and target port
            - should be a noop
            - should return ports matching the spec's ports
        to change published port
            - should allocate the new port
            - should free the old port
            - should not free any other ports
        to change protocol
            - should allocate the new port
            - should free the old port
    dyanmic port allocation
        adding a port with no PublishPort specified
            - should succeed
            - should assign a port from the dynamic port range
            - should be idempotent
            - should not modify the spec
            - should cause new allocations to take the next available port
        when the dynamic port space is exhausted
            - should fill all the way up successfully
            - should not prevent dynamic allocation in a different port space
            adding a port to the same space
                - should return ErrPortSpaceExhausted
        when a port in the dynamic range desired to be statically allocated
            - should succeed
            - should give the statically allocated port first priority
        when changing ports
            from dynamic to statically allocated
                - should succeed
            from statically to dynamically allocated
                - should succeed
                - should free the old static allocation
                - should dynamically allocate a new port number
    endpoints with publish mode host ports
        when adding a host port
            - should succeed
            - should be a noop
            - should include the host ports in the returned ports
            - should not mark any ports in use
            - should be idemopotent
            when adding another host port with no PublishPort specified
                - should succeed
                - should be a noop
                - should not fill in the published port
    deallocate
        - should return a proposal that isn't a noop but has no ports
        - should not modify the spec
        - should free any ports in the endpoint
        - should only free endpoints of the correct protocol
        - should not free any ports with publish mode host

dperny · 2018-04-02T20:58:11Z

rebased and added doc.go.

cyli

Thanks for working on this @dperny and adding so much test coverage!!

I have a couple questions - what happens in the following cases:

If I had a previously allocated port, but changed the publish mode to host? Is the previously allocated port never deallocated?
If we're near exhausting the port space, and I update the endpoint to change the name of the dynamically allocated ports, will it allocate me the new ports or will I get an exhaustion error?

Another question, which is I think something that may be outside the scope of the port allocator, and perhaps would just be useful to comment on in doc.go - what happens if someone specifies a conflicting host port? Do you know if this is handled by some other part of swarm?

cyli · 2018-04-05T18:44:29Z

manager/allocator/network/port/doc.go

+// no PublishedPort.
+//
+// Notably, the allocator only keeps track of which ports are in use, not which
+// servies they are in use by. For the port allocator to function correctly,


Spelling nit: "servies" -> "services"

good catch.

cyli · 2018-04-05T19:00:16Z

manager/allocator/network/port/port.go

+	Commit()
+
+	// IsNoop returns true if the Proposal doesn't actually modify allocator
+	// state This does not mean the endpoint object has not been modified, just


Punctuation nit: "state This" -> "state. This"

Also further down, "it hasn't gained or lost any allocated published ports": do you mean "the allocator hasn't gained or lost any allocated published ports"? Or do you specifically mean the endpoint? Could you also give an example of when this could happen, not in a test?

Another good catch, will clarify.

cyli · 2018-04-05T20:12:39Z

manager/allocator/network/port/port.go

+	return prop.ports
+}
+
+// Commit commits the proposal to the port allocator. Part of our


Comment here ends with a half-finished sentence.

I have no idea what you

(this is a joke i'll fix it)

cyli · 2018-04-05T20:14:45Z

manager/allocator/network/port/port.go

+// Commit commits the proposal to the port allocator. Part of our
+func (prop *proposal) Commit() {
+	if prop.IsNoop() {
+		// cannot commit with nil port allocator


This doesn't seem to check prop.pa for nil, nor does prop.IsNoop() or anything that it calls - should we add a check for prop.pa? It seems like only Allocate and Deallocate construct prop.pa, and the proposal is non-exported, so not sure if we need to actually do the check - maybe just remove the comment?

You're right. I'll remove the comment. I probably had the check in IsNoop and removed it.

cyli · 2018-04-05T20:58:40Z

manager/allocator/network/port/port.go

+}
+
+func (prop *proposal) IsNoop() bool {
+	// if every port in allocate is in deallocate, and every port in deallocate


Can we just check that they both have the same length, rather than do 2 loops (e.g. check that allocate has the same keys as deallocate)? I found this statement slightly more confusing than just saying that the ports in allocate and deallocate are the same.

We can't check the length, because the actual ports in use may have changed. The comment can be reworded though.

Oops sorry I meant do a length check, then a single loop as opposed to 2 loops. If they are the same length and every port in allocate is also in reallocate they must have the same keys I think

cyli · 2018-04-05T21:23:39Z

manager/allocator/network/port/port.go

+// Allocate does _not_ change the state of the Allocator. To
+// prevent the caller from having to do bulky rollback logic, we return a
+// "Proposal" has all the information required. In order to correctly allocate
+// another endpoint with this same Allocator, the caller must later call


To be clear, given that the allocator is not concurrency-safe, do you mean that if Commit() is not called before Allocate is called on a different endpoint, the two proposals may conflict? If so, would it make sense to clarify this case (where two endpoint proposals for the same set of ports exist?)

cyli · 2018-04-05T23:27:14Z

manager/allocator/network/port/port.go

+		// one available
+		for i := DynamicPortStart; i <= DynamicPortEnd; i++ {
+			portObj.port = i
+			if _, ok := pa.ports[portObj]; !ok {


This doesn't seem to take into account any deallocated ports. If we're near almost exhausting the port space, and this update deallocates 3 ports but needs to allocate 3 different ports, we should be able to do so (at least, by share number) but we can't, since this loop isn't checking to see if the taken port has been deallocated.

I can also see how maybe that makes sense to do because if we're changing the name of the port or something else about the port config, etc., maybe it'd be confusing if the changed port config gets dynamically allocated the same port on update.

Actually, yeah, you're right. I'll need to figure out the correct solution.

Fixed this issue by adding another loop further down checking if any dynamic ports are freed.

Totally non-blocking nit - that loop looks fine and definitely fixes the issue - I was just wondering if it'd be easier to understand as an extra else block here? Since this loop already iterates over only the dynamic ports, that would mean that you may not need an extra check for whether the deallocated port is in the dynamic ports?

Also you already have a port object you can use to index into the deallocate map.

This is probably just a disfunction of how my brain works, but I find goto (or continue <label>) control flow a little more confusing to follow than just if else blocks.

doing this in two loops preserves the behavior of preferring to not reassign the same dynamic ports that were just freed over again. i don't think that that behavior is required, and i don't want to make that behavior part of the api contract, but it is rather nice. i can add a comment explaining that we try this but not to rely on it.

Also you already have a port object you can use to index into the deallocate map.

I'm brain farting on what you mean by this.

You're right about continue, though, it is a slightly-less-harmful goto.

doing this in two loops preserves the behavior of preferring to not reassign the same dynamic ports that were just freed over again. i don't think that that behavior is required, and i don't want to make that behavior part of the api contract, but it is rather nice. i can add a comment explaining that we try this but not to rely on it.

Ah that makes sense.

I'm brain farting on what you mean by this.

Er sorry, I meant if you had the one loop, you could just index into deallocated with https://github.com/docker/swarmkit/pull/2579/files#diff-50dd126d07c8b730e5bae674f9542aa2R384.

Not really relevant if we want a separate loop as you say to try to change the dynamically allocated ports each time.

cyli · 2018-04-05T23:35:00Z

manager/allocator/network/port/port_test.go

+				endpointCopy := endpoint.Copy()
+				endpointCopy.Spec = spec
+				endpointCopy.Ports = p.Ports()
+				p.Commit()


Non-blocking: would it make sense to add an extra check here before the commit to assert it's not a noop?

Sure, this test can actually probably be reworked a bit to verify that allocator state changes AFTER commit as well, but I thought I had that test someplace else...

Added a check for not noop to the "should succeed" case.

cyli · 2018-04-05T23:47:24Z

manager/allocator/network/port/port_test.go

+		})
+		// Pending, we need to test this but I'm not sure what the appropriate
+		// behavior should be
+		PContext("to change publish mode", func() {


Are we still deciding what the appropriate behavior should be? It seems like nothing gets deallocated or allocated (so if it previously had an allocated port, changing the publish mode to host would not deallocate the previous port).

Must've missed this when I was going through, I will add these test cases, thanks!

cyli · 2018-04-05T23:52:20Z

manager/allocator/network/port/port_test.go

+					Expect(e).ToNot(HaveOccurred())
+					Expect(p).ToNot(BeNil())
+					Expect(p.IsNoop()).ToNot(BeTrue())
+					Expect(p.Ports()).ToNot(BeEmpty())


Should we add more assertions that the previous dynamic allocation was freed?

Added this assertion.

dperny · 2018-04-06T00:43:55Z

Thanks for the excellent review!

If I had a previously allocated port, but changed the publish mode to host? Is the previously allocated port never deallocated?

It should be, actually, but there's no test case for it, so I'll add one and make sure.

If we're near exhausting the port space, and I update the endpoint to change the name of the dynamically allocated ports, will it allocate me the new ports or will I get an exhaustion error?

You're right, actually, that it will give an exhaustion error, I think. I'm not sure, so I'll write a test case.

dperny · 2018-04-06T19:10:23Z

Updated PR:

Added tests for changing port publish mode
Fixed issue where trying to change ports when the dynamic port range is exhausted would fail
Added more assertions
Reworded some comments

cyli

LGTM, thanks for this very thoroughly tested module @dperny! Definitely easier to understand with all the test cases written!

The one other stylistic nitpick I have which does not actually need to be done is that the Allocate function is very long - would it be easier to read if it were broken up into smaller non-exported functions, so that reading the function read a little more like pseudocode (e.g. the names of the helper functions), rather than the longer loop code? I'm totally fine with not doing this, as it's just a personal style preference.

cyli · 2018-04-07T00:42:48Z

manager/allocator/network/port/port.go

+	// object, which contains the proposed changes to the Allocator. this
+	// means if there is a failure in the caller after calling Allocate, the
+	// caller can discard the changes
+	prop := &proposal{


Non-blocking: I was wondering if we could just call Deallocate here, and modify the proposal that it returned with an allocate map? Since otherwise the code is the same.

cyli · 2018-04-07T00:53:59Z

manager/allocator/network/port/port.go

+		// one available
+		for i := DynamicPortStart; i <= DynamicPortEnd; i++ {
+			portObj.port = i
+			if _, ok := pa.ports[portObj]; !ok {


Totally non-blocking nit - that loop looks fine and definitely fixes the issue - I was just wondering if it'd be easier to understand as an extra else block here? Since this loop already iterates over only the dynamic ports, that would mean that you may not need an extra check for whether the deallocated port is in the dynamic ports?

Also you already have a port object you can use to index into the deallocate map.

This is probably just a disfunction of how my brain works, but I find goto (or continue <label>) control flow a little more confusing to follow than just if else blocks.

Adds the ginkgo BDD testing framework. Signed-off-by: Drew Erny <drew.erny@docker.com>

Adds a new, more robust implementation of assigning ports. Tested with the ginkgo testing framework. Part of a series of changes rewriting the network allocator. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny · 2018-04-17T19:32:16Z

split into two commits, one vendoring Ginkgo and the other adding the port allocator.

Adirio

I made some comments arround using uint16 but this may need to go in another place.

Additionally, port 0 is usually used when you don't mind which port it uses, it may have sense to have this same behaviour (when port 0 is specified select one in the dynamic range).

Adirio · 2018-04-19T06:48:38Z

manager/allocator/network/port/port.go

+const (
+	// DynamicPortStart is the start of the dynamic port range from which node
+	// ports will be allocated when the user did not specify a port.
+	DynamicPortStart uint32 = 30000


Why is it using a uint32 in the first place? TCP ports are limited to the uint16 range. I don't find any reason to use a higher capacity type.

Adirio · 2018-04-19T06:54:45Z

manager/allocator/network/port/port.go

+
+	// The end of master port range which will hold all the allocation state of
+	// ports allocated so far regardless of whether it was user defined or not.
+	masterPortEnd uint32 = 65535


Using an uint16, you may not need these two variables as they are the type limits.

Adirio · 2018-04-19T07:02:59Z

manager/allocator/network/port/port.go

+	//
+	// Used mostly for testing, and should probably not be relied on outside of
+	// this package.
+	IsNoop() bool


If you think it should not be trusted from outside, maybe not exporting it would be wise.

I'm going to change this comment actually. It is probably a useless function outside of the tests, but it should be able to be relied on just fine.

Adirio · 2018-04-19T07:13:23Z

manager/allocator/network/port/port.go

+
+// Deallocate takes an endpoint and provides a Proposal that will deallocate
+// all of its ports.
+func (pa *Allocator) Deallocate(endpoint *api.Endpoint) Proposal {


Should this function admit variadic arguments? func (pa *Allocator) Deallocate(endpoints ...*api.Endpoint) Proposal { Would only need using a for loop wrapping the current one and will allow current behaviour but would optimize deallocating multiple endpoints by grouping them in a single Proposal. May you ever need multiple endpoints?

Nah, because you very rarely are deallocating many endpoints at once. You're usually deallocating them one-by-one. To make use of a variadic, you'd have to write the caller to do batching. It's not worthwhile.

Adirio · 2018-04-19T07:23:26Z

manager/allocator/network/port/port.go

+		}
+		if spec.TargetPort > masterPortEnd {
+			return nil, ErrInvalidEndpoint{fmt.Sprintf("target port %v isn't in the valid port range", spec.TargetPort)}
+		}


These are the checks that could be avoided if the underlying type was uint16.

Adirio · 2018-04-19T08:05:13Z

manager/allocator/network/port/port.go

+
+type proposal struct {
+	pa         *Allocator
+	ports      []*api.PortConfig


Is ports and the getter function *proposal.Ports() being used for anything? It seems they are not needed, maybe some deprecated code?

Yes. it's the most important part, used by the caller. The alternative to Ports() would be to return (Proposal, []*api.PortConfig, error) from Allocate. That might be cleaner. WDYT?

Oh, I see, you need to modify the given []*api.PortConfig (e.g. dynamic assigned ports) but you can not do it over the given pointer as the whole two-phase model would get broken.

Between the two proposed options, I would suggest sticking to the first because all the information is contained in the proposal, you do not have to keep two variables to keep information that does not have any meaning by its own.

Adirio

Tell me what you think, mainly about extracting general allocator and proposal interfaces so that every allocator needs to implement them, as I think this will open the possibility to "plugable" allocators. Maybe this conversation is a best fit for #2516 so free fill to mention me there if you prefer to have it there.

Adirio · 2018-04-20T09:36:27Z

manager/allocator/network/port/port.go

+	// protocol represented by this port space
+	protocol api.PortConfig_Protocol
+	port     uint32
+}


I've seen a struct very similar to this in the scheduler code:

type hostPortSpec struct { protocol api.PortConfig_Protocol publishedPort uint32 }

Are you following any pattern to avoid this type duplications?

no, i'm not. the allocator code is entirely contained within itself. i don't think there's anything to be gained by deduplicating this small struct across those two components, though.

Adirio · 2018-04-24T07:22:42Z

manager/allocator/network/port/port.go

+
+type proposal struct {
+	pa         *Allocator
+	ports      []*api.PortConfig


Oh, I see, you need to modify the given []*api.PortConfig (e.g. dynamic assigned ports) but you can not do it over the given pointer as the whole two-phase model would get broken.

Between the two proposed options, I would suggest sticking to the first because all the information is contained in the proposal, you do not have to keep two variables to keep information that does not have any meaning by its own.

Adirio · 2018-04-24T07:26:03Z

manager/allocator/network/port/port.go

+
+// Restore adds the current endpoints to the local state of the port allocator
+// but does not perform any new allocation.
+func (pa *Allocator) Restore(endpoints []*api.Endpoint) {


Am I correct thinking that this method is only used just after creating the allocator? Taking it to a normal function instead of a "method", creating the allocator inside and returning the allocator looks nicer in my head, as it will ensure that it only gets called once on an empty allocator.

there are a few good reasons not to do it like this:

the constructor never returns an error, which means that anything that relies on it can in turn avoid returning errors from its constructor, all the way to the top. this is handy because when everything is said and done, it means you can have the top-level allocator component initialize the subcomponents in its constructor, and then run the Restore method in its Run function. it's hard to explain

you can call Restore multiple times, which isn't always necessary, but which is useful for testing because subtests can call Restore again with more state.

I won't mention the second reason as I consider it a nice thing but that shouldn't be taken into account while making design decissions. If you want something for the tests you can always implement non-exported methods in the test files. Don't get me wrong, it's a nice side-effect, but not one that should be looked for while taking design decissions.

About the first point, I think I understood you. I'm not suggesting removing the constructor. Further constructors could use this Restore function and fallback to New if the previous gave an error. Or you could make Restore error free by giving a blank allocator. But I get your point, offering it as a method lets the higher abstraction layer choose how to behave if unable to restore instead of giving this behaviour here. On the other side with the method version you are open to errors if it is getting called on allocators that already have some ports reserved.

Adirio · 2018-04-24T07:26:28Z

manager/allocator/network/port/port.go

+
+// Deallocate takes an endpoint and provides a Proposal that will deallocate
+// all of its ports.
+func (pa *Allocator) Deallocate(endpoint *api.Endpoint) Proposal {


Adirio · 2018-04-24T08:00:04Z

manager/allocator/network/port/port.go

+	// Used mostly for testing, and should probably not be relied on outside of
+	// this package.
+	IsNoop() bool
+}


You are creating an interface in order not to export the type itself so that proposals can not be created directly. I would go even further and create an Allocator interface and extract both interfaces to the allocator package so that all allocators need to follow these interfaces. That would requirte some generalization.

Something in the lines of:

type Allocator interface { Allocate(old, new *api.Task) (Proposal, error) Deallocate(*api.Task) (Proposal, error) } type Proposal interface { Commit() IsNOOP() bool }

And then if you want to still hide the proposal object you can make a specialized interface inside the ports subpackage:

type PortProposal interface { allocator.Proposal Ports() []*api.PortConfig }

Adirio · 2018-04-24T12:53:06Z

manager/allocator/network/port/port.go

+					portObj.port = deallocated.port
+					p.PublishedPort = deallocated.port
+					prop.allocate[portObj] = struct{}{}
+					continue ports


What about removing it from deallocate instead of inserting it into allocate?
This would remove the need to check if it is in the allocate map, reduce the iterations in this loop to get to a solution and also simplify the commit by removing one add and one remove task.

Adirio

Extra comment

Moves all of the error types into a separate package, which can be shared across all allocator components. Signed-off-by: Drew Erny <drew.erny@docker.com>

Adds a subcomponent to the network allocator with the purpose of interfacing with the IPAM drivers and allocating IP addresses for swarmkit objects. Signed-off-by: Drew Erny <drew.erny@docker.com>

Adds a subcomponent for the network allocator that has the responsibility of allocating network resources and driver state from network drivers. Signed-off-by: Drew Erny <drew.erny@docker.com>

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. Signed-off-by: Drew Erny <drew.erny@docker.com>

Adds the new top level allocator object. This component is intended to replace the existing Allocator component and exactly replicate its functionality, except without the bugs. It is responsible for translating between the swarmkit event stream and object store, and the network allocator component. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny · 2018-04-27T21:29:53Z

Ah, shit, I pushed to the wrong branch. My naming system is terrible. Anyway, closing this in favor of a pending PR.

dperny added the area/networking label Mar 28, 2018

stevvooe reviewed Mar 28, 2018

View reviewed changes

dperny force-pushed the rewrite-allocator branch from 48c10ac to 6659c83 Compare March 29, 2018 18:20

dperny force-pushed the rewrite-allocator branch 2 times, most recently from fe03354 to a683ac2 Compare March 29, 2018 19:16

dperny force-pushed the rewrite-allocator branch 4 times, most recently from 6b6d186 to ee3d8ab Compare March 29, 2018 21:08

dperny force-pushed the rewrite-allocator branch from ee3d8ab to 5cb1087 Compare March 29, 2018 22:25

dperny force-pushed the rewrite-allocator branch from 5cb1087 to 9f81f45 Compare March 29, 2018 22:34

dperny force-pushed the rewrite-allocator branch from 9f81f45 to 5443a00 Compare March 30, 2018 18:19

dperny force-pushed the rewrite-allocator branch from 5443a00 to 442b6bb Compare April 2, 2018 18:01

dperny force-pushed the rewrite-allocator branch from 442b6bb to d2bf98b Compare April 2, 2018 22:25

cyli reviewed Apr 6, 2018

View reviewed changes

dperny force-pushed the rewrite-allocator branch from d2bf98b to 382d7fb Compare April 6, 2018 19:08

dperny force-pushed the rewrite-allocator branch from 382d7fb to e7e5503 Compare April 6, 2018 19:36

cyli approved these changes Apr 7, 2018

View reviewed changes

dperny mentioned this pull request Apr 13, 2018

[WIP] Add new IPAM subcomponent #2605

Closed

dperny added 2 commits April 17, 2018 12:28

Add ginkgo

85ae4b5

Adds the ginkgo BDD testing framework. Signed-off-by: Drew Erny <drew.erny@docker.com>

Add new PortAllocator

2ccbca9

Adds a new, more robust implementation of assigning ports. Tested with the ginkgo testing framework. Part of a series of changes rewriting the network allocator. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny force-pushed the rewrite-allocator branch from e7e5503 to 2ccbca9 Compare April 17, 2018 19:31

Adirio reviewed Apr 19, 2018

View reviewed changes

Adirio reviewed Apr 24, 2018

View reviewed changes

Adirio mentioned this pull request Apr 25, 2018

[Proposal] Rewrite allocator dperny/swarmkit-1#1

Closed

dperny added 5 commits April 27, 2018 14:20

Add errors package

0cafbbe

Moves all of the error types into a separate package, which can be shared across all allocator components. Signed-off-by: Drew Erny <drew.erny@docker.com>

Add IPAM allocator

7db3584

Adds a subcomponent to the network allocator with the purpose of interfacing with the IPAM drivers and allocating IP addresses for swarmkit objects. Signed-off-by: Drew Erny <drew.erny@docker.com>

Add driver allocator

21ae142

Adds a subcomponent for the network allocator that has the responsibility of allocating network resources and driver state from network drivers. Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny closed this Apr 27, 2018

dperny mentioned this pull request Apr 27, 2018

Rewrite allocator #2615

Open

Add new PortAllocator #2579

Add new PortAllocator #2579

Conversation

dperny commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Mar 29, 2018

anshulpundir commented Mar 29, 2018

dperny commented Mar 29, 2018

dperny commented Mar 29, 2018

codecov bot commented Mar 29, 2018 • edited

Codecov Report

dperny commented Mar 29, 2018

dperny commented Mar 29, 2018

dperny commented Mar 30, 2018

dperny commented Mar 30, 2018

dperny commented Apr 2, 2018

cyli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny Apr 6, 2018 • edited

Choose a reason for hiding this comment

cyli Apr 5, 2018 • edited

Choose a reason for hiding this comment

dperny Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Apr 6, 2018

dperny commented Apr 6, 2018

cyli left a comment • edited

Choose a reason for hiding this comment

cyli Apr 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Apr 17, 2018

Adirio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adirio left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 29, 2018 •

edited

dperny Apr 6, 2018 •

edited

cyli Apr 5, 2018 •

edited

dperny Apr 6, 2018 •

edited

cyli left a comment •

edited

cyli Apr 7, 2018 •

edited

Adirio left a comment •

edited