Skip writing somaxconn when running in containers

[alexjh]

/proc/sys/net/core/somaxconn isn't writeable in some container configurations, add a config value so it can be disabled.

Modify running_in_containers() so it detects Docker containers.

See cloudfoundry/diego-release#146 for more details about the format of /proc/self/cgroup

[cfdreddbot]

Hey alexjh!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you've already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/116251345.

[Amit-PivotalLabs]

@shalako @crhino can you guys take a look at this PR?

[shalako]

@alexjh How does adding the property max_connections prevent writing to /proc/sys/net/core/somaxconn?

Thank you

[alexjh]

It should be wrapped in an if_p macro, and we would set it to null in our
config. My apologies for not checking, I only have email access right now.
I'll double check next week when I'm back in town.
On Mar 25, 2016 10:53 AM, "Shannon Coen" [email protected] wrote:

@alexjh https://github.com/alexjh How does adding the property
max_connections prevent writing to /proc/sys/net/core/somaxconn?

Thank you

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#939 (comment)

[alexjh]

Hi @shalako, in the change I'm proposing, the writing would be skipped because of the if_p here: https://github.com/cloudfoundry/cf-release/pull/939/files#diff-a1f8943b6afeff4e576766b6dd942900R49

In a situation where you don't want to write to this , you'd set router.max_connections to NULL `, see comments below.

[alexjh]

@shalako after talking to the BOSH team, I realized that using an if p would be more obvious. I've updated the PR with 366eb50.

[shalako]

Hello @alexjh

You're running gorouter in Docker containers, and echo 1024 > /proc/sys/net/core/somaxconn causes a deploy failure because the file can't be written?

This file governs max inbound connections? What happens when this file doesn't exist, or we don't overwrite it? Are max connections then unlimited (65535) or is there a kernel default? http://linux.die.net/man/2/listen suggests the default is 128. Is this acceptable when deployed in Docker?

Thank you

[alexjh]

Thanks for reviewing!

You're running gorouter in Docker containers, and echo 1024 > /proc/sys/net/core/somaxconn causes a deploy failure because the file can't be written?

Correct.

This file governs max inbound connections?

Yes.

What happens when this file doesn't exist, or we don't overwrite it? Are max connections then unlimited (65535) or is there a kernel default? http://linux.die.net/man/2/listen suggests the default is 128. Is this acceptable when deployed in Docker?

Yes, the kernel default is 128 and this will be acceptable in Docker.

[shalako]

You would like to accomplish three use cases:

1. with no action, max connections should be 1024
2. with some action, max connections should be kernel default (128)
3. with some other action, max connections should be a custom value (e.g. 65535)

We don't believe depending on NULL for 2 is a viable option. If the operator provides NULL, then the job spec default (1024) will be used. Your condition "if max_connections is present" will always be true. Have you tested this works?

How about choose another value, like -1 to trigger use case 2. -1 typically implies "unlimited" but we could put in the job spec description that when value is -1, kernel default takes precedence.

I also considered suggesting a different property that addresses your problem more directly, e.g. router.max_connections_kernel_default: true. If ever did add a max connection property, there would be complexity in these co-existing. I appreciate your thinking about solving your problem with a generically valuable feature.

[alexjh]

We don't believe depending on NULL for 2 is a viable option. If the operator provides NULL, then the job spec default (1024) will be used. Your condition "if max_connections is present" will always be true. Have you tested this works?

Ah, sorry, I misspoke earlier, the config setting would be set to false, not NULL, to skip rendering the write to somaxconn. That was before I switched from the if_p function to the if p conditional.

[shalako]

If the property were router.override_max_connections_kernel_default a value of false would make sense (default to true). Mixing booleans with integers seems odd. How about using -1?

[alexjh]

Ok, sounds good. I'll push and update the PR when I'm done testing.

[shalako]

@alexjh thank you! Could you also please:

• update the job spec description to describe the -1 behavior
• make the necessary template changes to support the property being updated using a spiff template stub

Thank you

[alexjh]

make the necessary template changes to support the property being updated using a spiff template stub

@shalako Can you elaborate on that? So far I've been testing via

./scripts/generate-bosh-lite-dev-manifest
bosh create release
bosh upload release
bosh deploy```

as described in the cf-release docs. 

I've been testing the different values (no entry, -1, 1234) in `templates/cf.yml` to see the effects in the generated files. I'm not clear on what needs to be done in the PR for the templates.

[shalako]

• create stub

  vim /tmp/cf-stub.yml
  put the following in it:

  properties:
    router:
      max_connections: -1
  

