Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

mount: Virtio-blk container rootfs mount for ACRN hypervisor #574

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

vijaydhanraj
Copy link
Contributor

For block devices added by acrn, virtpath is directly udpated
in storage.source and so no scanning of PCIAddr is required.
This is because, the virtio-blk device is not hot-plugged but
added during VM launch and updated later with container rootfs
using block rescan feature.

Fixes: #573

Signed-off-by: Vijay Dhanraj [email protected]

@vijaydhanraj
Copy link
Contributor Author

Related to: kata-containers/runtime#1779

@amshinde
Copy link
Member

amshinde commented Jun 7, 2019

@vijaydhanraj Seeing this error:

mount.go:275: File is not `gofmt`-ed with `-s` (gofmt)
        // If hot-plugged, get the device node path based on the PCI address else
        // use the virt path provided in Storage Source
        if strings.Contains(storage.Source, "/dev") {
                // Make sure the virt path is valid
                _, err := os.Stat(storage.Source)
                if err != nil {
                        return "", err
                }
        } else {
                devPath, err := getPCIDeviceName(s, storage.Source)
                if err != nil {
                        return "", err
                }

Can you gofmt the mount.go file?

@vijaydhanraj
Copy link
Contributor Author

@vijaydhanraj Seeing this error:

mount.go:275: File is not `gofmt`-ed with `-s` (gofmt)
        // If hot-plugged, get the device node path based on the PCI address else
        // use the virt path provided in Storage Source
        if strings.Contains(storage.Source, "/dev") {
                // Make sure the virt path is valid
                _, err := os.Stat(storage.Source)
                if err != nil {
                        return "", err
                }
        } else {
                devPath, err := getPCIDeviceName(s, storage.Source)
                if err != nil {
                        return "", err
                }

Can you gofmt the mount.go file?

Sure will do. Thanks!

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #574 into master will increase coverage by 0.22%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
+ Coverage   50.39%   50.62%   +0.22%     
==========================================
  Files          17       17              
  Lines        2385     2392       +7     
==========================================
+ Hits         1202     1211       +9     
  Misses       1026     1026              
+ Partials      157      155       -2

@amshinde
Copy link
Member

amshinde commented Jun 7, 2019

/test

mount.go Outdated

// If hot-plugged, get the device node path based on the PCI address else
// use the virt path provided in Storage Source
if strings.Contains(storage.Source, "/dev") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you add some unit tests for this code? This is a very weak match (not anchored) so would match things like /home/developer/foo.txt. Can you add anchoring reliably to guarantee you only match on actual devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using HasPrefix() now instead of Contains. Added a few negative test cases to check the flow.

mount.go Outdated
// use the virt path provided in Storage Source
if strings.Contains(storage.Source, "/dev") {
// Make sure the virt path is valid
_, err := os.Stat(storage.Source)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are matching devices, can you use os.Stat.Mode() and os.ModeDevice to check you really got one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

For block devices added by acrn, virtpath is directly udpated
in storage.source and so no scanning of PCIAddr is required.
This is because, the virtio-blk device is not hot-plugged but
added during VM launch and updated later with container rootfs
using block rescan feature.

v2->v3:
1. Addressed weak match for checking storage block device.
2. Added device check for storage block device.
3. Added negative unit tests for the new code.

v1->v2:
gofmt'd the mount file.

Fixes: kata-containers#573

Signed-off-by: Vijay Dhanraj <[email protected]>
@bergwolf
Copy link
Member

/test

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mount: virtioblk container rootfs mount for ACRN hypervisor
5 participants