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

Swap Support #14950

Closed
ReillyBrogan opened this issue Jan 4, 2023 · 19 comments · Fixed by #15632
Closed

Swap Support #14950

ReillyBrogan opened this issue Jan 4, 2023 · 19 comments · Fixed by #15632
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@ReillyBrogan
Copy link
Contributor

/kind feature

1. Describe IN DETAIL the feature/behavior/change you would like to see.
Kubernetes 1.22 introduced support for enabling swap on nodes. There are many usecases that benefit from this, so it would be great if Kops supported enabling this.

Unfortunately while it appears that the NodeSwap feature gate and failSwapOn: false work as expected it appears that there is no way to modify the MemorySwapConfiguration in the kubelet config to change the default setting from LimitedSwap to UnlimitedSwap. This means that it is still not possible to actually use swap for Kubernetes workloads, only that it can be used for non-kubernetes processes running on the host.

2. Feel free to provide a design supporting your feature request.
Perhaps kubelet and masterKubelet could be extended with the following:

kubelet:
  memorySwapConfiguration:
    swapBehavior: "", "LimitedSwap", or "UnlimitedSwap"

Reference:

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 4, 2023
@olemarkus
Copy link
Member

This sounds like a good feature to support and an easy thing to add. Happy to review a PR.

@tomesink
Copy link
Contributor

tomesink commented Jan 24, 2023

Hi @olemarkus I am a returning newbie in Kops project (2 PRs 3 years ago :)) looking for a refresher task. This one looks promising. Before I submit the PR I just want to make sure my understanding is correct. The following is my idea:

I'd modify pkg/api/kops/componentconfig.go and pkg/apis/kops/v1alpha2 by adding the following struct:

// MemorySwapConfiguration is used to configure the swap behavior of a node
type MemorySwapConfiguration struct {
	// SwapBehavior is used to specify the swap behavior of the node.
	// It can be set to "", "LimitedSwap", or "UnlimitedSwap"
	SwapBehavior string `json:"swapBehavior,omitempty"`
}

And than simply add the option into KubeletConfigSpec struct:

// KubeletConfigSpec defines the kubelet configuration
type KubeletConfigSpec struct {
	....
	// MemorySwapConfiguration is used to configure the swap behavior of a node
	MemorySwapConfiguration *MemorySwapConfiguration `json:"memorySwapConfiguration,omitempty"`
}

What do you think? Are my thoughts going in the right direction?

@olemarkus
Copy link
Member

LimitedSwap probably doesn't make much sense for kOps since we always use cgroupv2.
You also need to pass this info into the kubelet component config that's built by nodeup.

@johngmyers
Copy link
Member

You'll also need to add it to v1alpha3.

@johngmyers
Copy link
Member

The field should be a custom type type SwapBehavior string. You should add API validation for it. You should document the default value in the field comment. There's no need to document the ability to leave it unset.

@tomesink
Copy link
Contributor

we always use cgroupv2.

What is the point of implementing this feature than? If kOps always uses cgroups v2, it means that the memory swap feature is not supported. Therefore, implementing the MemorySwapConfiguration would not have any effect on the behavior of kOps clusters and would not provide any additional functionality. So, IMHO it would be redundant and unnecessary to add this feature. Or am I missing something? :)

Why setting UnlimitedSwap as default option if memory swap is not supported at all?

@ReillyBrogan
Copy link
Contributor Author

Why setting UnlimitedSwap as default option if memory swap is not supported at all?

I'm not sure where you're getting this but it's incorrect. Memory swap is supported with cgroups v2 however the only supported options are "" where swap is not allowed for Kubernetes workloads but is otherwise unconstrained (IE system daemons and other services are allowed to use swap) or UnlimitedSwap which allows Kubernetes workloads to use swap.

@tomesink
Copy link
Contributor

I'm not sure where you're getting this but it's incorrect.

I deduced it from the reference doc :)

UnlimitedSwap: Kubernetes workloads can use as much swap memory as they request, up to the system limit.

vs

cgroups v2: Kubernetes workloads cannot use swap memory.

@ReillyBrogan
Copy link
Contributor Author

I'm not sure where you're getting this but it's incorrect.

I deduced it from the reference doc :)

cgroups v2: Kubernetes workloads cannot use swap memory.

That section is only for the behavior of LimitedSwap. There is no such restriction for UnlimitedSwap.

@tomesink
Copy link
Contributor

@ReillyBrogan got it. One more thing, the default value should be "" or "UnlimitedSwap"?

@ReillyBrogan
Copy link
Contributor Author

The default if a user does not set memorySwapConfiguration.swapBehavior should be that the config option should not be present which will result in whatever the Kubernetes default is taking effect. If upstream Kubernetes changes the default at some point then that new default will take effect without any kops users needing to update their configuration or for kops to need to change any code.

The rough logic should look like this:
memorySwapConfiguration.swapBehavior is not set:

  • No change to kubelet configuration from what it is currently happening (currently the line memorySwap: {} is added to the kubelet config)

memorySwapConfiguration.swapBehavior: "":

  • kubelet config should contain memorySwap: ""

memorySwapConfiguration.swapBehavior: UnlimitedSwap:

  • kubelet config should contain memorySwap: UnlimitedSwap

I'm not sure if setting UnlimitedSwap should imply featureGates.NodeSwap = "true" and failSwapOn: false (both of which are required for swap to work currently). @olemarkus thoughts?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2023
@jabdoa2
Copy link

jabdoa2 commented Apr 26, 2023

/remove-lifecycle stale

We would like this option as well. Makes a lot of sense to have it in kops.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2023
@olemarkus
Copy link
Member

As mentioned above, we're happy to review a PR.

@tomesink
Copy link
Contributor

/assign

Omg, I totally shadowed this! Sorry. PR is on a way.

@tomesink
Copy link
Contributor

Regarding setting featureGates.NodeSwap = "true" and failSwapOn: false when UnlimitedSwap is specified, it's generally better to keep these configurations separate.

Users should explicitly enable the NodeSwap feature gate and set failSwapOn: false in their Kops cluster configuration to use swap memory for Kubernetes workloads. This approach ensures that users are aware of the alpha feature and its implications and makes it easier to manage feature gate lifecycle independently.

@jan-kantert
Copy link

@tomesm any update? can we help/test something?

@tomesink
Copy link
Contributor

tomesink commented Jun 15, 2023

@jan-kantert > @tomesm any update? can we help/test something?

I have opened the PR 2 months ago and have been waiting for any feedback since than. Is there anything more I can do?

@jan-kantert
Copy link

@olemarkus are you still available to review the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants