-
Notifications
You must be signed in to change notification settings - Fork 256
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 ensure filesystem #1757
SCSI ensure filesystem #1757
Conversation
internal/guest/storage/scsi/scsi.go
Outdated
// wait for `source` to show up. | ||
for { | ||
if _, err := osStat(source); err != nil { | ||
if errors.Is(err, fs.ErrNotExist) || errors.Is(err, unix.ENOENT) || errors.Is(err, unix.ENXIO) { |
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.
Previously we were checking the error from unix.Mount
against ENOENT
and ENXIO
. Now we are checking the error from os.Stat
against those as well as ErrNotExist
. What's the reasoning behind the set of errors checked here? Do we expect it will always be ErrNotExist
, and we are keeping the previous errors just in case, but don't think they are needed?
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.
The man page for stat does mention ENOENT
(see here), but doesn't directly mention ENXIO
. The page could just be out of date though.
I also did some testing where I ran Test_70LayerImagesWithNoVPmemForLayers, recommended by Amit, roughly 150 times. I only ever saw ErrNotExist
and ENOENT
(at the same time), but I'm not confident we'd never hit ENXIO
so I left it.
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.
syscall.ENOENT
is implemented such that it will always be equal to fs.ErrNotExist
, so I don't think we need to check both of those. I'd also be in favor of either removing ENXIO
from the set unless we have a reason to keep it in. Maybe @anmaxvl has context here?
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.
I removed the ENOENT
option. Waiting to hear from Maksim or Amit on the ENXIO
error.
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.
sorry, just saw the ping. I think Amit was the one who was hitting ENXIO
with newer kernel. In my case, I was running into ENODEV
, when trying to format/encrypt scratch...
Edit:
It could've also been when mounting layers with verity actually, now that I think about it... formatting scratch won't return an actual error, but rather we parse through the mkfs.xfs
or cryptsetup
output. I believe @KenGordon actually saw "device not found" errors when adding scratch.
fbd2f91
to
67676d2
Compare
67676d2
to
6c08fb7
Compare
After finding some issues with busybox's I first intended to include code here to determine if a device was formatted with xfs and code to format devices with xfs if requested when |
8f08336
to
246204e
Compare
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 pending the following:
- Final rebase on earlier PRs once they are merged.
- Rebase to combine commits in this PR into a good set.
- CI green.
2a76b0d
to
0415226
Compare
059cb9a
to
507bf4c
Compare
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
* Add new `EnsureFilesystem` and `Filesystem` options on LCOWMappedVirtualDisk * Add matching `EnsureFilesystem` and `Filesystem` options on scsi MountConfig * Set `EnsureFilesystem` and `Filesystem` when attaching scratch devices * Add plumbing in guest scsi package to support `EnsureFilesystem` and `Filesystem`. * Create new `Config` type in guest scsi package for passing in setup/cleanup configuration settings * Add new call to get a device's filesystem by reading its superblock * Add new scsi unit tests for new features and update existing tests * New package for formatting using xfs * Move xfs formatting for encrypted devices out of crypt pkg into scsi Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
507bf4c
to
af8c444
Compare
Rebased. Will merge on CI passing. |
This PR updates our ADO fork to commits in hcsshim up to commit hash [7769a64](7769a64). This includes support for partitioned scsi devices and ensuring filesystem format for lcow scsi devices. Related work items: #1728, #1740, #1741, #1742, #1743, #1744, #1745, #1747, #1748, #1749, #1750, #1752, #1754, #1756, #1757, #1767, #1769, #1771, #1772, #1773, #1779
This PR is based on #1747, which is in turn based on the below PRs:
This PR additionally has one temporary commit that was used for testing. When the PRs that this PR is based on are updated, this PR will also need to be updated and will likely have some rebase conflicts. You can find the target code changes in the last commit on this PR.
The main work in this PR is the creation of two new options on an LCOW scsi mount
EnsureFilesystem
andFilesystem
. These options are added to light up the ability to optionally ensure that a disk has a specific filesystem configured on it in the guest.This PR does the following:
EnsureFilesystem
andFilesystem
options on LCOWMappedVirtualDiskEnsureFilesystem
andFilesystem
options on scsi MountConfigEnsureFilesystem
andFilesystem
when attaching scratch devicesEnsureFilesystem
andFilesystem
.Config
type in guest scsi package for passing in setup/cleanup configuration settingsNote: it is expected that this PR does not currently pass the linter or integration tests in the CI.