Allows ignoring memory modules #98
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
September 23, 2019 13:26
Some hosts may have disabled some memory modules (e.g. `memsw` metrics
are not available if the Kernel is built without the
`CONFIG_MEMCG_SWAP_ENABLED` variable. This made the memory controller
`Stat` function to not work properly, either because it returns error
when a single memory entry is not available.
Even if you run `cgroups.Stat(cgroups.IgnoreNotExist)` to ignore memory
Stat errors, you won't get most of the memory metrics.
This patch adds a configuration function parameter to the `NewMemory`
constructor. This way, we don't introduce breaking changes in the
current API. You can invoke the `NewMemory` constructor as usual or
with a new optional configuration option:
```go
mem := NewMemory(root, IgnoreModules("memsw"))
```
Signed-off-by: Mario Macias <[email protected]>
Signed-off-by: Mario Macias <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
==========================================
+ Coverage 41.76% 45.01% +3.24%
==========================================
Files 23 23
Lines 1616 1626 +10
==========================================
+ Hits 675 732 +57
+ Misses 817 768 -49
- Partials 124 126 +2
Continue to review full report at Codecov.
|
|
Could you make the Ignore more dynamic by checking for the memsw file as an Opt? You can keep the current IgnoreModule the same but add another opt that can be used in containerd that will add the ignore only if a stat() on memsw fails? |
|
Thanks for the quick feedback @crosbymichael . I submitted a new patch with the Feel free to suggest any other name. |
Signed-off-by: Mario Macias <[email protected]>
|
LGTM Thanks @mariomac !! |
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Oct 31, 2019
full diff: containerd/cgroups@c4b9ac5...5fbad35 - containerd/cgroups#82 Add go module support - containerd/cgroups#96 Move metrics proto package to stats/v1 - containerd/cgroups#97 Allow overriding the default /proc folder in blkioController - containerd/cgroups#98 Allows ignoring memory modules - containerd/cgroups#99 Add Go 1.13 to Travis - containerd/cgroups#100 stats/v1: export per-cgroup stats Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Oct 31, 2019
full diff: containerd/cgroups@c4b9ac5...5fbad35 - containerd/cgroups#82 Add go module support - containerd/cgroups#96 Move metrics proto package to stats/v1 - containerd/cgroups#97 Allow overriding the default /proc folder in blkioController - containerd/cgroups#98 Allows ignoring memory modules - containerd/cgroups#99 Add Go 1.13 to Travis - containerd/cgroups#100 stats/v1: export per-cgroup stats Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 27552ceb15bca544820229e574427d4c1d6ef585 Component: engine
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Dec 3, 2019
full diff: containerd/cgroups@c4b9ac5...5fbad35 - containerd/cgroups#82 Add go module support - containerd/cgroups#96 Move metrics proto package to stats/v1 - containerd/cgroups#97 Allow overriding the default /proc folder in blkioController - containerd/cgroups#98 Allows ignoring memory modules - containerd/cgroups#99 Add Go 1.13 to Travis - containerd/cgroups#100 stats/v1: export per-cgroup stats Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit 27552ce) Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jan 23, 2020
full diff: containerd/cgroups@c4b9ac5...5fbad35 - containerd/cgroups#82 Add go module support - containerd/cgroups#96 Move metrics proto package to stats/v1 - containerd/cgroups#97 Allow overriding the default /proc folder in blkioController - containerd/cgroups#98 Allows ignoring memory modules - containerd/cgroups#99 Add Go 1.13 to Travis - containerd/cgroups#100 stats/v1: export per-cgroup stats Signed-off-by: Sebastiaan van Stijn <[email protected]> (cherry picked from commit 27552ceb15bca544820229e574427d4c1d6ef585) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: 9ab162a73ac9e133a21cffbadd3339cbb5213939 Component: engine
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some hosts may have disabled some memory modules (e.g.
memswmetricsare not available if the Kernel is built without the
CONFIG_MEMCG_SWAP_ENABLEDvariable. This made the memory controllerStatfunction to not work properly, because it returns errorwhen a single memory entry is not available.
Even if you run
cgroups.Stat(cgroups.IgnoreNotExist)to ignore memoryStat errors, you won't get most of the memory metrics.
This patch adds a configuration function parameter to the
NewMemoryconstructor. This way, we don't introduce breaking changes in the
current API. You can invoke the
NewMemoryconstructor as usual orwith a new optional configuration option:
If you want to let the constructor to automatically ignore
memswmoduleonly if it is not available:
Signed-off-by: Mario Macias [email protected]