Skip to content

util: do not use mount-utils.IsLikelyNotMountPoint anymore#5203

Merged
mergify[bot] merged 2 commits intoceph:develfrom
nixpanic:issue/4633
Mar 14, 2025
Merged

util: do not use mount-utils.IsLikelyNotMountPoint anymore#5203
mergify[bot] merged 2 commits intoceph:develfrom
nixpanic:issue/4633

Conversation

@nixpanic
Copy link
Copy Markdown
Member

@nixpanic nixpanic commented Mar 7, 2025

IsLikelyNotMountPoint() is an optimized version for IsMountPoint()
which can not detect all type of mounts (anymore). The slower
IsMountPoint() is more safe to use. This can cause a slight
performance regression in the case there are many mountpoints on the
system, but correctness is more important than speed while mounting.

Fixes: #4633

@iPraveenParihar
Copy link
Copy Markdown
Contributor

/test ci/centos/mini-e2e-helm/k8s-1.32

@Madhu-1
Copy link
Copy Markdown
Collaborator

Madhu-1 commented Mar 10, 2025

/test ci/centos/mini-e2e-helm/k8s-1.31

@Madhu-1
Copy link
Copy Markdown
Collaborator

Madhu-1 commented Mar 10, 2025

@nixpanic we are using IsLikelyNotMountPoint in two other places (nfs,rbd) can we replace it as well?

@nixpanic
Copy link
Copy Markdown
Member Author

@nixpanic we are using IsLikelyNotMountPoint in two other places (nfs,rbd) can we replace it as well?

Sure, done!

@nixpanic nixpanic requested review from a team, Madhu-1 and iPraveenParihar March 11, 2025 08:59
@nixpanic
Copy link
Copy Markdown
Member Author

/test ci/centos/mini-e2e-helm/k8s-1.32

@nixpanic nixpanic requested review from a team and Madhu-1 March 12, 2025 12:01
Copy link
Copy Markdown
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, if possible please see if you can address the nits.

`IsLikelyNotMountPoint()` is an optimized version for `IsMountPoint()`
which can not detect all type of mounts (anymore). The slower
`IsMountPoint()` is more safe to use. This can cause a slight
performance regression in the case there are many mountpoints on the
system, but correctness is more important than speed while mounting.

Fixes: ceph#4633
Signed-off-by: Niels de Vos <ndevos@ibm.com>
The inverse checking and returning of is-a-mounted-path makes it
difficult to understand the function. It is easier to follow the code
when the function just returns what it says it does, hence added the
comment for the function too.

Some errors were returned directly, others were converted to gRPC
errors. This has been corrected now too, and the caller converts the
plain error to a gRPC error now.

Signed-off-by: Niels de Vos <ndevos@ibm.com>
Copy link
Copy Markdown
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM

@nixpanic nixpanic requested a review from a team March 13, 2025 15:12
@iPraveenParihar
Copy link
Copy Markdown
Contributor

/test ci/centos/mini-e2e-helm/k8s-1.32

@Rakshith-R
Copy link
Copy Markdown
Contributor

@Mergifyio queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 14, 2025

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 7f7988b

@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Mar 14, 2025
@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.32

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.31

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.32

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.32

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.31

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Mar 14, 2025
@mergify mergify bot merged commit 7f7988b into ceph:devel Mar 14, 2025
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

e2e: corrupted mount not attempting to recover.

5 participants