temporary PR to help investigate ZONE_NORMAL #959
Closed
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
sharnoff
force-pushed
the
sharnoff/tmp-investigate-ZONE_NORMAL
branch
from
4ed5647 to
dcbef79
Compare
sharnoff
added a commit
that referenced
this pull request
Jun 18, 2024
Adapted from #705. This implementation is focused on making virtio-mem a drop-in replacement for our existing DIMM hotplug -based memory scaling. As such, the externally visible differences are: 1. VirtualMachines have a new optional field: .spec.guest.memoryProvider 2. VirtualMachines have a new optional field: .status.memoryProvider 3. VirtualMachine .status.memorySize changes in smaller increments if using virtio-mem. This change introduces the concept of a "memory provider", which is either "DIMMSlots" or "VirtioMem". If a VM is created without setting the memory provider in the spec, the controller will use the default one, which is specified by its --default-memory-provider flag. This will only persist for the lifetime of the runner pod. If the VM is restarted, then it may change to a new default. Additionally, because our usage of virtio-mem has 8MiB block sizes, we actually *don't* default to virtio-mem if the VM was created with some odd memory slot size that isn't divisible by 8MiB. The memory provider of the current runner pod is stored in the new status field. For old VMs created before this change, the status field is set to DIMMSlots iff the pod name is set without the status field. Also, we allow mutating .spec.guest.memoryProvider IFF it is being set equal to the current .status.memoryProvider, in case we we want old VMs to keep their dimm slots after switching to virtio-mem by default. --- Changes to split out from this PR: 1. Requiring that .spec.guest.{cpus,memorySlots}.{min,max} are always non-nil (we already did in some places, and we have a defaulter, and the control plane always sets them anyways!) 2. Move setting .status.podName so it only happens when phase=Pending. This was required for setting .status.memoryProvider properly, but it should also fix #967. 3. Emitting "VM uses ... memory/CPU" events on every change, not just when reconciling is done. On top of those, there's a couple places where code was indented that should probably be split into its own commit so it can be properly added to .git-blame-ignore-revs :) --- Remaining work on this PR before merging: 1. Test that backwards compatibility works as expected 1. Old VMs are updated while running as described above 2. Old VMs switch to the new default on restart 2. Test rollback safety: that it's ok to go between the last release and this PR (with --default-memory-provider=DIMMSlots). - Note: If --default-memory-provider=VirtioMem when this is released, we cannot make it rollback-safe. 3. Test that updating .spec.guest.memoryProvider works as expected --- Future/follow-up work: 1. Add support for automatic MOVABLE:NORMAL zone ratios - Design work needed. Ideally would be customizable. What to do about VMs using DIMMs? - See #959 - Should fix #117. 2. Switch to virtio-mem by default 3. Remove support for DIMM hotplug 4. Move from representing memory as "memory slots" towards the implementation 5. Fix the raciness issue with migration target pod creation. The implementation here focuses on making virtio-mem a drop-in replacement for our existing memory scaling. Co-authored-by: Andrey Taranik <[email protected]>
5 tasks
sharnoff
force-pushed
the
sharnoff/tmp-investigate-ZONE_NORMAL
branch
from
bd1c806 to
98396c1
Compare
|
Rebased the |
This reverts commit 43751cb.
This implementation is focused on making virtio-mem a drop-in replacement for our existing DIMM hotplug -based memory scaling. As such, the externally visible differences are: 1. VirtualMachines have a new optional field: .spec.guest.memoryProvider 2. VirtualMachines have a new optional field: .status.memoryProvider 3. VirtualMachine .status.memorySize changes in smaller increments if using virtio-mem. This change introduces the concept of a "memory provider", which is either "DIMMSlots" or "VirtioMem". If a VM is created without setting the memory provider in the spec, the controller will use the default one, which is specified by its --default-memory-provider flag. This will only persist for the lifetime of the runner pod. If the VM is restarted, then it may change to a new default. Additionally, because our usage of virtio-mem has 8MiB block sizes, we actually *don't* default to virtio-mem if the VM was created with some odd memory slot size that isn't divisible by 8MiB. The memory provider of the current runner pod is stored in the new status field. For old VMs created before this change, the status field is set to DIMMSlots iff the pod name is set without the status field. Also, we allow mutating .spec.guest.memoryProvider IFF it is being set equal to the current .status.memoryProvider, in case we we want old VMs to keep their dimm slots after switching to virtio-mem by default. Co-authored-by: Andrey Taranik <[email protected]>
sharnoff
force-pushed
the
sharnoff/tmp-investigate-ZONE_NORMAL
branch
from
e7f7ec1 to
d5798bd
Compare
sharnoff
added a commit
that referenced
this pull request
Jun 20, 2024
In short: Currently we hotplug all memory as ZONE_MOVABLE. This has known issues because there are theoretical and practical limits on the ratio between ZONE_MOVABLE and ZONE_NORMAL. With 'memory_hotplug.online_policy=auto-movable', we can use the config value 'memory_hotplug.auto_movable_ratio' to set a ratio of MOVABLE:NORMAL to uphold during hotplug and the kernel will Do The Right Thing™. This change adds a new flag to neonvm-controller and neonvm-runner to set the value of this ratio: '-memhp-auto-movable-ratio'. However, please note: The new behavior is only available for VMs using virito-mem because memory_hotplug.auto_movable_ratio does not count hotplugged ZONE_NORMAL DIMM slot towards the ratio, so IMO it's not worth changing the existing behavior. [1] [2] [3] This is the productionized version of the experimentation from #959. For more on that, see [4]. [1]: https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#module-parameters [2]: https://lwn.net/Articles/865618/ [3]: https://neondb.slack.com/archives/C03TN5G758R/p1718225978299459?thread_ts=1718158200.324759 [4]: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89
sharnoff
added a commit
that referenced
this pull request
Jun 22, 2024
…981) In short: Currently we hotplug all memory as ZONE_MOVABLE. This has known issues because there are theoretical and practical limits on the ratio between ZONE_MOVABLE and ZONE_NORMAL. With 'memory_hotplug.online_policy=auto-movable', we can use the config value 'memory_hotplug.auto_movable_ratio' to set a ratio of MOVABLE:NORMAL to uphold during hotplug and the kernel will Do The Right Thing™. This change adds a new flag to neonvm-controller and neonvm-runner to set the value of this ratio: '-memhp-auto-movable-ratio'. However, please note: The new behavior is only available for VMs using virito-mem because memory_hotplug.auto_movable_ratio does not count hotplugged ZONE_NORMAL DIMM slot towards the ratio, so IMO it's not worth changing the existing behavior. [1] [2] [3] This is the productionized version of the experimentation from #959. For more on that, see [4]. [1]: https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#module-parameters [2]: https://lwn.net/Articles/865618/ [3]: https://neondb.slack.com/archives/C03TN5G758R/p1718225978299459?thread_ts=1718158200.324759 [4]: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89
sharnoff
added a commit
that referenced
this pull request
Jun 22, 2024
…981) In short: Currently we hotplug all memory as ZONE_MOVABLE. This has known issues because there are theoretical and practical limits on the ratio between ZONE_MOVABLE and ZONE_NORMAL. With 'memory_hotplug.online_policy=auto-movable', we can use the config value 'memory_hotplug.auto_movable_ratio' to set a ratio of MOVABLE:NORMAL to uphold during hotplug and the kernel will Do The Right Thing™. This change adds a new flag to neonvm-controller and neonvm-runner to set the value of this ratio: '-memhp-auto-movable-ratio'. However, please note: The new behavior is only available for VMs using virito-mem because memory_hotplug.auto_movable_ratio does not count hotplugged ZONE_NORMAL DIMM slot towards the ratio, so IMO it's not worth changing the existing behavior. [1] [2] [3] This is the productionized version of the experimentation from #959. For more on that, see [4]. [1]: https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#module-parameters [2]: https://lwn.net/Articles/865618/ [3]: https://neondb.slack.com/archives/C03TN5G758R/p1718225978299459?thread_ts=1718158200.324759 [4]: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89
sharnoff
added a commit
that referenced
this pull request
Jun 22, 2024
…981) In short: Currently we hotplug all memory as ZONE_MOVABLE. This has known issues because there are theoretical and practical limits on the ratio between ZONE_MOVABLE and ZONE_NORMAL. With 'memory_hotplug.online_policy=auto-movable', we can use the config value 'memory_hotplug.auto_movable_ratio' to set a ratio of MOVABLE:NORMAL to uphold during hotplug and the kernel will Do The Right Thing™. This change adds a new flag to neonvm-controller and neonvm-runner to set the value of this ratio: '-memhp-auto-movable-ratio'. However, please note: The new behavior is only available for VMs using virito-mem because memory_hotplug.auto_movable_ratio does not count hotplugged ZONE_NORMAL DIMM slot towards the ratio, so IMO it's not worth changing the existing behavior. [1] [2] [3] This is the productionized version of the experimentation from #959. For more on that, see [4]. [1]: https://docs.kernel.org/admin-guide/mm/memory-hotplug.html#module-parameters [2]: https://lwn.net/Articles/865618/ [3]: https://neondb.slack.com/archives/C03TN5G758R/p1718225978299459?thread_ts=1718158200.324759 [4]: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89
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.
It's more convenient to have CI build & push the images than do it myself
ref: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89?pvs=4#dcf1ea2e65464f618db917b77e3dcf42
ref: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89?pvs=4#8df8a3aa3cf64ecb8aa68266913b7182
ref: https://www.notion.so/neondatabase/59c9e9b2619e4ccd8fbb99e9b0cb1a89?pvs=4#3fec5babb1474eeebffe72619728ea68