-
Notifications
You must be signed in to change notification settings - Fork 257
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
scsi: Support optional guest path for mount, add tests, refcount fix #2278
Conversation
I think you're missing the AddVirtualDisk call in a few places, particularly the runhcs create-scratch call, some functional tests, and the uvmboot tool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Currently the SCSI mount manager will generate a new path for each new mount, based on a format string that it is instantiated with. However, it turns out some code in the GCS (e.g. sandbox mounts) assumes that the container scratch is mounted at a certain path. The long-term best solution here is probably to pass what paths to use explicitly to the GCS, but that would be more impactful. We need a more contained fix. This commit addresses the issue by allowing an optional guest path to be given for a SCSI mount. The mount manager has been changed as follows: - If a guest path is not supplied: The mount can re-use (refcount++) any existing mount with the same controller/lun/options. If a new mount is created, the mount manager will generate a path for it. - If a guest path is supplied: The mount can re-use (refcount++) any existing mount with the same controller/lun/guestpath/options. If a new mount is created, the mount manager will use the supplied path for it. Accordingly, code calling into the mount manager has been updated to pass an empty string for the guest path. The exception to this is the LCOW layer mounting code, which will pass an explicit guest path for the scratch disk. As far as I know, WCOW does not depend on a specific path for its scratch disk. Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
78d08f4
to
6830384
Compare
Thanks, fixed |
Actually might still need to fix tests. I think find references in gopls might have missed those |
cdc61ef
to
a596691
Compare
Adds various tests to the SCSI manager code. As part of this testing, a bug tracking attachment refcounts was also found. The attachment refcount is increased every time a new top-level request comes in, even if an existing mount is re-used. This means that the attachment refcount should also be decremented every time one is released, even if the mount refcount is not at 0. Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
a596691
to
3b7c087
Compare
@katiewasnothere any concerns once CI passes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds support for callers to
internal/uvm/scsi.Manager
to specify a path where the disk will be mounted in the guest.Adds unit tests for SCSI manager behavior.
Fixes a refcount bug found through the tests.
Details in the individual commits.