Skip to content

Commit

Permalink
disk: fix partition detection
Browse files Browse the repository at this point in the history 
Do not rely on the device path to determine partition. Read sysfs instead.

Fixes CSI sanity failure:
Volume d-2ze2vto7mqwoffxi2uh9 GetDeviceRootAndIndex with error Device /dev/disk/by-id/virtio-2ze2vto7mqwoffxi2uh9 has error format more than one digit locations

Fixes: 9a71d43
  • Loading branch information
@huww98
huww98 committed Apr 26, 2024
1 parent 4812803 commit 2b3d217
Show file tree
Hide file tree
Showing 4 changed files with 156 additions and 82 deletions.
29 changes: 29 additions & 0 deletions pkg/disk/device_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"
"syscall"

Expand Down Expand Up @@ -226,3 +227,31 @@ func (m *DeviceManager) GetRootBlockByVolumeID(volumeID string) (string, error)

return "", utilerrors.Flatten(utilerrors.NewAggregate(errs))
}

func (m *DeviceManager) GetDeviceRootAndPartitionIndex(devicePath string) (string, int, error) {
if !m.EnableDiskPartition {
return devicePath, 0, nil
}
major, minor, err := m.DevTmpFS.DevFor(devicePath)
if err != nil {
return "", 0, err
}
devSysfsPath := fmt.Sprintf("%s/dev/block/%d:%d", m.SysfsPath, major, minor)
idxStr, err := os.ReadFile(devSysfsPath + "/partition")
if err == nil {
idx, err := strconv.Atoi(strings.TrimSpace(string(idxStr)))
if err != nil {
return "", 0, fmt.Errorf("parse partition from sysfs %q failed: %w", idxStr, err)
}
link, err := os.Readlink(devSysfsPath) // /sys/dev/block/253:3 -> ../../devices/pci0000:00/0000:00:05.0/virtio2/block/vda/vda3
if err != nil {
return "", 0, fmt.Errorf("readlink %q failed: %w", devSysfsPath, err)
}
rootDevName := filepath.Base(filepath.Dir(link)) // vda
return filepath.Join(m.DevicePath, rootDevName), idx, nil
}
if errors.Is(err, os.ErrNotExist) {
return devicePath, 0, nil
}
return "", 0, fmt.Errorf("read partition from sysfs %q failed: %w", devSysfsPath+"/partition", err)
}
139 changes: 122 additions & 17 deletions pkg/disk/device_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"os"
"path/filepath"
"strconv"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -35,6 +36,11 @@ var (
Minor: 16,
Path: "disk/by-id/virtio-mydiskserial",
}
virtIOPart = fakeDev{
Major: 253,
Minor: 23,
Path: "vdb23",
}
nvmeDev = fakeDev{
Major: 259,
Minor: 2,
Expand All @@ -45,6 +51,11 @@ var (
Minor: 2,
Path: "disk/by-id/nvme-Alibaba_Cloud_Elastic_Block_Storage_mydiskserial",
}
nvmePart = fakeDev{
Major: 259,
Minor: 29,
Path: "nvme1n1p27",
}
otherDev = fakeDev{
Major: 123,
Minor: 45,
Expand Down Expand Up @@ -108,7 +119,8 @@ func TestGetDeviceBySerialNotFound(t *testing.T) {
}

func setupVirtIOBlockDevice(t *testing.T, sysfsPath string) string {
sysfsDevicePath := filepath.Join(sysfsPath, "devices/pci0000:00/0000:00:0a.0/virtio7/block/vdb")
sysfsDev := "devices/pci0000:00/0000:00:0a.0/virtio7/block/vdb"
sysfsDevicePath := filepath.Join(sysfsPath, sysfsDev)
err := os.MkdirAll(sysfsDevicePath, 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(sysfsDevicePath, "serial"), []byte("mydiskserial\n"), 0o644)
Expand All @@ -118,11 +130,12 @@ func setupVirtIOBlockDevice(t *testing.T, sysfsPath string) string {
err = os.Symlink("../../devices/pci0000:00/0000:00:0a.0/virtio7/block/vdb", filepath.Join(sysfsPath, "dev/block/253:16"))
assert.NoError(t, err)

return sysfsDevicePath
return sysfsDev
}

func setupNVMeBlockDevice(t *testing.T, sysfsPath string) string {
sysfsDevicePath := filepath.Join(sysfsPath, "devices/pci0000:00/0000:00:07.0/nvme/nvme1/nvme1n1")
sysfsDev := "devices/pci0000:00/0000:00:07.0/nvme/nvme1/nvme1n1"
sysfsDevicePath := filepath.Join(sysfsPath, sysfsDev)
err := os.MkdirAll(sysfsDevicePath, 0o755)
assert.NoError(t, err)
err = os.Symlink("../../nvme1", filepath.Join(sysfsDevicePath, "device"))
Expand All @@ -134,7 +147,7 @@ func setupNVMeBlockDevice(t *testing.T, sysfsPath string) string {
err = os.Symlink("../../devices/pci0000:00/0000:00:07.0/nvme/nvme1/nvme1n1", filepath.Join(sysfsPath, "dev/block/259:2"))
assert.NoError(t, err)

return sysfsDevicePath
return sysfsDev
}

func TestGetDeviceBySerialVirtIO(t *testing.T) {
Expand Down Expand Up @@ -173,15 +186,23 @@ func extractGzip(t *testing.T, src, dst string) {
assert.NoError(t, err)
}

func sysfsSetupPartition(t *testing.T, sysfs, sysfsDev, deviceName string, dev *fakeDev, partitionIndex int) {
err := os.Mkdir(filepath.Join(sysfs, sysfsDev, deviceName), 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(sysfs, sysfsDev, deviceName, "partition"), []byte(strconv.Itoa(partitionIndex)+"\n"), 0o644)
assert.NoError(t, err)
err = os.Symlink(fmt.Sprintf("../%s/%s", sysfsDev, deviceName), fmt.Sprintf("%s/block/%s", sysfs, deviceName))
assert.NoError(t, err)
err = os.Symlink(fmt.Sprintf("../../%s/%s", sysfsDev, deviceName), fmt.Sprintf("%s/dev/block/%d:%d", sysfs, dev.Major, dev.Minor))
assert.NoError(t, err)
}

// returns the single formatted partition
func TestAdaptDevicePartitionPositive(t *testing.T) {
m := testingManager(t)
// Create a fake NVMe block device.
sysfsDev := setupNVMeBlockDevice(t, m.SysfsPath)
err := os.Mkdir(filepath.Join(sysfsDev, "nvme1n1p27"), 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(sysfsDev, "nvme1n1p27/partition"), []byte("27\n"), 0o644)
assert.NoError(t, err)
sysfsSetupPartition(t, m.SysfsPath, sysfsDev, "nvme1n1p27", &nvmePart, 27)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{nvmeDev}

extractGzip(t, "testdata/parted_disk.img.gz", filepath.Join(m.DevicePath, "nvme1n1"))
Expand Down Expand Up @@ -210,17 +231,11 @@ func TestAdaptDevicePartitionMultiplePartitions(t *testing.T) {
m := testingManager(t)
// Create a fake NVMe block device.
sysfsDev := setupNVMeBlockDevice(t, m.SysfsPath)
err := os.Mkdir(filepath.Join(sysfsDev, "nvme1n1p27"), 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(sysfsDev, "nvme1n1p27/partition"), []byte("27\n"), 0o644)
assert.NoError(t, err)
err = os.Mkdir(filepath.Join(sysfsDev, "nvme1n1p28"), 0o755)
assert.NoError(t, err)
err = os.WriteFile(filepath.Join(sysfsDev, "nvme1n1p28/partition"), []byte("28\n"), 0o644)
assert.NoError(t, err)
sysfsSetupPartition(t, m.SysfsPath, sysfsDev, "nvme1n1p27", &nvmePart, 27)
sysfsSetupPartition(t, m.SysfsPath, sysfsDev, "nvme1n1p28", &fakeDev{Major: 259, Minor: 28}, 28)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{nvmeDev}

_, err = m.adaptDevicePartition(filepath.Join(m.DevicePath, "nvme1n1"))
_, err := m.adaptDevicePartition(filepath.Join(m.DevicePath, "nvme1n1"))
assert.Error(t, err)
}

Expand Down Expand Up @@ -346,3 +361,93 @@ func TestGetDeviceByVolumeID_Positive(t *testing.T) {
})
}
}

func TestGetDeviceRootAndPartitionIndex_NoPartition(t *testing.T) {
cases := []struct {
name string
partitionEnabled bool
init func(t *testing.T, m *DeviceManager)
devicePath string
}{
{
name: "virtio",
partitionEnabled: true,
init: func(t *testing.T, m *DeviceManager) {
setupVirtIOBlockDevice(t, m.SysfsPath)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{virtIODev, virtIOLink}
},
devicePath: "disk/by-id/virtio-mydiskserial",
}, {
name: "nvme",
partitionEnabled: true,
init: func(t *testing.T, m *DeviceManager) {
setupNVMeBlockDevice(t, m.SysfsPath)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{nvmeDev, nvmeLink}
},
devicePath: "disk/by-id/nvme-Alibaba_Cloud_Elastic_Block_Storage_mydiskserial",
}, {
name: "disabled",
partitionEnabled: false,
init: func(t *testing.T, m *DeviceManager) {},
devicePath: "any-device",
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
m := testingManager(t)
m.EnableDiskPartition = c.partitionEnabled
c.init(t, m)

devPath := filepath.Join(m.DevicePath, c.devicePath)
root, index, err := m.GetDeviceRootAndPartitionIndex(devPath)
assert.NoError(t, err)
assert.Equal(t, devPath, root)
assert.Equal(t, 0, index)
})
}
}

func TestGetDeviceRootAndPartitionIndex_Partition(t *testing.T) {
cases := []struct {
name string
init func(t *testing.T, m *DeviceManager)
rootPath string
partitionPath string
partitionIndex int
}{
{
name: "virtio",
init: func(t *testing.T, m *DeviceManager) {
sysfsDev := setupVirtIOBlockDevice(t, m.SysfsPath)
sysfsSetupPartition(t, m.SysfsPath, sysfsDev, "vdb23", &virtIOPart, 23)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{virtIODev, virtIOPart}
},
rootPath: "vdb",
partitionPath: "vdb23",
partitionIndex: 23,
}, {
name: "nvme",
init: func(t *testing.T, m *DeviceManager) {
sysfsDev := setupNVMeBlockDevice(t, m.SysfsPath)
sysfsSetupPartition(t, m.SysfsPath, sysfsDev, "nvme1n1p27", &nvmePart, 27)
m.DevTmpFS.(*fakeDevTmpFS).Devs = []fakeDev{nvmeDev, nvmePart}
},
rootPath: "nvme1n1",
partitionPath: "nvme1n1p27",
partitionIndex: 27,
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
m := testingManager(t)
m.EnableDiskPartition = true
c.init(t, m)

part := filepath.Join(m.DevicePath, c.partitionPath)
root, index, err := m.GetDeviceRootAndPartitionIndex(part)
assert.NoError(t, err)
assert.Equal(t, filepath.Join(m.DevicePath, c.rootPath), root)
assert.Equal(t, c.partitionIndex, index)
})
}
}
11 changes: 5 additions & 6 deletions pkg/disk/nodeserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,12 +948,11 @@ func (ns *nodeServer) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandV
}

