[KEP-2400] add no swap as the default option for swap #122745
Conversation
|
/sig node |
|
/hold |
|
/test |
|
@kannon92: The
The following commands are available to trigger optional jobs:
Use
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. |
|
/test pull-kubernetes-node-swap-fedora |
dabdf66 to
ad437a5
Compare
|
/test pull-kubernetes-node-swap-fedora |
ad437a5 to
2eb61e6
Compare
c1eae5f to
b752bef
Compare
Delete this commit once PR kubernetes#122745 perges. This reverts commit 159cd9a. Signed-off-by: Itamar Holder <[email protected]>
Delete this commit once PR kubernetes#122745 perges. This reverts commit 159cd9a. Signed-off-by: Itamar Holder <[email protected]>
| @@ -168,6 +168,13 @@ func (m *kubeGenericRuntimeManager) configureContainerSwapResources(lcr *runtime | |||
| return | |||
There was a problem hiding this comment.
I would not expect to log about this on every container setup, just once
There was a problem hiding this comment.
pushed up a fix for this.
There was a problem hiding this comment.
it still looks like we'll be logging klog.InfoS("No swap cgroup controller present" for every container for the whole kubelet lifetime... that seems like log spam, right?
There was a problem hiding this comment.
The function that is called says it should only be called once.
// Note: this function variable is being added here so it would be possible to mock
// the swap controller availability for unit tests by assigning a new function to it. Without it,
// the swap controller availability would solely depend on the environment running the test.
var swapControllerAvailable = func() bool {
// See https://github.com/containerd/containerd/pull/7838/
swapControllerAvailabilityOnce.Do(func() {
const warn = "Failed to detect the availability of the swap controller, assuming not available"
p := "/sys/fs/cgroup/memory/memory.memsw.limit_in_bytes"
if isCgroup2UnifiedMode() {
// memory.swap.max does not exist in the cgroup root, so we check /sys/fs/cgroup/<SELF>/memory.swap.max
_, unified, err := cgroups.ParseCgroupFileUnified("/proc/self/cgroup")
if err != nil {
klog.V(5).ErrorS(fmt.Errorf("failed to parse /proc/self/cgroup: %w", err), warn)
return
}
p = filepath.Join("/sys/fs/cgroup", unified, "memory.swap.max")
}
if _, err := os.Stat(p); err != nil {
if !errors.Is(err, os.ErrNotExist) {
klog.V(5).ErrorS(err, warn)
}
return
}
swapControllerAvailability = true
})
return swapControllerAvailability
}
There was a problem hiding this comment.
It isn't clear to me how one gets in this state.
We would only log this if /sys/fs/cgroup/memory/memory.memsw.limit_in_bytes or the cgroupv2 location is missing. And then we would log and say that this isn't specified.
It seems to be a special case and not really a case for log spam.
There was a problem hiding this comment.
huh, I assumed configureContainerSwapResources was only called when NodeSwap feature was enabled, and defaulting it on in this PR would increase log spam, but it looks like it has already been called unconditionally, so any log spam already exists.
Given that, I'm ok with trying to mitigate the log spam in a follow-up PR (though I think that should definitely be done). We probably want to output the message once at startup very visibly.
A follow-up PR to relocate this log message to startup seems fine to me.
There was a problem hiding this comment.
@harche @iholder101 Please follow up on this one.
|
#122745 (comment) is my only outstanding comment, the rest of the changes look reasonable to me |
|
/approve /hold for last-review of the impl changes (relocating logs, tweak to validation, etc) by @mrunalp |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kannon92, liggitt, mrunalp 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 |
e2ebe94 to
6a4e19a
Compare
|
/lgtm |
|
LGTM label has been added. Git tree hash: 9a4311d640054216e970746b8793c3d9ee772b87
|
|
/test pull-kubernetes-e2e-kind-ipv6 |
| @@ -803,6 +804,14 @@ func run(ctx context.Context, s *options.KubeletServer, kubeDeps *kubelet.Depend | |||
| return fmt.Errorf("topology manager policy options %v require feature gates %q enabled", | |||
| s.TopologyManagerPolicyOptions, features.TopologyManagerPolicyOptions) | |||
| } | |||
| if utilfeature.DefaultFeatureGate.Enabled(features.NodeSwap) { | |||
| if !isCgroup2UnifiedMode() && s.MemorySwap.SwapBehavior == kubelettypes.LimitedSwap { | |||
There was a problem hiding this comment.
Shouldn't we return an error since this is not supported? What do we want to let kubelet continue running for an unsupported combination?
There was a problem hiding this comment.
Thats a good point. We never failed in the cases around this swap feature so it has become difficult to enforce that from this point.
We had an issue to fail cgroupv1 and this feature but then we discovered that people were using --fail-swap-on=false with cgroupv1. So I may have been more pragmatic here than I needed.
There was a problem hiding this comment.
Users will need to explicitly specify LimitedSwap in this case, so failing cleanly may be a better experience
There was a problem hiding this comment.
I think this PR is minutes away from merging so can i do a minor follow up?
There was a problem hiding this comment.
follow up to fail fast at startup when explicitly configured to use limited swap on a machine where it cannot sgtm
There was a problem hiding this comment.
This PR is taking its sweet time to merge..
Its like watching water boil.
I have #123738 as a quick follow up once this merges. I'll check and rebase accordingly when this merges.
There was a problem hiding this comment.
#123738 is up now and rebased.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implementation of beta2 findings for KEP-2400.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Waiting on KEP PR.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: