Add knobs on LB sandbox #2154

fcrisciani · 2018-05-18T21:22:32Z

Refactored the ostweaks file and made it more generic to be reused.

codecov-io · 2018-05-18T21:42:52Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9e422a6). Click here to learn what that means.
The diff coverage is 9.47%.

@@            Coverage Diff            @@
##             master    #2154   +/-   ##
=========================================
  Coverage          ?   40.33%           
=========================================
  Files             ?      141           
  Lines             ?    22550           
  Branches          ?        0           
=========================================
  Hits              ?     9096           
  Misses            ?    12120           
  Partials          ?     1334

Impacted Files	Coverage Δ
store.go	`60.43% <0%> (ø)`
osl/namespace_linux.go	`34.59% <0%> (ø)`
osl/kernel/knobs.go	`0% <0%> (ø)`
endpoint_info.go	`59.33% <0%> (ø)`
drivers/overlay/ostweaks_linux.go	`0% <0%> (ø)`
service_common.go	`4.16% <0%> (ø)`
default_gateway.go	`26.08% <0%> (ø)`
service_linux.go	`1.31% <1.13%> (ø)`
sandbox.go	`45.69% <20%> (ø)`
network.go	`62.6% <21.42%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e422a6...43e2b5c. Read the comment docs.

ctelfer

There are some issues w/ CI that I think are just a matter of including the wrong package in a few places.

Aside from that my only major concern is with the ApplyOSTweaks() interface method added to osl/sandbox.go. See comment for details.

ctelfer · 2018-05-21T13:57:49Z

drvregistry/drvregistry_test.go

-	"Indicates if the test is running in a container")
+	// this takes care of the incontainer flag
+	_ "github.com/docker/libnetwork/netutils"
+)


Should this be "github.com/docker/libnetwork/testutils" instead of "...netutils"? Would explain why Circle-CI is failing.

totally, I'm not sure how did I put the wrong package there.

ctelfer · 2018-05-21T13:58:48Z

networkdb/networkdb_test.go

-var (
-	dbPort             int32 = 10000
-	runningInContainer       = flag.Bool("incontainer", false, "Indicates if the test is running in a container")
+	_ "github.com/docker/libnetwork/netutils"
 )


yep in all the tests

ctelfer · 2018-05-21T14:06:18Z

osl/kernel/knobs_linux.go

+				continue
+			}
+			logrus.Debugf("updated kernel parameter %s = %s (was %s)", k, v.Value, oldv)
+		}


As long as we are touching this ... do we want to add a debug log when we don't update a property to indicate why? Something like:

} else if oldv != v.Value { logrus.Debugf("leaving kernel parameter %s as %s instead of updating to %s", k, oldv, v.Value) }

Just a thought.

ctelfer · 2018-05-21T14:07:13Z

osl/kernel/knobs_linux_test.go

+	"github.com/stretchr/testify/assert"
+
+	_ "github.com/docker/libnetwork/netutils"
+)


here again for testutils instead of netutils?

ctelfer · 2018-05-21T14:11:44Z

types/types_test.go


-var runningInContainer = flag.Bool("incontainer", false, "Indicates if the test is running in a container")
+	_ "github.com/docker/libnetwork/netutils"
+)


last one for testutils?

ctelfer · 2018-05-21T14:37:39Z

osl/sandbox.go

@@ -70,6 +70,9 @@ type Sandbox interface {

 	// restore sandbox
 	Restore(ifsopt map[string][]IfaceOption, routes []*types.StaticRoute, gw net.IP, gw6 net.IP) error
+
+	// ApplyOSTweaks applies operating system specific knobs on the sandbox
+	ApplyOSTweaks()


It would probably be good to flesh this interface out a bit. As written, I don't get what this function actually means. If there are special OS tweaks that apply to all sandboxes, then why not do it in NewSandbox(). If it only applies to some sandboxes (which is what we really want here) how does the sandbox determine whether it is special? Right now, we just have this optionally called in createLoadBalancerSandbox(), but what if there are other types of sandboxes that need OS tweaks down the road? Do they all get the same sets of tweaks?

There are at least two two general paths forward that have better "code hygiene".

Rename this to ApplyLoadBalancerOSTweaks() -- that makes it clear that this is just for tweaking that particular special type of sandbox.

Change the method signature to receive some kind of parameters indicating what kinds of tweaks to apply. A simplistic view would be a bool parameter indicating the LB sandbox status. But a more generic approach would be a map[string]string parameter which indicates some kind of OS-specific setting that should be applied. In the case of load balancer sandboxes it would likely be something as simple as map[string]string{"loadBalancer":"true"}. The OS specific sandbox could would then look through the keys to see which of key's it might have a tweak perhaps modulating how to apply it based on the value. Then it would know what to try to apply.

Option 1 is the simplest and its reasonably clean. I would go with this if we really don't foresee any other types of sandbox tweaks down the road ever.

Option 2 may be over-engineering a bit for the purpose. But it is the better long term interface IMHO. If we imagine that there could be other OS-specific tweaks, then we'd have to change the sandbox interface again down the road with Option 1. Option 2, by contrast would require changes only to the backend code for those OSes affected by the new option and nothing to the interface at all.

BTW, with Option 2 controller.go code above would then change to:

sb.osSbox.ApplyOSTweaks(map[string]string{"loadBalancer": fmt.Sprintf("%v", sb.loadBalancer)})

Come to think of it, it would be good to options well documented so, we might want to put the key ("loadBalancer") as a constant in osl/sandbox.go.

There may well be other options. That's just what has occurred to me so far.

I was trying to keep it simple because I'm not sure which is going to be the next use of it, so did not want to push into a direction that makes it less flexible when the next purpose will be found.
Anyway I agree with the general POV that this is a too simplistic and not power approach.
I personally don't like the option 1, only because will put assumption on the sandbox interface, that not necessarily reflects a generic sandbox implementation.
I like more the second option, so my proposal is to change the interface of the sandbox method like:

ApplyOSTweaks(tweaks map[string]*kernel.OSValue)

WDYT?

Option 2 sounds sounds cleaner way for long run .

Was the idea that each OSL would declare something like:

var SandboxLoadBalancerTweaks = map[string]*kernel.OSValue{...} // possibly empty

and then the createLoadBalancerSandbox() function would do something like:
sb.ApplyOSTweaks(osl.SandboxLoadBalancerTweaks)?

This could be a reasonable approach. I think that means less complexity in the solution with the cost of requiring changes in all (ok: all 2-3 of them :) ) OSLs whenever we add a sandbox tweak.

ctelfer · 2018-05-21T20:27:03Z

Overall, I like the new approach @fcrisciani just posted much better. My one suggestion (which I'd love to hear other's opinions on as well) would be to remove the loadBalancer bool from sandbox.go and replace it with oslTypes []osl.SandboxType. If we do that then OptionLoadBalancer becomes:

func OptionLoadBalancer() SandboxOption {
	return func(sb *sandbox) {
		sb.oslTypes = append(sb.oslTypes, osl.SandboxTypeLoadBalancer)
	}
}

and the controller.go simply becomes:

sb.osSbox.ApplyOSTweaks(sb.oslTypes...)

This makes NewSandbox() stop testing for special cases and it allows adding different types of tweaks w/o necessarily having to change the sandbox data structure down the road.

fcrisciani · 2018-05-21T20:30:52Z

@ctelfer yep makes sense the only think that stopped me to follow such approach was that at this point Ingress should follow the same approach of being a sandbox property, but then now being a slice of properties will make it more difficult to identify it, pushing the need to have it as a map, but that will make it more complex for the real objective of this patch. Thoughts?

ctelfer · 2018-05-21T21:03:08Z

Agree that this change would suggest folding 'ingress' into the other types .... So I guess that points to two possible questions:

Is ingress special enough that it clearly should be treated differently than other possible 'types'?
Is loadBalancer special enough that it clearly needs to be called out as its own property?

Re: 1 -- I think the answer is "yes". We can only have 1 ingress sandbox per controller and being ingress affects the redirect rules programmed into the sandbox. So I don't think we want to remove this field.

Re: 2 -- I could see it being state that we want to return in a query. But it has no real impact to the logic of the OSL code at the moment. So ... dunno ... could go either way with its inclusion. It's not currently that useful, but conceptually it seems correct.

We could also keep the two ideas (OSL tweaks and load-balancer-ness) orthogonal/separate. In that model we have the loadBalancer bool as well as the oslTypes []osl.SandboxType fields. The load balancer option would set both of them. The former would be used to answer queries as well and be something we key off of for any non-OS-specific libnetwork code pertaining to load-balancer sandboxes. The latter is purely for OSL bringup/tuning. Intersection is only conceptually because the option ties them together.

fcrisciani · 2018-05-23T21:17:05Z

@ctelfer fair enough, at the moment I removed the bool for the load balancer and I added the type Ingress and LB

ctelfer

Thanks! LGTM

Refactor the ostweaks file to allows a more easy reuse Add a method on the osl.Sandbox interface to allow setting knobs on the sandbox Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani · 2018-06-28T23:17:39Z

cc @ctelfer this was already been approved, but can you LGTM again if your opinion did not change about it?

fcrisciani · 2018-07-03T21:15:41Z

lint failed because of no engine available but the re run passed, merging it

fcrisciani force-pushed the ipvs-sysctl branch from f22f60a to 4a37702 Compare May 18, 2018 21:26

fcrisciani force-pushed the ipvs-sysctl branch 4 times, most recently from 9d30cc8 to cb50c90 Compare May 19, 2018 00:31

ctelfer suggested changes May 21, 2018

View reviewed changes

fcrisciani force-pushed the ipvs-sysctl branch 3 times, most recently from 11cf211 to 8ebae36 Compare May 21, 2018 20:15

fcrisciani force-pushed the ipvs-sysctl branch 2 times, most recently from 3edf68c to 43e2b5c Compare May 23, 2018 21:16

ctelfer approved these changes May 24, 2018

View reviewed changes

Allows to set generic knobs on the Sandbox

449672e

Refactor the ostweaks file to allows a more easy reuse Add a method on the osl.Sandbox interface to allow setting knobs on the sandbox Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the ipvs-sysctl branch from 43e2b5c to 449672e Compare June 28, 2018 23:16

fcrisciani merged commit 007f5cf into moby:master Jul 3, 2018

fcrisciani deleted the ipvs-sysctl branch July 3, 2018 21:15

ctelfer mentioned this pull request Jul 9, 2018

Improve scalability of the Linux load balancing moby/moby#37372

Merged

thaJeztah mentioned this pull request Jul 9, 2018

[18.06] Improve scalability of the Linux load balancing docker-archive/engine#16

Merged

levesquejf mentioned this pull request Jul 26, 2018

Failed to receive UDP traffic after container restart moby/moby#8795

Closed

thaJeztah mentioned this pull request Mar 12, 2020

Reduce downtime after LB backend removal #1691

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add knobs on LB sandbox #2154

Add knobs on LB sandbox #2154

fcrisciani commented May 18, 2018

codecov-io commented May 18, 2018 •

edited

Loading

ctelfer left a comment

ctelfer May 21, 2018

fcrisciani May 21, 2018

ctelfer May 21, 2018

fcrisciani May 21, 2018

ctelfer May 21, 2018

ctelfer May 21, 2018

ctelfer May 21, 2018

ctelfer May 21, 2018

fcrisciani May 21, 2018

selansen May 21, 2018

ctelfer May 21, 2018

ctelfer commented May 21, 2018

fcrisciani commented May 21, 2018

ctelfer commented May 21, 2018

fcrisciani commented May 23, 2018

ctelfer left a comment

fcrisciani commented Jun 28, 2018

fcrisciani commented Jul 3, 2018

Add knobs on LB sandbox #2154

Add knobs on LB sandbox #2154

Conversation

fcrisciani commented May 18, 2018

codecov-io commented May 18, 2018 • edited Loading

Codecov Report

ctelfer 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

ctelfer commented May 21, 2018

fcrisciani commented May 21, 2018

ctelfer commented May 21, 2018

fcrisciani commented May 23, 2018

ctelfer left a comment

Choose a reason for hiding this comment

fcrisciani commented Jun 28, 2018

fcrisciani commented Jul 3, 2018

codecov-io commented May 18, 2018 •

edited

Loading