Allow for container fs and image fs to be on the same drive but in a different partition #128344
Conversation
k8s-ci-robot
added
size/L
containerfs are on the same drive but different path
kannon92
force-pushed
the
revert-128333-eviction-manager
branch
from
943caa9 to
742ca47
Compare
k8s-ci-robot
added
release-note
|
/kind bug |
k8s-ci-robot
added
kind/bug
|
/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-sigs/prow repository. |
|
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-eviction |
|
/cc @AnishShah |
|
/lgtm ref: #126559 |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 1c23910a66b96dc412d6d8e028b61b6b16d89d0c
|
|
Eviction looks are much better with this. /assign @SergeyKanzhelev |
|
/assign @yujuhong |
|
/priority important-longterm |
k8s-ci-robot
added
priority/important-longterm
| splitContainerImageFs := m.containerGC.IsContainerFsSeparateFromImageFs(ctx) | ||
| splitContainerImageFs, splitErr := diskInfoProvider.HasDedicatedContainerFs(ctx) | ||
| if splitErr != nil { | ||
| klog.ErrorS(splitErr, "eviction manager: failed to check if we have separate container filesystem. Ignoring.") |
There was a problem hiding this comment.
What's the impact of ignoring the error and assuming container and image filesystem are shared?
(in line 258 we error out when "HasDedicatedImageFs" returns an error)
There was a problem hiding this comment.
The first try of this PR returned an error if we couldn't check split fs. This lead to us not setting any eviction signals so we were hitting disk pressure limits and we would never trigger any eviction signals.
We set the signals only once and we re use those. Since we return once, we skip setting signals.
In case of container and image fs are shared, we don't really even need the containerfs signal.
There was a problem hiding this comment.
So if there's an error, we assume shared container and image filesystem. This is not great but at least we'll continue the evaluation and try to evict the pods if necessary. Is that right?
With the same reasoning, why shouldn't we ignore the imageFsErr? Just trying to understand if I missed something.
There was a problem hiding this comment.
I thought about it but that has been that way forever. So I thought against changing that.
There was a problem hiding this comment.
And yes, your thought process is correct.
There was a problem hiding this comment.
I guess I'm not sure in what situations we'd have permanent errors vs transient ones. If errors are usually transient, returning it and retry next time seems reasonable. If they could be permanent, continue and do best-effort eviction seems more desirable. In that case, maybe kubelet should send an event to surface the issue.
Have we seen any errors being reported in real life?
We could at least document why we ignore the error and add a TODO to revisit the error handling of both later.
There was a problem hiding this comment.
For imageFS I am not really sure of the error path.
SplitFS we add a label to cadvisor to detect the split filesystem only if the system is split. In this case we had an error and then we dropped eviction signals entirely.
If errors are usually transient, returning it and retry next time seems reasonable. If they could be permanent, continue and do best-effort eviction seems more desirable.
We only set eviction signals once on the sync loop of the eviction manager. There is a nill check for some flags around imagefs and if that is nil then we check for dedicatedImageFs. Once that value gets set that code is skipped.
There was a problem hiding this comment.
I added a comment. This PR wasn't really about fixing the imagefs path so I left it alone for now.
|
I don't understand what the changelog is saying about drives and partitions. "Drive" is a very dated term when it comes to storage, except on Windows. |
I went ahead and used the release note from the first try of this PR. |
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: cf760325e714513f4e5f1b786b6b4a2c406a2162
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kannon92, yujuhong 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
@kannon92: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Reintroduce #126562 (comment) and fix a bug where when split filesystem is not detected we were ending the eviction manager early and not setting rank functions.