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

Jump to bottom

Improved mount detection using openat2 for kernel 5.10+ #109217

Merged
merged 3 commits into from Jul 27, 2022
Merged

Improved mount detection using openat2 for kernel 5.10+ #109217

merged 3 commits into from Jul 27, 2022

Conversation

manugupt1
Copy link
Contributor

@manugupt1 manugupt1 commented Apr 1, 2022 •
edited

What type of PR is this?

/kind cleanup

/kind cleanup

What this PR does / why we need it:

Faster mount detection for linux kernel 5.10+ using openat2 speeding up pod churn rates.

Which issue(s) this PR fixes:

Fixes # #105642

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Faster mount detection for linux kernel 5.10+ using openat2 speeding up pod churn rates. On Kernel versions less 5.10, it will fallback to using the original way of detecting mount points i.e by parsing /proc/mounts.

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

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 1, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @manugupt1. 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 1, 2022
@k8s-ci-robot k8s-ci-robot requested review from dchen1107 and a team April 1, 2022 06:00
@k8s-ci-robot k8s-ci-robot added area/dependency Issues or PRs related to dependency changes sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/storage Categorizes an issue or PR as relevant to SIG Storage. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 1, 2022
@manugupt1
Copy link
Contributor Author

/kind cleanup

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 1, 2022
@jsafrane
Copy link
Member

jsafrane commented Apr 1, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 1, 2022
@jingxu97
Copy link
Contributor

jingxu97 commented Apr 1, 2022

Can this mountedFast function is also used by IsNotMountPoint? Is it work for bind mount too?

@jingxu97 jingxu97 mentioned this pull request Apr 1, 2022
Closed
@manugupt1
Copy link
Contributor Author

Can this mountedFast function is also used by IsNotMountPoint? Is it work for bind mount too?

Only for kernel 5.10+ / when openat2 is supported: https://github.com/moby/sys/blob/main/mountinfo/mounted_linux_test.go#L303-L320

jsafrane
jsafrane reviewed Apr 5, 2022
View reviewed changes
staging/src/k8s.io/mount-utils/mount_unsupported.go Outdated
// IsNotMountPoint enumerates all the mountpoints using List() and
// the list of mountpoints may be large, then it uses
// isMountPointMatch to evaluate whether the directory is a mountpoint.
func IsNotMountPoint(mounter Interface, file string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mount_unsupported.go, I think returning errUnsupported is perfectly fine here.

@manugupt1
Use faster mountdetections using openat2 for IsNotMountPoint
5a6cccc 
Reimplement IsNotMountpoint as IsMountPoint and return appropriate
error when a mount point is not detected.

IsMountPoint depends on MountedFast function in moby/sys/mountinfo
that uses openat2 call in kernel versions5.10 to detect
mount points without falling back to /proc/mounts.

Mark IsNotMountPoint as deprecated in order to ask the users to
use IsMountPoint directly.
@manugupt1
Fix type in mount_windows.go
08ef110
@manugupt1
Return unwrapped ErrNotExist when EvalSymlink returns PathError
44bea35 
By default filepath.EvalSymlink returns PathError. When a file is
not found, we should unwrap it and return ErrNotExist as this
is what this function expects.

Similar to the comment at:
kubernetes#109217 (comment)
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 27, 2022 •
edited

@manugupt1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-aks-engine-windows-containerd cd74df8930dab71ded02babebc3f839c16eac96b link false /test pull-kubernetes-e2e-aks-engine-windows-containerd
pull-kubernetes-e2e-capz-windows-containerd 44bea35 link false /test pull-kubernetes-e2e-capz-windows-containerd

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/test-infra repository. I understand the commands that are listed here.

@manugupt1
Copy link
Contributor Author

/test pull-kubernetes-verify-govet-levee

@jingxu97
Copy link
Contributor

/lgtm

in release note, maybe mention that for old kernel version, it will fall back to the original way to check mount point.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022
@jingxu97
Copy link
Contributor

@liggitt the change looks good to me. Thanks!

@liggitt
Copy link
Member

liggitt commented Jul 27, 2022

/approve
for go.mod file update

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, liggitt, manugupt1

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2022
@manugupt1
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd

@BenTheElder
Copy link
Member

/triage accepted
/priority important-longterm

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 27, 2022
@k8s-ci-robot k8s-ci-robot merged commit c27e826 into kubernetes:master Jul 27, 2022
@manugupt1 manugupt1 deleted the improve-mount-detection branch July 27, 2022 18:46
k8s-publishing-bot pushed a commit to kubernetes/mount-utils that referenced this pull request Jul 27, 2022
@manugupt1 @k8s-publishing-bot
Return unwrapped ErrNotExist when EvalSymlink returns PathError
50b2453 
By default filepath.EvalSymlink returns PathError. When a file is
not found, we should unwrap it and return ErrNotExist as this
is what this function expects.

Similar to the comment at:
kubernetes/kubernetes#109217 (comment)

Kubernetes-commit: 44bea358045e09f228ee0fbc594ec0f77bc71112
@jsafrane jsafrane mentioned this pull request Sep 26, 2022
Closed
@cartermckinnon cartermckinnon mentioned this pull request Jul 19, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@kolyshkin kolyshkin kolyshkin left review comments

@cartermckinnon cartermckinnon cartermckinnon left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@jingxu97 jingxu97

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@manugupt1 @k8s-ci-robot @jsafrane @jingxu97 @kolyshkin @hosseinsalahi @liggitt @BenTheElder @cartermckinnon @smarterclayton