Fix IP overlap with empty EndpointSpec #2505

fcrisciani · 2018-02-06T02:51:06Z

Passing and empty EndpointSpec in the service spec
was correctly triggering the VIP allocation but
the leader election was erroneusly handling the IPAM
state restore.
This fix ensure that the EndpointSpec if not specified is
actually added to the ServiceSpec selection endpoint mode VIP.
Also the allocate service has now the restart flag that will skip
the deallocation logic that was erroneously triggered.

Fix: moby/moby#33795

Signed-off-by: Flavio Crisciani flavio.crisciani@docker.com

codecov · 2018-02-06T03:07:57Z

Codecov Report

Merging #2505 into master will increase coverage by 0.36%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
+ Coverage   61.32%   61.68%   +0.36%     
==========================================
  Files         131      129       -2     
  Lines       21451    21293     -158     
==========================================
- Hits        13154    13135      -19     
+ Misses       6878     6753     -125     
+ Partials     1419     1405      -14

marcusmartins · 2018-02-06T03:10:32Z

Cc @anshulpundir @dperny @nishanttotla

anshulpundir · 2018-02-06T19:21:09Z

manager/allocator/network.go

@@ -940,7 +940,7 @@ func updatePortsInHostPublishMode(s *api.Service) {
 	s.Endpoint.Spec = s.Spec.Endpoint.Copy()
 }

-func (a *Allocator) allocateService(ctx context.Context, s *api.Service) error {
+func (a *Allocator) allocateService(ctx context.Context, s *api.Service, restart bool) error {


Please add a comment for this function along with a description for each of the params.

anshulpundir · 2018-02-06T19:25:38Z

manager/controlapi/service.go

@@ -306,6 +306,7 @@ func validateTaskSpec(taskSpec api.TaskSpec) error {
 func validateEndpointSpec(epSpec *api.EndpointSpec) error {
 	// Endpoint spec is optional
 	if epSpec == nil {
+		epSpec = &api.EndpointSpec{Mode: api.ResolutionModeVirtualIP}


I would suggest doing this outside this function. This function is for validation purposes only and should not have any side effects.

I strongly agree. We should not modify the user's spec after it's been sent to Swarmkit. There are Good Reasons to do this.

moved it outside

also probably return an InvalidArgument here.

then we don't need to set any default if we bail out with an error

The change essentially is to make EndpointSpec mandatory, correct ? So, to avoid breaking old clients, we're populating it in swarm.

Its OK to return an error from validateEndpointSpec and handle it wherever it is called by filling in an empty EndpointSpec.

anshulpundir · 2018-02-06T19:26:13Z

manager/allocator/network.go

@@ -1188,7 +1190,7 @@ func (a *Allocator) procUnallocatedServices(ctx context.Context) {
 	var allocatedServices []*api.Service
 	for _, s := range nc.unallocatedServices {
 		if !nc.nwkAllocator.IsServiceAllocated(s) {
-			if err := a.allocateService(ctx, s); err != nil {
+			if err := a.allocateService(ctx, s, false); err != nil {


nit: add a comment on why restart flag is false here.

anshulpundir · 2018-02-06T19:27:22Z

manager/allocator/network.go

@@ -587,8 +587,8 @@ func (a *Allocator) allocateServices(ctx context.Context, existingAddressesOnly
 			continue
 		}

-		if err := a.allocateService(ctx, s); err != nil {
-			log.G(ctx).WithError(err).Errorf("failed allocating service %s during init", s.ID)
+		if err := a.allocateService(ctx, s, existingAddressesOnly); err != nil {


nit: Please add a comment on how existingAddressesOnly relates to the restart flag which is an arg for allocateService()

change the name of the variable to match to show that the relation is that they are the same thing

Did you mean to make this change ? I don't see it in the patch @fcrisciani

the name of the variable is changed inside the allocateService not here, I kept the original name existingAddressesOnly

anshulpundir · 2018-02-06T19:28:00Z

manager/allocator/network.go

@@ -274,7 +274,7 @@ func (a *Allocator) doNetworkAlloc(ctx context.Context, ev events.Event) {
 			}
 			updatePortsInHostPublishMode(s)
 		} else {
-			if err := a.allocateService(ctx, s); err != nil {
+			if err := a.allocateService(ctx, s, false); err != nil {


nit: add a comment on why restart flag is false here.

anshulpundir · 2018-02-06T19:28:07Z

manager/allocator/network.go

@@ -244,7 +244,7 @@ func (a *Allocator) doNetworkAlloc(ctx context.Context, ev events.Event) {
 			break
 		}

-		if err := a.allocateService(ctx, s); err != nil {
+		if err := a.allocateService(ctx, s, false); err != nil {


nit: add a comment on why restart flag is false here.

I put a comment on the allocateService itself, instead to spread a one line everywhere saying that is not a restart case

anshulpundir · 2018-02-06T19:28:55Z

manager/allocator/cnmallocator/networkallocator.go

@@ -244,6 +245,7 @@ vipLoop:
 		}
 		for _, nAttach := range specNetworks {
 			if nAttach.Target == eAttach.NetworkID {
+				log.L.WithFields(logrus.Fields{"service_id": s.ID, "vip": eAttach.Addr}).Infof("allocate vip")


super nit: use Info instead fo Infof ?

I think this should be Debug ? Info will print it for every ip ?

I was actually thinking to want this printed. Concerns?

@fcrisciani only concern is that it might be too frequent to print in the logs. We normally would like to avoid that.

Definitely change from Info to Debug. It'll be too verbose for Info.

@nishanttotla today we have 0 visibility and debug is very difficult, do you have any other idea?

Guys putting it to debug, but we will continue to have no clue of what is going on and what is assigned especially when allocation are not correct

anshulpundir · 2018-02-06T19:29:09Z

manager/allocator/allocator_test.go

@@ -796,6 +796,145 @@ func TestAllocatorRestoreForDuplicateIPs(t *testing.T) {
 	}
 }

+func TestAllocatorRestartNoEndpointSpec(t *testing.T) {


Please add a summery of this test case.

abhi

LGTM. One nit

abhi · 2018-02-06T19:35:51Z

manager/allocator/cnmallocator/networkallocator.go

@@ -244,6 +245,7 @@ vipLoop:
 		}
 		for _, nAttach := range specNetworks {
 			if nAttach.Target == eAttach.NetworkID {
+				log.L.WithFields(logrus.Fields{"service_id": s.ID, "vip": eAttach.Addr}).Infof("allocate vip")


I think this should be Debug ? Info will print it for every ip ?

nishanttotla · 2018-02-06T20:10:52Z

manager/allocator/network.go

-		if err := a.allocateService(ctx, s); err != nil {
-			log.G(ctx).WithError(err).Errorf("failed allocating service %s during init", s.ID)
+		if err := a.allocateService(ctx, s, existingAddressesOnly); err != nil {
+			log.G(ctx).WithField("existingAddressesOnly", existingAddressesOnly).WithError(err).Errorf("failed allocating service %s during init", s.ID)


nit: can this comment be made more clear? Should be it something like "failed to allocate network..." or something of that nature, perhaps with more info?

The error field will give that information.

dperny · 2018-02-07T22:30:19Z

LGTM

anshulpundir

Looks good. Minor nits. I'll merge as soon as you address these.

anshulpundir · 2018-02-07T23:06:57Z

manager/allocator/allocator_test.go

@@ -796,6 +796,149 @@ func TestAllocatorRestoreForDuplicateIPs(t *testing.T) {
 	}
 }

+// TestAllocatorRestartNoEndpointSpec covers the leader election case when the service Spec
+// does not contains the EndpointSpec.


nit: does not contains => does not contain

anshulpundir · 2018-02-07T23:07:17Z

manager/allocator/allocator_test.go

@@ -796,6 +796,149 @@ func TestAllocatorRestoreForDuplicateIPs(t *testing.T) {
 	}
 }

+// TestAllocatorRestartNoEndpointSpec covers the leader election case when the service Spec
+// does not contains the EndpointSpec.
+// The expected behavior iis that the VIP(s) are still correctly populated inside


anshulpundir · 2018-02-07T23:08:15Z

manager/allocator/network.go

@@ -587,8 +587,8 @@ func (a *Allocator) allocateServices(ctx context.Context, existingAddressesOnly
 			continue
 		}

-		if err := a.allocateService(ctx, s); err != nil {
-			log.G(ctx).WithError(err).Errorf("failed allocating service %s during init", s.ID)
+		if err := a.allocateService(ctx, s, existingAddressesOnly); err != nil {


Did you mean to make this change ? I don't see it in the patch @fcrisciani

anshulpundir · 2018-02-07T23:09:03Z

manager/allocator/network.go

@@ -940,7 +940,10 @@ func updatePortsInHostPublishMode(s *api.Service) {
 	s.Endpoint.Spec = s.Spec.Endpoint.Copy()
 }

-func (a *Allocator) allocateService(ctx context.Context, s *api.Service) error {
+// allocateService takes care to align the desired state with the spec passed
+// the last parameter is true only during restart when the data are read from raft


nit: data are => data is

Passing and empty EndpointSpec in the service spec was correctly triggering the VIP allocation but the leader election was erroneusly handling the IPAM state restore trying to release the VIP. The fix focuses on proper handling of the restart case. Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the fixendpointspec branch from 060844d to 687a9ae Compare February 6, 2018 02:56

fcrisciani force-pushed the fixendpointspec branch from 687a9ae to 944e9a6 Compare February 6, 2018 15:59

anshulpundir suggested changes Feb 6, 2018

View reviewed changes

abhi approved these changes Feb 6, 2018

View reviewed changes

nishanttotla reviewed Feb 6, 2018

View reviewed changes

fcrisciani force-pushed the fixendpointspec branch 2 times, most recently from 9be2619 to 03eda38 Compare February 7, 2018 22:12

anshulpundir approved these changes Feb 7, 2018

View reviewed changes

fcrisciani force-pushed the fixendpointspec branch from 03eda38 to bd4e923 Compare February 7, 2018 23:16

nishanttotla approved these changes Feb 7, 2018

View reviewed changes

anshulpundir merged commit 608c4dd into moby:master Feb 8, 2018

fcrisciani deleted the fixendpointspec branch February 8, 2018 19:31

This was referenced Feb 10, 2018

Bump SwarmKit to f74983e7c015a38a81c8642803a78b8322cf7eac moby/moby#36274

Merged

[17.12] Fix IP overlap with empty EndpointSpec #2513

Merged

fcrisciani mentioned this pull request Mar 12, 2018

Problem with overlay network and DNS resolution between containers moby/moby#30487

Closed

fcrisciani mentioned this pull request Mar 22, 2018

Wrong behaviour of the DNS resolver within Swarm Mode overlay network moby/moby#30134

Closed

Fix IP overlap with empty EndpointSpec #2505

Fix IP overlap with empty EndpointSpec #2505

Conversation

fcrisciani commented Feb 6, 2018

codecov bot commented Feb 6, 2018 • edited Loading

Codecov Report

marcusmartins commented Feb 6, 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

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

nishanttotla Feb 6, 2018 • edited Loading

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

abhi 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

dperny commented Feb 7, 2018

anshulpundir 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

codecov bot commented Feb 6, 2018 •

edited

Loading

nishanttotla Feb 6, 2018 •

edited

Loading