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

Improve mountinfo filtering for hotplugging NFS disks #9591

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

awels
Copy link
Member

@awels awels commented Apr 7, 2023

What this PR does / why we need it:
When a VM was running multiple NFS disks (one boot disk) and one hotplugged disk. The mount filtering was returning multiple possible mounts on the host, and sorting on the length of the 'root' path of the mount. For NFS these are the same (/). This caused the wrong NFS mount to be picked when hotplugging and an attempt was made to hotplug the boot disk. This commit improves the situation by passing the 'source' field to the filter, and it filters out entries that don't have matching sources. This allows us to find the correct mount on the host.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

BugFix: allow multiple NFS disks to be used/hotplugged

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Apr 7, 2023
@@ -192,12 +192,21 @@ func MountInfoRoot(r IsolationResult) (mountinfo *mount.Info, err error) {
return mountInfoFor(r, "/")
}

func mountsFilter(compare, m *mount.Info, source string) (bool, bool) {
nfsMatch := false
if m.FSType == "nfs4" && compare.FSType == m.FSType {
Copy link
Member

Choose a reason for hiding this comment

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

What about nfs3 or m.FSType == "nfs"?

Copy link
Member Author

@awels awels Apr 10, 2023

Choose a reason for hiding this comment

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

We don't support nfs3, just nfs4, but I can make the test be like strings.contains(m.fsType, "nfs")

When a VM was running multiple NFS disks (one boot disk)
and one hotplugged disk. The mount filtering was returning
multiple possible mounts on the host, and sorting on the
length of the 'root' path of the mount. For NFS these are
the same (/). This caused the wrong NFS mount to be picked
when hotplugging and an attempt was made to hotplug the boot
disk. This commit improves the situation by passing the 'source'
field to the filter, and it filters out entries that don't have
matching sources. This allows us to find the correct mount on
the host.

Signed-off-by: Alexander Wels <awels@redhat.com>
if multiple match the filter, the longest root is
selected per the sort of the length of the root
Changed check from exact nfs4 to prefix nfs to handle
other possible nfs versions

Signed-off-by: Alexander Wels <awels@redhat.com>
@mhenriks
Copy link
Member

/retest-required

@mhenriks
Copy link
Member

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2023
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 13, 2023

@awels: 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-kubevirt-fossa 4f812aa link false /test pull-kubevirt-fossa
pull-kubevirt-e2e-arm64 4f812aa link false /test pull-kubevirt-e2e-arm64

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.

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@awels
Copy link
Member Author

awels commented Apr 13, 2023

/test pull-kubevirt-e2e-arm64

@awels
Copy link
Member Author

awels commented May 31, 2023

/cherrypick release-0.59

@kubevirt-bot
Copy link
Contributor

@awels: new pull request created: #9825

In response to this:

/cherrypick release-0.59

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants