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.

Sign up for GitHub

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

Add longhorn v2 provisioner #82

Closed
wants to merge 3 commits into from
Closed

Add longhorn v2 provisioner #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68e5a0e
manifest: add provisioner
Vicente-Cheng Mar 2, 2024
d1733c1
controller: add longhorn v2 engine provisioner
Vicente-Cheng Mar 2, 2024
eab91a9
misc: move doCommand and remove redundant code
Vicente-Cheng Mar 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions manifests/crds/harvesterhci.io_blockdevices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ spec:
nodeName:
description: name of the node to which the block device is attached
type: string
provisioner:
description: a string with the provisioner name, e.g. "longhornv1",
longhornv2"
type: string
tags:
description: a string list with device tag for provisioner, e.g. ["default",
"small", "ssd"]
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/harvesterhci.io/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ type BlockDeviceSpec struct {

// a string list with device tag for provisioner, e.g. ["default", "small", "ssd"]
Tags []string `json:"tags,omitempty"`

// a string with the provisioner name, e.g. "longhornv1", longhornv2"
Provisioner string `json:"provisioner,omitempty"`
}

type BlockDeviceStatus struct {
Expand Down
41 changes: 26 additions & 15 deletions pkg/controller/blockdevice/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (

const (
blockDeviceHandlerName = "harvester-block-device-handler"
defaultProvisioner = "longhornv1"
)

// semaphore is a simple semaphore implementation in channel
Expand Down Expand Up @@ -212,12 +213,16 @@ func (c *Controller) OnBlockDeviceChange(_ string, device *diskv1.BlockDevice) (
if devPath == "" {
return nil, fmt.Errorf("failed to resolve persistent dev path for block device %s", device.Name)
}
if device.Spec.Provisioner == "" {
Copy link

Choose a reason for hiding this comment

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

nit: Do we have a mutator mechanism to add the default value? If yes, we can prevent this sort of non-biz code in controller.

deviceCpy.Spec.Provisioner = defaultProvisioner
}

filesystem := c.BlockInfo.GetFileSystemInfoByDevPath(devPath)
devPathStatus := convertFSInfoToString(filesystem)
logrus.Debugf("Get filesystem info from device %s, %s", devPath, devPathStatus)

needFormat := deviceCpy.Spec.FileSystem.ForceFormatted && (deviceCpy.Status.DeviceStatus.FileSystem.Corrupted || deviceCpy.Status.DeviceStatus.FileSystem.LastFormattedAt == nil)
if needFormat {
if deviceCpy.Spec.Provisioner == defaultProvisioner && needFormat {
logrus.Infof("Prepare to force format device %s", device.Name)
err := c.forceFormat(deviceCpy, devPath, filesystem)
if err != nil {
Expand All @@ -233,7 +238,8 @@ func (c *Controller) OnBlockDeviceChange(_ string, device *diskv1.BlockDevice) (
return device, err
}

if needMountUpdate := needUpdateMountPoint(deviceCpy, filesystem); needMountUpdate != NeedMountUpdateNoOp {
needMountUpdate := needUpdateMountPoint(deviceCpy, filesystem)
if deviceCpy.Spec.Provisioner == defaultProvisioner && needMountUpdate != NeedMountUpdateNoOp {
err := c.updateDeviceMount(deviceCpy, devPath, filesystem, needMountUpdate)
if err != nil {
err := fmt.Errorf("failed to update device mount %s: %s", device.Name, err.Error())
Expand Down Expand Up @@ -461,13 +467,14 @@ func (c *Controller) provisionDeviceToNode(device *diskv1.BlockDevice) error {
}

nodeCpy := node.DeepCopy()
diskSpec := longhornv1.DiskSpec{
Path: extraDiskMountPoint(device),
AllowScheduling: true,
EvictionRequested: false,
StorageReserved: 0,
Tags: device.Spec.Tags,

diskSpec := generateDiskSpec(device.Spec.Provisioner)
if device.Spec.Provisioner == defaultProvisioner {
Copy link

Choose a reason for hiding this comment

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

nit: suggest handling each case explicitly to prevent using else for v2. Can use switch.

diskSpec.Path = extraDiskMountPoint(device)
} else {
diskSpec.Path = device.Status.DeviceStatus.DevPath
}
diskSpec.Tags = device.Spec.Tags

updated := false
if disk, found := node.Spec.Disks[device.Name]; found {
Expand Down Expand Up @@ -803,12 +810,16 @@ func convertFSInfoToString(fsInfo *block.FileSystemInfo) string {
return fmt.Sprintf("mountpoint: %s, fsType: %s", fsInfo.MountPoint, fsInfo.Type)
}

func removeUnNeeded[T string | int](x []T, y []T) []T {
result := make([]T, 0)
for _, item := range x {
if !slices.Contains(y, item) {
result = append(result, item)
}
func generateDiskSpec(provisioner string) longhornv1.DiskSpec {
diskType := longhornv1.DiskTypeFilesystem
if provisioner == "longhornv2" {
diskType = longhornv1.DiskTypeBlock
}
spec := longhornv1.DiskSpec{
Type: diskType,
AllowScheduling: true,
EvictionRequested: false,
StorageReserved: 0,
}
return result
return spec
}
12 changes: 12 additions & 0 deletions pkg/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package utils

import (
"bytes"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -175,3 +176,14 @@ func CallerWithCondLock[T any](cond *sync.Cond, f func() T) T {
defer cond.L.Unlock()
return f()
}

// DoCommand executes a command and returns the stdout, stderr and error
func DoCommand(cmdString string) (string, string, error) {
Copy link

Choose a reason for hiding this comment

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

Nit: This function is quite general. Not sure if there is an existing lib able to use or even we can add this to common lib for other repos to reuse later if appropriate.

Can extend it to pass io.Writer out and err instead of hard-coded bytes buffer here only.

If for testing only, why not just add this to a related test package folder?

var stdout bytes.Buffer
var stderr bytes.Buffer
cmd := exec.Command("bash", "-c", cmdString)
cmd.Stdout = &stdout
cmd.Stderr = &stderr
err := cmd.Run()
return stdout.String(), stderr.String(), err
}
20 changes: 4 additions & 16 deletions tests/integration/test_1_disk_hotplug_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
package integration

import (
"bytes"
"context"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
Expand Down Expand Up @@ -118,7 +116,7 @@ func (s *HotPlugTestSuite) Test_0_PreCheckForDiskCount() {
func (s *HotPlugTestSuite) Test_1_HotPlugRemoveDisk() {
// remove disk dynamically
cmd := fmt.Sprintf("virsh detach-disk %s %s --live", hotplugTargetNodeName, hotplugTargetDiskName)
_, _, err := doCommand(cmd)
_, _, err := utils.DoCommand(cmd)
require.Equal(s.T(), err, nil, "Running command `virsh detach-disk` should not get error")

// wait for controller handling
Expand All @@ -137,7 +135,7 @@ func (s *HotPlugTestSuite) Test_1_HotPlugRemoveDisk() {
func (s *HotPlugTestSuite) Test_2_HotPlugAddDisk() {
// remove disk dynamically
cmd := fmt.Sprintf("virsh attach-device --domain %s --file %s --live", hotplugTargetNodeName, hotplugDiskXMLFileName)
_, _, err := doCommand(cmd)
_, _, err := utils.DoCommand(cmd)
require.Equal(s.T(), err, nil, "Running command `virsh attach-device` should not get error")

// wait for controller handling
Expand All @@ -161,7 +159,7 @@ func (s *HotPlugTestSuite) Test_3_AddDuplicatedWWNDsik() {
duplicatedDeviceRaw = "/tmp/hotplug_disks/node1-sdb.qcow2"
)
cmdCpyRawFile := fmt.Sprintf("cp %s %s", originalDeviceRaw, duplicatedDeviceRaw)
_, _, err := doCommand(cmdCpyRawFile)
_, _, err := utils.DoCommand(cmdCpyRawFile)
require.Equal(s.T(), err, nil, "Running command `cp the raw device file` should not get error")

disk, err := utils.DiskXMLReader(hotplugDiskXMLFileName)
Expand All @@ -172,7 +170,7 @@ func (s *HotPlugTestSuite) Test_3_AddDuplicatedWWNDsik() {
require.Equal(s.T(), err, nil, "Write xml file should not get error")

cmd := fmt.Sprintf("virsh attach-device --domain %s --file %s --live", hotplugTargetNodeName, duplicatedDeviceXML)
_, _, err = doCommand(cmd)
_, _, err = utils.DoCommand(cmd)
require.Equal(s.T(), err, nil, "Running command `virsh attach-device` should not get error")

// wait for controller handling
Expand All @@ -189,13 +187,3 @@ func (s *HotPlugTestSuite) Test_3_AddDuplicatedWWNDsik() {
require.Equal(s.T(), s.curBusPath, curBlockdevice.Status.DeviceStatus.Details.BusPath, "Disk path should not replace by duplicated wwn disk")

}

func doCommand(cmdString string) (string, string, error) {
var stdout bytes.Buffer
var stderr bytes.Buffer
cmd := exec.Command("bash", "-c", cmdString)
cmd.Stdout = &stdout
cmd.Stderr = &stderr
err := cmd.Run()
return stdout.String(), stderr.String(), err
}
Loading