• generate manifest for boshlite, confirm the new property was added

  ./scripts/generate-bosh-lite-dev-manifest /tmp/cf-stub.yml
  vim bosh-lite/deployments/cf.yml
  

• deploy
  bosh -n deploy

• test that templates for other IaaS also support the property by generating manifests

  ./scripts/generate_deployment_manfest aws /tmp/cf-stub.yml > /tmp/cf-aws.yml
  ./scripts/generate_deployment_manifest vsphere /tmp/cf-stub.yml >  /tmp/cf-vsphere.yml
  ./scripts/generate_deployment_manifest openstack /tmp/cf-stub.yml > /tmp/cf-openstack.yml
  

To support this, I believe you have to update this section: https://github.com/cloudfoundry/cf-release/blob/release-candidate/templates/cf.yml#L975. But I'll have someone from the team provide details.

[crhino]

@shalako got it spot on really, you probably will need to update the cf.yml file where Shannon has mentioned. Following the pattern of (( merge || nil )) should be all you need to do.

[alexjh]

Thanks @shalako and @crhino, just pushed the updated.

[shalako]

@alexjh My team alerted me to issues with making this configurable. After the initial setting, a change would be ignored until the VM is restarted. As a restart only happens on a stemcell update, this will lead to unexpected behavior (your changes are not applied).

In speaking with @ematpl (PM for Diego), it seems they've already been through similar changes with you. Their solution was not to write to the proc when running in a container. There are are already dynamic mechanisms to determine this in gorouter: https://github.com/cloudfoundry/cf-release/blob/master/jobs/gorouter/templates/gorouter_ctl.erb#L20-L47

Your goal is to prevent writing this proc when in a container. You've said you'd be fine with the kernel default when running in a container. I have no product requirements to make max_connections configurable at this time, and it appears that this isn't actually as configurable as we thought.

I apologize for the back and forth, and appreciate your willingness to enhance your PR so far. We would like to change it one more time.

We prefer not to make this configurable at all. Instead, we'd like to move the logic for writing the hardcoded 1024 into the existing conditional logic that detects we're not running in a container. If running in a container, you get the kernel default (128?), otherwise you get 1024.

As you've already changed this several times at our request, we are willing to make this change for you. If instead you're willing to modify your PR again, we would certainly welcome the change.

Please let us know your preference.

Thank you

[alexjh]

@shalako No problem with the back-and-forth. I wasn't clear at the start that this only happens in a Docker container, not the bosh-lite containers (garden?). I didn't move it into the running_in_container() function because that would change the behaviour for the existing bosh-lite containers.

How about a new function that checks if /proc/sys is mounted RO?

diff --git jobs/gorouter/templates/gorouter_ctl.erb jobs/gorouter/templates/gorouter_ctl.erb
index a18e36a..04907b2 100644
--- jobs/gorouter/templates/gorouter_ctl.erb
+++ jobs/gorouter/templates/gorouter_ctl.erb
@@ -46,8 +46,10 @@ case $1 in
         echo 1 > /proc/sys/net/ipv4/tcp_tw_reuse
     fi

-    # Allow a few more queued connections than are allowed by default
-    echo 1024 > /proc/sys/net/core/somaxconn
+    if is_proc_sys_writeable; then
+      # Allow a few more queued connections than are allowed by default
+      echo 1024 > /proc/sys/net/core/somaxconn
+    fi

     # Allowed number of open file descriptors
     ulimit -n 100000
diff --git src/common/utils.sh src/common/utils.sh
index 615e668..1d1adeb 100644
--- src/common/utils.sh
+++ src/common/utils.sh
@@ -86,3 +86,7 @@ kill_and_wait() {
 running_in_container() {
   grep -q -E '/instance|/docker/' /proc/self/cgroup
 }
+
+is_proc_sys_writeable() {
+  ! { awk '$4 ~ "^ro[,$]" {print $2}' /proc/mounts | grep -q "/proc/sys"; }
+}

[shalako]

Seems reasonable, I'll move the story into our backlog for the engineering team to review.

[alexjh]

Ok, great, I pushed the changes to add is_proc_sys_writeable() to this PR.

[crhino]

@alexjh I think it would make more sense to move the setting of somaxconn value into the else block of the running_in_container conditional.

My understanding is that if /proc is mounted read-only, none of the /proc parameters will be writable, so having a totally separate conditional seems unnecessary. As you note, this would change bosh-lite behavior in regards to somaxconn, but since bosh-lite is for development and does not need any TCP tuning, I think this change would be more consistent with the other /proc/sys/net properties.

[alexjh]

@crhino Ok, sounds good. Just pushed the new version.