Bug 1907929: enable madvdontneed in system components #2299
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
rphillips
force-pushed
the
test_madvdontneed
branch
2 times, most recently
from
0df46c8 to
ab74617
Compare
|
We would also need to tweak the base image as well to set the GODEBUG: https://github.com/openshift/images/blob/515726d73cf1b35abbe0183860f5cde0aad5f387/base/Dockerfile.rhel#L23 |
rphillips
force-pushed
the
test_madvdontneed
branch
from
ab74617 to
6beb42d
Compare
|
Is the thought that we should do this OpenShift-wide? The basic tradeoff here is latency vs returning memory to the OS, right? That seems to be what https://go-review.googlesource.com/c/go/+/135395/ is claiming. One thing I'd note is at least for experimenting with this w/kubelet and crio, there's no need to patch the MCO; one could provide a |
rphillips
changed the title
openshift-ci-robot
added
bugzilla/severity-medium
|
@rphillips: This pull request references Bugzilla bug 1907929, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@cgwalters correct. The thought is that instead of getting into a memory pressure scenario is to have GO free the memory back to the kernel more aggressively. This will help in quite a few scenarios: scheduling, NodeNotReady, etc. Since, Golang 1.16 is going to default this option, it should have minimal impact to the system. This PR is Part 2. |
|
@rphillips: This pull request references Bugzilla bug 1907929, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
/test e2e-aws-serial |
|
@rphillips: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close closing for now... |
|
@rphillips: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@rphillips: This pull request references Bugzilla bug 1907929. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@rphillips: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@rphillips: This pull request references Bugzilla bug 1907929, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/invalid-bug
|
Is this EVERY component, or just openshift / rhcos system component services? |
|
This is every component. Since Golang 1.16 is going to default the option it seems reasonable to default it everywhere. |
|
Every component inside AND outside a container? Or just all system golang services not running in a container? If it's the latter, i think that's ok (we control all of them) BUT we would need to verify that teams in RHCOS that use golang are ready for it. Probably can't do the former because we don't control all the possible images that run on the system and the golang code they were compiled with (customer could have a golang app that doesn't work with this that we don't want to take from their control). In general we don't inject our logic into our customers code unless we have to to ensure they function. |
|
This option does not get passed down to inside the container... so all system golang services not running in a container is correct. Customer workloads do not see this option. |
|
Ok, then I think this is acceptable for RHCOS and OCP and OKD to enforce. |
|
/bugzilla refresh |
|
@rphillips: This pull request references Bugzilla bug 1907929, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@rphillips: This pull request references Bugzilla bug 1907929, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
rphillips
force-pushed
the
test_madvdontneed
branch
from
6beb42d to
fdf3b5b
Compare
|
This change is safer. It'll only target crio and kubelet. If other system components would like to enable it, then we can do another drop-in for that component. |
rphillips
force-pushed
the
test_madvdontneed
branch
from
fdf3b5b to
0c7541e
Compare
new changes seem to fullfil @cgwalters ask and is limited to crio/kubelet so makes sense /approve |
openshift-ci-robot
added
the
approved
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kikisdeliveryservice, mrunalp, rphillips The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rphillips: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@rphillips: All pull requests linked via external trackers have merged: Bugzilla bug 1907929 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.6 |
|
@rphillips: new pull request created: #2397 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cherry-pick release-4.6 |
|
@rphillips: new pull request created: #2632 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Golang 1.12 changed to use MADV_FREE. MADV_FREE is somewhat faster than
MADV_DONTNEED; however, the kernel will not try and reclaim memory in golang processes until the system is memory constrained. Cloud servers can potentially page out executable caches, which can cause IOPS throttles. Since golang 1.16 will be changing the default to MADV_DONTNEED, we are going to enable the flag by default.
golang/go#42330