Replace stat syscall with statx #119968

j4ckstraw · 2023-08-16T09:13:50Z

statx with AT_STATX_DONT_SYNC will not stuck kubelet syncLoop.

stat will stuck in syscall when nfs server not responding, replace stat with statx to fix it.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #101622

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-08-16T09:13:58Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2023-08-16T09:13:59Z

Hi @j4ckstraw. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

staging/src/k8s.io/mount-utils/mount_linux.go

jsafrane · 2023-08-16T13:06:20Z

staging/src/k8s.io/mount-utils/mount_linux.go

+	var stat, rootStat unix.Statx_t
+	var err error
+
+	if stat, err = statx(file); err != nil {


Would it be better to reuse MountFast that needs 5.6 ?

It falls back to stat(), but only in < 5.6

I'm afraid not, because normalizePath will call Stat again.

rather than falling back to IsMountPoint - should it fall back to a implementation of IsLikelyNotMountPoint which uses stat ?

Sorry, I don't understand what you mean.

The key point here is that stat syscall will trap in kernel, which make kubelet struck in main loop. So I replace stat with statx. AT_STATX_DONT_SYNC option make statx don't sync with remote nfs server, so will not trap in kernel.

English is not my native language. Please forgive me if there are any grammatical mistakes :)

Good catch. IMO it would make sense to use statx in moby's normalizePath then
@kolyshkin, WDYT?

@j4ckstraw I was saying on kernels that don't support statx syscalls, rather than going to IsMountPoint which is slow and could be considered a regression in certain environments, we fall back to an implementation of IsLikelyNotMountPoint which still uses stat and yes, it will get kubelet stuck but it at least behave same as older version and will not be considered a regression.

@gnufied OK, I see. I agree with you, regression is bad, I will try to file a new patch. thank you for your comments.

@gnufied Updated, it's fallback to origin IsLikelyNotMountPoint now.
As you can see, I change the comment of IsLikelyNotMountPoint, I change the semantic of the IsLikelyNotMountPointin the last patch, so it is reliable, but with risk of regression and slow if the kernel not support statx.

After updated, it may be free of regression, but still have risk of making kubelet struck in main loop.
WDYT?

j4ckstraw · 2023-08-28T01:38:32Z

@jingxu97 Do you have time to take a look at this?

gnufied · 2023-08-30T15:23:44Z

thanks, this is looking good. Can you also add some tests while we are are at it?

j4ckstraw · 2023-08-31T02:12:18Z

thanks, this is looking good. Can you also add some tests while we are are at it?
let me try

j4ckstraw · 2023-10-09T11:53:29Z

thanks, this is looking good. Can you also add some tests while we are are at it?

I've been busy with other things, sorry for the late reply.
I failed to add test for IsLikelyNotMountPoint, because I can't make stat syscall trap in kernel in a test.

gnufied · 2023-10-09T17:13:07Z

/ok-to-test

staging/src/k8s.io/mount-utils/mount_linux.go

j4ckstraw · 2023-10-10T02:19:28Z

/retest

statx with AT_STATX_DONT_SYNC will not stuck kubelet syncLoop. Signed-off-by: j4ckstraw <j4ckstraw@foxmail.com> fix remove useless comment Signed-off-by: j4ckstraw <j4ckstraw@foxmail.com>

j4ckstraw · 2023-12-27T01:31:56Z

Unmount

I agree with you.

249043822 · 2023-12-28T08:06:48Z

@j4ckstraw I cherry-picked your commit to our branch, on Linux 4.18, we found that the IsLikelyNotMountPoint works not well, the emptydir mounts netested continuously

j4ckstraw · 2023-12-29T01:26:57Z

@j4ckstraw I cherry-picked your commit to our branch, on Linux 4.18, we found that the IsLikelyNotMountPoint works not well, the emptydir mounts netested continuously

@249043822 If no cherry-pick, whether there is a nested mount

249043822 · 2023-12-29T01:38:26Z

@j4ckstraw I cherry-picked your commit to our branch, on Linux 4.18, we found that the IsLikelyNotMountPoint works not well, the emptydir mounts netested continuously

@249043822 If no cherry-pick, whether there is a nested mount

No, it's normal

staging/src/k8s.io/mount-utils/mount_linux.go

Signed-off-by: j4ckstraw <j4ckstraw@foxmail.com>

staging/src/k8s.io/mount-utils/mount_linux.go

Signed-off-by: j4ckstraw <j4ckstraw@foxmail.com>

j4ckstraw · 2024-01-08T03:12:21Z

/test pull-kubernetes-node-e2e-containerd

jsafrane · 2024-01-08T19:05:48Z

/lgtm
/approve

k8s-ci-robot · 2024-01-08T19:05:56Z

LGTM label has been added.

Git tree hash: 9d7a7289f31ee79dc8014877ab3efcfb62fe19e2

k8s-ci-robot · 2024-01-08T19:05:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: j4ckstraw, jsafrane

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:

~~staging/src/k8s.io/mount-utils/OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Fix: IsLikelyNotMountPoint hangs forever if nfs server unreachable. See kubernetes/kubernetes#119968

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 16, 2023

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 16, 2023

k8s-ci-robot requested review from gnufied and saad-ali August 16, 2023 09:14

j4ckstraw force-pushed the replace-stat-with-statx branch from 606ad88 to a8d03fe Compare August 16, 2023 09:14

sftim reviewed Aug 16, 2023

View reviewed changes