log.Infof("NodeExpandVolume:: volumeId: %s, devicePath: %s, volumePath: %s", diskID, devicePath, volumePath)
if DefaultDeviceManager.EnableDiskPartition && !IsDeviceNvme(devicePath) && isDevicePartition(devicePath) {
rootPath, index, err := getDeviceRootAndIndex(devicePath)
if err != nil {
log.Errorf("NodeExpandVolume:: GetDeviceRootAndIndex: %s with error: %s", diskID, err.Error())
return nil, status.Errorf(codes.InvalidArgument, "Volume %s GetDeviceRootAndIndex with error %s ", diskID, err.Error())
}
rootPath, index, err := DefaultDeviceManager.GetDeviceRootAndPartitionIndex(devicePath)
if err != nil {
return nil, status.Errorf(codes.Internal, "GetDeviceRootAndIndex(%s) failed: %v", diskID, err)
}
if index != 0 {
output, err := exec.Command("growpart", rootPath, strconv.Itoa(index)).CombinedOutput()
if err != nil {
if bytes.Contains(output, []byte("NOCHANGE")) {
Expand Down
59 changes: 0 additions & 59 deletions pkg/disk/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,9 @@ import (
"os"
"path"
"path/filepath"
"regexp"
"strconv"
"strings"
"time"
"unicode"

aliyunep "github.com/aliyun/alibaba-cloud-sdk-go/sdk/endpoints"
"github.com/aliyun/alibaba-cloud-sdk-go/sdk/requests"
Expand Down Expand Up @@ -331,54 +329,6 @@ func makeDevicePath(name string) string {
return filepath.Join("/dev/", name)
}

// return root device name, the partition index
// input /dev/vdb, output: /dev/vdb, -1, nil
// input /dev/vdb1, output: /dev/vdb, 1, nil
// input /dev/vdb22, output: /dev/vdb, 22, nil
func getDeviceRootAndIndex(devicePath string) (string, int, error) {
rootDevicePath := ""
index := -1
if IsDeviceNvme(devicePath) {
return devicePath, -1, nil
}
re := regexp.MustCompile(`\d+`)
regexpRes := re.FindAllStringSubmatch(devicePath, -1)
if len(regexpRes) == 0 {
// no digit find in device name
rootDevicePath = devicePath
index = -1
} else if len(regexpRes) == 1 {
if len(regexpRes[0]) == 0 {
return "", -1, fmt.Errorf("GetDeviceRootAndIndex: Device %s has error format %s ", devicePath, regexpRes[0])
}
numStr := regexpRes[0][0]
if !strings.HasSuffix(devicePath, numStr) {
return "", -1, fmt.Errorf("GetDeviceRootAndIndex: Device %s has error format, not endwith %s ", devicePath, numStr)
}
rootDevicePath = strings.TrimSuffix(devicePath, numStr)
indexTmp, err := strconv.Atoi(numStr)
if err != nil {
return "", -1, fmt.Errorf("GetDeviceRootAndIndex: Device %s strconv %s, with error: %s ", devicePath, numStr, err.Error())
}
index = indexTmp
} else {
// the partition format is end with digit, so never more than one digit locations
return "", -1, fmt.Errorf("Device %s has error format more than one digit locations ", devicePath)
}
return rootDevicePath, index, nil
}

func isDevicePartition(device string) bool {
if len(device) == 0 {
return false
}
lastChar := rune(device[len(device)-1])
if unicode.IsDigit(lastChar) {
return true
}
return false
}

// GetDiskFormat uses 'blkid' to see if the given disk is unformatted
func GetDiskFormat(disk string) (string, string, error) {
args := []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", disk}
Expand Down Expand Up @@ -1403,15 +1353,6 @@ func checkOptionFalse(opt string) bool {
}
}

// IsDeviceNvme check device is nvme type or not;
func IsDeviceNvme(deviceName string) bool {
fileName := filepath.Base(deviceName)
if strings.HasPrefix(fileName, "nvme") {
return true
}
return false
}

// getPvPvcFromDiskId returns a pv instance with specified disk ID
func getPvPvcFromDiskId(diskId string) (*v1.PersistentVolume, *v1.PersistentVolumeClaim, error) {
ctx := context.Background()
Expand Down

0 comments on commit 2b3d217

Please sign in to comment.