Skip to content
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

running_in_container function in gorouter_ctl does not work properly #138

Closed
sravankumar777 opened this issue Feb 26, 2019 · 12 comments
Closed
Labels

Comments

@sravankumar777
Copy link

@sravankumar777 sravankumar777 commented Feb 26, 2019

Thanks for submitting an issue to routing-release. We are always trying to improve! To help us, please fill out the following template.

Issue

While gorouter_ctl is initiated/started, gorouter_ctl does not configure the /proc/sys/net/ipv4 parameters.

Context

Performing stemcell version upgrade on platform.

Current using Stemcell Version: Ubuntu Trusty v3586.60
Plan to update Stemcell Version: Ubuntu Xenial v170.25

Steps to Reproduce

  1. Routing-release version used: 0.170.0
  2. Stop gorouter_ctl using sudo /var/vcap/jobs/gorouter/bin/gorouter_ctl stop
  3. Start gorouter_ctl using sudo /var/vcap/jobs/gorouter/bin/gorouter_ctl start

With latest stemcell version v170.25,

cat /var/vcap/bosh/etc/stemcell_version
170.25
$ source /var/vcap/packages/routing_utils/pid_utils.sh
$ running_in_container && echo "IN CONTAINER" || echo "NOT IN CONTAINER"
IN CONTAINER

# grep --quiet --invert-match ':/$' /proc/self/cgroup; echo $?
0

With previous stemcell version v3586.60

cat /var/vcap/bosh/etc/stemcell_version
3586.60

$ source /var/vcap/packages/routing_utils/pid_utils.sh

running_in_container && echo "IN CONTAINER" || echo "NOT IN CONTAINER"
NOT IN CONTAINER

grep --quiet --invert-match ':/$' /proc/self/cgroup; echo $?
1

Expected result

Function running_in_container defined in pid_utils should provide correct output based on the running of process.
grep --quiet --invert-match ':/$' /proc/self/cgroup should be properly checking about the /proc/self/cgroup services.

Current result

After updating the Ubuntu Xenial stemcell, running_container function always returns that process is running inside a container.

$ sudo /var/vcap/jobs/gorouter/bin/gorouter_ctl start
------------ STARTING gorouter_ctl at Tue Feb 26 00:45:34 UTC 2019 --------------
Not setting /proc/sys/net/ipv4 parameters, since I'm running inside a linux container

Possible Fix

grep --quiet --invert-match ':/$' /proc/self/cgroup should be handled properly.

@cf-gitbot

This comment has been minimized.

Copy link
Collaborator

@cf-gitbot cf-gitbot commented Feb 26, 2019

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/164231927

The labels on this github issue will be updated when the story is started.

@giner

This comment has been minimized.

Copy link
Contributor

@giner giner commented Feb 26, 2019

The following should work on the stemcells with systemd:

running_in_container() {
    grep --quiet --invert-match ':/\(init\.scope\|\)$' /proc/1/cgroup
}
@emalm

This comment has been minimized.

Copy link
Member

@emalm emalm commented Feb 26, 2019

Couple of notes:

  • The Diego team encountered similar issues with diego-release jobs when we adopted Xenial stemcells with systemd, and decided simply to make the operator specify in the deployment manifest whether to adjust kernel parameters. For example, https://github.com/cloudfoundry/diego-release/blob/v2.28.0/jobs/rep/spec#L321-L323.
  • Depending on the specific kernel parameters, they might also be namespaced in the Linux kernel, in which case it would be safe to alter them if the job is running in its own network namespace (as it would be in a BOSH-Lite "VM" container). This namespacing does change over time, so the 4.15 kernels in the current Xenial stemcells namespace more than the 4.4 kernels in the Trusty stemcells. (Apologies that I don't have a reference for the parameter namespacing off-hand.)
@giner

This comment has been minimized.

Copy link
Contributor

@giner giner commented Feb 26, 2019

@emalm, good point. Though this approach still sounds quite error-prone. Multiple jobs may potentially change same parameters. Also some jobs change kernel parameters in ctl script instead of pre-start which leads to race condition with other jobs. Wouldn't it be feasible to define kernel settings per instance group using os-conf-release in cf-deployment?

@emalm

This comment has been minimized.

Copy link
Member

@emalm emalm commented Feb 26, 2019

Yeah, in general I think that the adjustment of kernel parameters in a BOSH deployment should be done on a per-instance-group basis because of the VM-wide effects, likely explicitly in the deployment manifest, and the os-conf-release overall can do that.

Even then, there are some subtleties: for example, https://github.com/cloudfoundry/diego-release/blob/v2.28.0/jobs/rep/templates/set-rep-kernel-params.erb#L21 sets /proc/sys/net/netfilter/nf_conntrack_max, but that conntrack parameter is not even available to set in the kernel until the nf_conntrack kernel module is loaded, which in CF happens implicitly when some other job (garden or a container-networking system such as Silk) runs iptables. (We've seen this in practice when garden's container-networking integration has been misconfigured, so that iptables did not actually run, and so the rep job failed to start at that line.)

@giner

This comment has been minimized.

Copy link
Contributor

@giner giner commented Feb 26, 2019

Those modules should be loaded explicitly then. Another way of doing this could be a job (may be a feature to os-conf-release) which would scan /var/vcap/jobs/*/modules/*.conf and load all modules, then would scan /var/vcap/jobs/*/sysctl/*.conf, check for duplicates if any and apply the settings.

It feels like we are reinventing yet another init system btw.

@emalm

This comment has been minimized.

Copy link
Member

@emalm emalm commented Feb 26, 2019

/cc @mfine30 @jfmyers9 re os-conf-release gaps

@mfine30

This comment has been minimized.

Copy link
Member

@mfine30 mfine30 commented Feb 26, 2019

@luan (not sure Mukesh's handle), this seems to fall in your domain?

@giner

This comment has been minimized.

Copy link
Contributor

@giner giner commented May 5, 2019

@mfine30

This comment has been minimized.

Copy link
Member

@mfine30 mfine30 commented May 6, 2019

@mgadiya @aashah PM/anchor respectively, are on the Bosh Systems team, which owns that release

@bruce-ricard

This comment has been minimized.

Copy link
Contributor

@bruce-ricard bruce-ricard commented May 6, 2019

@sravankumar777 Thank you for the very clear and descriptive issue.

We'll let the BOSH and Diego people deal with this, since they seem to have a good grasp of the problem.

@ameowlia and @bruce-ricard , members of the CF Networking Program.

@barakyo

This comment has been minimized.

Copy link
Contributor

@barakyo barakyo commented May 20, 2019

Hi @sravankumar777,

Given that it may take time for cloudfoundry/os-conf-release#47 to be merged in, we decided to follow @emalm's suggest of making this configurable through the spec. We've addressed this in e0e13e6

Thanks!
@rodolfo2488 and @barakyo, members of the CF Networking Program

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.