set cgroup parent on build child containers #14688
Conversation
|
builds on top of openshift/source-to-image#756 |
|
need to do the s2i bump here once the PR(openshift/source-to-image#756) merges, but @csrwng @derekwaynecarr @smarterclayton ptal. |
pkg/build/builder/common.go
Outdated
| @@ -130,6 +130,10 @@ func execPostCommitHook(client DockerClient, postCommitSpec api.BuildPostCommitS | |||
| if err != nil { | |||
| return fmt.Errorf("read cgroup limits: %v", err) | |||
| } | |||
| parent, err := GetCgroupParent() | |||
There was a problem hiding this comment.
Starting in 3.6, there's no case where this could fail? I.e. all systems with cgroups we'd run origin on will work?
There was a problem hiding this comment.
that seems like a question for @derekwaynecarr
if there are systems where we can read the cpu/memory(which we do require today), but can't do this to read the cgroup parent, that's something only he can probably answer.
pkg/build/builder/util.go
Outdated
|
|
||
| // GetCgroupParent determines the parent cgroup for a container from | ||
| // within that container. | ||
| func GetCgroupParent() (string, error) { |
There was a problem hiding this comment.
nit ... does this need to be public?
There was a problem hiding this comment.
no, will change. I initially had grander visions for it.
pkg/build/builder/util.go
Outdated
|
|
||
| // GetCgroupParent determines the parent cgroup for a container from | ||
| // within that container. | ||
| func GetCgroupParent() (string, error) { |
pkg/build/builder/util.go
Outdated
| // non-systemd, take everything except the last segment. | ||
| cgroupParent = strings.Join(parts[:len(parts)-1], "/") | ||
| } | ||
| glog.V(5).Infof("running docker build under cgroup parent %v", cgroupParent) |
|
[testextended][extended:core(build with a quota)] |
bparees
force-pushed
the
cgroup_parents
branch
2 times, most recently
from
8d70ec6
to
a671b39
Compare
|
unit test added. |
|
since the discussion was hidden by new commits...there's an open question here:
if there are systems where we can read the cpu/memory(which we do require today), but can't do this to read the cgroup parent, that's something only @derekwaynecarr can probably answer. I could certainly change the logic to just punt on setting the cgroup parent if we get an error...that would make it robust but leave us open to running builds w/ no cpu limits (since we're no longer explicitly setting those limits w/ this PR). |
|
Might be a good idea to have some verify on an ubuntu system
On Jun 15, 2017, at 7:16 PM, OpenShift Bot <notifications@github.com> wrote:
continuous-integration/openshift-jenkins/testextended Running (
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/636/)
(Base Commit: 0d6482d
<0d6482d>)
(PR Branch Commit: a671b39
<a671b39>)
(Extended Tests: core(build with a quota))
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14688 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p2RvusuoYy7f-8XEvvkVj0rxmrtrks5sEbssgaJpZM4N7v87>
.
|
|
I much prefer to fail closed
|
|
LGTM @sjenning - be aware of what the builds are doing when they launch their own containers in the pod level cgroup for your cpu pinning work. as i discussed with @bparees , i am not sure we can support them if a kubelet has static cpu pinning enabled as we will not know about their container to move it on/off an exclusive core. |
i figured, just throwing it out there.
I spun up an ubuntu ec2 instance and poked around. it was using systemd, but docker was not using systemd cgroups. When i looked at the /proc/self/cgroup value I saw what @derekwaynecarr had predicted and what this code looks for: values that don't end in ".scope". What i have not done is validate that the value this code extracts in that case, is indeed the correct value for the parent cgroup on that system. I can try to get this running in an ubuntu machine tomorrow if we need that level of additional validation. |
|
LGTM |
|
@bparees - it looked like your test case covered that, no?
…On Fri, Jun 16, 2017 at 10:49 AM, Cesar Wong ***@***.***> wrote:
LGTM
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14688 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF8dbJchSZy6DUps6PfZsQfeaehhp7Phks5sEpYQgaJpZM4N7v87>
.
|
@derekwaynecarr my unit test covers the parsing of the content, but i haven't actually confirmed that the value it parses out, actually works when used as the cgroup parent value (on a non-systemd system). |
|
ok I exercised this fully on ubuntu w/ [merge] |
|
Evaluated for origin testextended up to 16f5632 |
|
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 14 |
|
Evaluated for origin merge up to 16f5632 |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 16f5632 |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/641/) (Base Commit: d40783e) (PR Branch Commit: 16f5632) (Extended Tests: core(build with a quota)) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2329/) (Base Commit: d40783e) (PR Branch Commit: 16f5632) |
| // getCgroupParent determines the parent cgroup for a container from | ||
| // within that container. | ||
| func getCgroupParent() (string, error) { | ||
| cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup") |
There was a problem hiding this comment.
this function only exists on linux
$ make
hack/build-go.sh
++ Building go targets for darwin/amd64: cmd/openshift cmd/oc cmd/kubefed
# github.com/openshift/origin/pkg/build/builder
pkg/build/builder/util.go:180: undefined: cgroups.ParseCgroupFile
There was a problem hiding this comment.
well it will only run inside a linux container so that's ok but i'm not sure what we do about the cross-compile build process. I assume this isn't the first instance of something like this.
No description provided.