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

fix: disable swap for executor_linux allocations #3958

Merged
merged 2 commits into from
Mar 13, 2018
Merged

fix: disable swap for executor_linux allocations #3958

merged 2 commits into from
Mar 13, 2018

Conversation

fho
Copy link
Contributor

@fho fho commented Mar 9, 2018

fix: disable swap for executor_linux allocations

A comment in the nomad source code states that swapping for
executor_linux allocations is disabled but it wasn't.

Nomad wrote -1 to the memsw.limit_in_bytes cgroup file to disable
swapping.
This has the following problems:

1.) Writing -1 to the file does not disable swapping. It sets
    the limit for memory and swap to unlimited.
2.) On common Linux distributions like Ubuntu 16.04 LTS the
    memsw.limit_in_bytes cgroup file does not exist by default.
    The memsw.limit_in_bytes file only exist if the Linux kernel is
    build with CONFIG_MEMCG_SWAP=yes and either
    CONFIG_MEMCG_SWAP_ENABLED=yes or when the kernel parameter
    swapaccount=1 is passed during boot.
    Most Linux distributions disable swap accounting by default because
    of higher memory usage.
    Nomad silently ignores if writing to the memsw.limit_in_bytes file
    fails. The allocation succeeds, no message is logged to notify the
    user.

To ensure that disabling swap works on common Linux kernels, disable
swapping by writing 0 to the memory.swappiness file.
Using the memory.swappiness file only requires that the kernel is
compiled with CONFIG_MEMCG=yes. This is the default in common Linux
kernels.

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @fho! I was unaware the swappiness cgroup parameter differed in behavior from the global procfs swappiness parameter!

For the record the swappiness cgroup parameter actually disables swap when set to 0 unlike the global procfs parameter:

Please note that unlike during the global reclaim, limit reclaim
enforces that 0 swappiness really prevents from any swapping even if
there is a swap storage available.

https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt

@@ -83,7 +83,8 @@ func (e *UniversalExecutor) configureCgroups(resources *structs.Resources) error
// Total amount of memory allowed to consume
e.resConCtx.groups.Resources.Memory = int64(resources.MemoryMB * 1024 * 1024)
// Disable swap to avoid issues on the machine
e.resConCtx.groups.Resources.MemorySwap = int64(-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to remove this? Is it possible that in some rare situations it may work while the swappiness approach may not?

Copy link
Contributor Author

@fho fho Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to remove this?

Yes

1.) Writing -1 to the memsw.limit_in_bytes file does not disable swap. It sets the limit to unlimited.

NOTE: We can write "-1" to reset the *.limit_in_bytes(unlimited).

https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt
I'll update my commit message accordingly.

2.) It's sufficient to disable swapping via a single mechanism.


Is it possible that in some rare situations it may work while the swappiness approach may not?

Not that I'm aware off. The swappiness value can be set when
CONFIG_MEMCG is enabled and the cggroup memory fs is mounted.
Both are also preconditions for enabling CONFIG_MEMCG_SWAP_ENABLED.

A comment in the nomad source code states that swapping for
executor_linux allocations is disabled but it wasn't.

Nomad wrote -1 to the memsw.limit_in_bytes cgroup file to disable
swapping.
This has the following problems:

1.) Writing -1 to the file does not disable swapping. It sets
    the limit for memory and swap to unlimited.
2.) On common Linux distributions like Ubuntu 16.04 LTS the
    memsw.limit_in_bytes cgroup file does not exist by default.
    The memsw.limit_in_bytes file only exist if the Linux kernel is
    build with CONFIG_MEMCG_SWAP=yes and either
    CONFIG_MEMCG_SWAP_ENABLED=yes or when the kernel parameter
    swapaccount=1 is passed during boot.
    Most Linux distributions disable swap accounting by default because
    of higher memory usage.
    Nomad silently ignores if writing to the memsw.limit_in_bytes file
    fails. The allocation succeeds, no message is logged to notify the
    user.

To ensure that disabling swap works on common Linux kernels, disable
swapping by writing 0 to the memory.swappiness file.
Using the memory.swappiness file only requires that the kernel is
compiled with CONFIG_MEMCG=yes. This is the default in common Linux
kernels.
@fho fho changed the title set swappiness instead memsw.limit_in_bytes to disable swapping fix: disable swap for executor_linux allocations Mar 13, 2018
@schmichael schmichael merged commit a90ce65 into hashicorp:master Mar 13, 2018
@schmichael
Copy link
Member

Thanks again for the excellent fix @fho and working through the details with me!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants