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

Feature: support direct disk mounting for service container #201

Merged
merged 1 commit into from
Apr 10, 2023
+476 −135
Merged

Feature: support direct disk mounting for service container #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 51 additions & 32 deletions cli/command/disks/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (

"github.com/fatih/color"
"github.com/opencurve/curveadm/cli/cli"
"github.com/opencurve/curveadm/internal/common"
comm "github.com/opencurve/curveadm/internal/common"
"github.com/opencurve/curveadm/internal/configure/disks"
"github.com/opencurve/curveadm/internal/configure/topology"
Expand All @@ -40,7 +39,8 @@ import (
)

const (
COMMIT_EXAMPLE = `Examples:
HOST_DEVICE_SEP = ":"
COMMIT_EXAMPLE = `Examples:
$ curveadm disks commit /path/to/disks.yaml # Commit disks`
)

Expand Down Expand Up @@ -89,35 +89,55 @@ func readAndCheckDisks(curveadm *cli.CurveAdm, options commitOptions) (string, [
}

// 3) check disks data
dcs, err = disks.ParseDisks(data, curveadm)
dcs, err = disks.ParseDisks(data)
return data, dcs, err
}

func assambleNewDiskRecords(dcs []*disks.DiskConfig,
oldDiskRecords []storage.Disk) ([]storage.Disk, []storage.Disk) {
keySep := ":"
newDiskMap := make(map[string]bool)

// "ServiceMountDevice=0" means write disk UUID into /etc/fstab for host mounting.
// "ServiceMountDevice=1" means not to update /etc/fstab, the disk UUID will be wrote
// into the config of service(chunkserver) container for disk automatic mounting.
serviceMountDevice := 0 // 0: false, 1: true
var newDiskRecords, diskRecordDeleteList []storage.Disk
for _, dc := range dcs {
for _, host := range dc.GetHost() {
key := strings.Join([]string{host, dc.GetDevice()}, keySep)
device := dc.GetDevice()
key := strings.Join([]string{host, device}, HOST_DEVICE_SEP)
newDiskMap[key] = true
if dc.GetServiceMount() {
serviceMountDevice = 1
}
diskSize := comm.DISK_DEFAULT_NULL_SIZE
diskUri := comm.DISK_DEFAULT_NULL_URI
diskChunkserverId := comm.DISK_DEFAULT_NULL_CHUNKSERVER_ID
for _, dr := range oldDiskRecords {
if dr.Host == host && device == dr.Device {
diskSize = dr.Size
diskUri = dr.URI
diskChunkserverId = dr.ChunkServerID
break
}
}
newDiskRecords = append(
newDiskRecords, storage.Disk{
Host: host,
Device: dc.GetDevice(),
Size: comm.DISK_DEFAULT_NULL_SIZE,
URI: comm.DISK_DEFAULT_NULL_URI,
MountPoint: dc.GetMountPoint(),
FormatPercent: dc.GetFormatPercent(),
ChunkServerID: comm.DISK_DEFAULT_NULL_CHUNKSERVER_ID,
Host: host,
Device: device,
Size: diskSize,
URI: diskUri,
MountPoint: dc.GetMountPoint(),
ContainerImage: dc.GetContainerImage(),
FormatPercent: dc.GetFormatPercent(),
ChunkServerID: diskChunkserverId,
ServiceMountDevice: serviceMountDevice,
})
}
}

for _, dr := range oldDiskRecords {
key := strings.Join([]string{dr.Host, dr.Device}, keySep)
key := strings.Join([]string{dr.Host, dr.Device}, HOST_DEVICE_SEP)
if _, ok := newDiskMap[key]; !ok {
diskRecordDeleteList = append(diskRecordDeleteList, dr)
}
Expand All @@ -127,19 +147,16 @@ func assambleNewDiskRecords(dcs []*disks.DiskConfig,
}

func writeDiskRecord(dr storage.Disk, curveadm *cli.CurveAdm) error {
if diskRecords, err := curveadm.Storage().GetDisk(
common.DISK_FILTER_DEVICE, dr.Host, dr.Device); err != nil {
if err := curveadm.Storage().SetDisk(
dr.Host,
dr.Device,
dr.MountPoint,
dr.ContainerImage,
dr.FormatPercent,
dr.ServiceMountDevice); err != nil {
return err
} else if len(diskRecords) == 0 {
if err := curveadm.Storage().SetDisk(
dr.Host,
dr.Device,
dr.MountPoint,
dr.ContainerImage,
dr.FormatPercent); err != nil {
return err
}
}

return nil
}

Expand All @@ -153,15 +170,17 @@ func syncDiskRecords(data string, dcs []*disks.DiskConfig,
oldDiskRecordsString := tui.FormatDisks(oldDiskRecords)
newDiskRecordsString := tui.FormatDisks(newDiskRecords)

if !options.slient {
diff := utils.Diff(oldDiskRecordsString, newDiskRecordsString)
curveadm.WriteOutln(diff)
}
if len(newDiskRecords) != len(oldDiskRecords) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the confirmation is required only if the length of disk records were changed. Is the confirmation necessary when there are any modification on any property(device, mount path, etc) of disk records ?

Copy link
Author

Choose a reason for hiding this comment

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

The confirmation of disk number changing(add or delete) is a double confirmation to show user that what disk records will be added or deleted. The modification of other properties will be shown in the first confirmation which is the confirmation of disks changing.

if !options.slient {
diff := utils.Diff(oldDiskRecordsString, newDiskRecordsString)
curveadm.WriteOutln(diff)
}

pass := tuicomm.ConfirmYes("Disk changes are showing above. Do you want to continue?")
if !pass {
curveadm.WriteOut(tuicomm.PromptCancelOpetation("commit disk table"))
return errno.ERR_CANCEL_OPERATION
pass := tuicomm.ConfirmYes("Disk changes are showing above. Do you want to continue?")
if !pass {
curveadm.WriteOut(tuicomm.PromptCancelOpetation("commit disk table"))
return errno.ERR_CANCEL_OPERATION
}
}

// write new disk records
Expand Down
14 changes: 11 additions & 3 deletions cli/command/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,17 @@ func runFormat(curveadm *cli.CurveAdm, options formatOptions) error {
if err != nil {
return err
}
fc.UseDiskUri = true
chunkserverId := dr.ChunkServerID
if len(chunkserverId) > 1 {

fc.FromDiskRecord = true

// "ServiceMountDevice=0" means write disk UUID into /etc/fstab for host mounting.
// "ServiceMountDevice=1" means not to update /etc/fstab, the disk UUID will be wrote
// into the config of service(chunkserver) container for disk automatic mounting.
if dr.ServiceMountDevice != 0 {
fc.ServiceMountDevice = true
}

if dr.ChunkServerID != comm.DISK_DEFAULT_NULL_CHUNKSERVER_ID {
// skip formatting the disk with nonempty chunkserver id
continue
}
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The patch adds logic to the runFormat() function to check the chunkserverID and ServiceMountDevice values before formatting a disk.
  2. The new logic appears to be sound, however it is advisable to include some unit tests to ensure that the new logic works as expected.
  3. It might be beneficial to add some comments to the code explaining why this new logic is necessary.
  4. Additionally, if the ServiceMountDevice value is not set to 0 then the code should include some additional checks to ensure that the disk UUID will be written into the correct config file.

Copy link

Choose a reason for hiding this comment

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

  1. The code looks clean and well-structured.
  2. There is a risk that dr.ServiceMountDevice != 0 could cause unexpected behavior if it is not properly handled.
  3. There is a potential bug with the condition if dr.ChunkServerID != comm.DISK_DEFAULT_NULL_CHUNKSERVER_ID, as the variable should be dr.ChunkServerID instead of dr.ChunkServerID.
  4. It is advisable to add comments to explain what the code is doing and also add some logging statements to help with debugging.

Copy link

Choose a reason for hiding this comment

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

with the code review:

  1. The first part of the code snippet checks the error from a given function and returns it if it exists. This is good practice, as it allows for errors to be easily identified and handled by the program.

  2. The second part sets the "FromDiskRecord" flag to true. This is necessary to ensure that the disk record is properly read before formatting the disk.

  3. The third part checks the "ServiceMountDevice" flag, and if it is set to 1, the "ServiceMountDevice" flag is set to true. This ensures that the disk UUID will be written into the config of the service (chunkserver) container for disk automatic mounting.

  4. The fourth part checks if the chunkserverId is not the default null chunkserver id, and if so, skips formatting the disk. This is important to avoid data loss.

Overall, the code looks correct and is well written. There doesn't seem to be any bugs or risks present, and no improvements are necessary.

Expand Down
11 changes: 6 additions & 5 deletions configs/bs/cluster/disks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
global:
format_percent: 95
container_image: opencurvedocker/curvebs:v1.2
service_mount_device: false # disk device will be mounted by service(chunkserver) directly if set "true", default "false"
host:
- curve-1
- curve-2
- curve-3

disk:
- device: /dev/sdb1
mount: /data/chunkserver0
- device: /dev/sdb1 # disk device path
mount: /data/chunkserver0 # mount point for disk formatting, consistent with the "data_dir" field of chunkserver service in topology
- device: /dev/sdc1
mount: /data/chunkserver1
format_percent: 90
format_percent: 90 # use a different value for disk format percent
- device: /dev/sdd1
mount: /data/chunkserver2
exclude: # for the use case that some hosts have not certain disk device
exclude: # for the use case that disk device does not exist in some hosts
- curve-3
- device: /dev/sde1
mount: /data/chunkserver3
host:
host: # override global host config, for the use case that disk device only exists in some hosts
- curve-1
- curve-2
Copy link

Choose a reason for hiding this comment

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

with a brief code review:

The code patch looks good. The addition of the "service_mount_device" field to the global block is a welcome change and it provides more flexibility in how disk devices are mounted. The inclusion of the "exclude" field in the disk block is also useful, as it allows disk devices to be excluded from certain hosts. Finally, the "host" field in the disk block allows for overriding the global host config for specific disk devices, which is helpful for cases where a disk device only exists on certain hosts. Overall, the code patch looks well-constructed and should not have any major risks or bugs.

Copy link

Choose a reason for hiding this comment

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

with code review.

  1. Code Structure: The code structure looks good and easy to read.
  2. Comments: The comments provided throughout the code give a good indication of what each section does.
  3. Variables: The variables are named well and are self-explanatory.
  4. Security: No security risks were identified in the code.
  5. Performance: The code looks optimised for performance.

Suggestions:

  1. Add more comments to explain the usage of certain variables.
  2. Make sure that the mount points for each disk device is consistent with the "data_dir" field of chunkserver service in topology.
  3. Check if all hosts have the same version of opencurvedocker/curvebs.

Copy link

Choose a reason for hiding this comment

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

Code Review:

  1. Global format_percent and service_mount_device:
    The global format_percent and service_mount_device variables should be placed at the top of the configuration. This allows for easier debugging and editing.

  2. Disk device and mount point:
    The disk device and mount point should be clearly labeled and the default value for service_mount_device should be clarified in the comments.

  3. Exclude and host override:
    The exclude and host override options should be documented more clearly with examples so that users know how to use them.

4 changes: 2 additions & 2 deletions internal/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ const (
DISK_FILTER_MOUNT = "mount"
DISK_FILTER_SERVICE = "service"
DISK_EXCLUDE_HOST = "exclude"

DISK_FORMAT_PERCENT = "format_percent"
DISK_SERVICE_MOUNT_DEVICE = "service_mount_device"
DISK_FORMAT_PERCENT = "format_percent"

DISK_FORMAT_MOUNT_POINT = "mount"
DISK_FORMAT_CONTAINER_IMAGE = "container_image"
Copy link

Choose a reason for hiding this comment

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

with the code review

  1. The code patch adds two new constants - DISK_SERVICE_MOUNT_DEVICE and DISK_FORMAT_PERCENT. This should be documented properly to give a better understanding of the purpose of the constants.

  2. The DISK_EXCLUDE_HOST constant should also be documented as it is an important parameter that defines which hosts need to be excluded from the disk operations.

  3. There should be some checks in place to ensure that the values assigned to the constants are valid. For example, the value of DISK_FORMAT_PERCENT should be checked to ensure it's a valid number between 0 and 100.

  4. A unit test should be written to ensure that the code is working as expected. This will help in catching any bugs that might have been missed during the code review.

Copy link

Choose a reason for hiding this comment

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

from the code patch.

The code looks correct and does not have any bug risks. The only suggestion for improvement is to add comments for each of the constants in order to make the code easier to read and understand. This will help other developers understand the purpose and usage of each constant more easily.

Copy link

Choose a reason for hiding this comment

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

with the code review

  1. The patch adds a new constant DISK_SERVICE_MOUNT_DEVICE which seems to be related to disk service mount device. The name is meaningful and consistent with other constants.

  2. The patch also adds DISK_FORMAT_PERCENT which looks like one of the formatting options for disk usage. The name is also meaningful and consistent.

  3. The indentation of the code is correct and in-line with existing code.

  4. There are no bugs or risks associated with the patch, and it should be safe to deploy.

  5. There is no room for improvement in this patch.

Expand Down
47 changes: 22 additions & 25 deletions internal/configure/common/item_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const (
REQUIRE_BOOL
REQUIRE_INT
REQUIRE_POSITIVE_INTEGER
REQUIRE_SLICE
REQUIRE_STRING_SLICE
)

type (
Expand Down Expand Up @@ -82,6 +82,25 @@ func (itemset *ItemSet) GetAll() []*Item {
return itemset.items
}

func convertSlice[T int | string | any](key, value any) ([]T, error) {
var slice []T
if !utils.IsAnySlice(value) || len(value.([]any)) == 0 {
return slice, errno.ERR_CONFIGURE_VALUE_REQUIRES_NONEMPTY_SLICE
}
anySlice := value.([]any)
switch anySlice[0].(type) {
case T:
for _, str := range anySlice {
slice = append(slice, str.(T))
}
default:
return slice, errno.ERR_UNSUPPORT_CONFIGURE_VALUE_TYPE.
F("%s: %v", key, value)
}

return slice, nil
}

func (itemset *ItemSet) Build(key string, value interface{}) (interface{}, error) {
item := itemset.Get(key)
if item == nil {
Expand Down Expand Up @@ -135,34 +154,12 @@ func (itemset *ItemSet) Build(key string, value interface{}) (interface{}, error
return v, nil
}

case REQUIRE_SLICE:
anySlice := value.([]any)
if len(anySlice) > 0 {
switch anySlice[0].(type) {
case string:
return convertSlice[string](value), nil
case int:
return convertSlice[int](value), nil
case bool:
return convertSlice[bool](value), nil
default:
return []any{}, errno.ERR_UNSUPPORT_CONFIGURE_VALUE_TYPE.
F("%s: %v", key, value)
}
}
return []any{}, nil
case REQUIRE_STRING_SLICE:
return convertSlice[string](key, value)

default:
// do nothing
}

return value, nil
}

func convertSlice[T int | string | any](value any) []T {
var slice []T
for _, str := range value.([]any) {
slice = append(slice, str.(T))
}
return slice
}
Copy link

Choose a reason for hiding this comment

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

.

The code patch adds a new function convertSlice[T int | string | any](key, value any) ([]T, error). It looks like it is used to convert any type of slice.

The main purpose of this patch is to add support for REQUIRE_STRING_SLICE type in the Build() function. However, it is possible that the convertSlice() function can be used in other parts of the code.

The code looks correct and there are no potential bugs. However, it may be improved by adding more documentation and comments to better explain the purpose of the function. Additionally, it may be useful to add unit tests for the added code.

Copy link

Choose a reason for hiding this comment

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

our code review

The patch looks ok. The only thing I would suggest is to add more comments to explain the logic behind the new code. Additionally, it would be better if you could provide more context about the changes and why it was done. For example, why did you decide to use convertSlice instead of REQUIRE_SLICE?

Apart from that, I think the code is well written and should work without any major issues.

Copy link

Choose a reason for hiding this comment

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

the review:

The patch looks okay, but there are some improvements that can be made. First, the type of value passed to convertSlice should be checked to make sure it is an array before attempting to cast it. Second, the return type of convertSlice should also be checked to ensure that it is of the correct type (int, string, or any). Finally, the error codes for errno should be updated to match the new function name.

9 changes: 9 additions & 0 deletions internal/configure/disks/dc_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ func (dc *DiskConfig) getInt(i *comm.Item) int {
return v.(int)
}

func (hc *DiskConfig) getBool(i *comm.Item) bool {
v := hc.get(i)
if v == nil {
return false
}
return v.(bool)
}

func (dc *DiskConfig) getStrSlice(i *comm.Item) []string {
v := dc.get(i)
if v == nil {
Expand All @@ -65,6 +73,7 @@ func (dc *DiskConfig) getStrSlice(i *comm.Item) []string {

func (dc *DiskConfig) GetContainerImage() string { return dc.getString(CONFIG_GLOBAL_CONTAINER_IMAGE) }
func (dc *DiskConfig) GetFormatPercent() int { return dc.getInt(CONFIG_GLOBAL_FORMAT_PERCENT) }
func (dc *DiskConfig) GetServiceMount() bool { return dc.getBool(CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE) }
func (dc *DiskConfig) GetHost() []string { return dc.getStrSlice(CONFIG_GLOBAL_HOST) }
func (dc *DiskConfig) GetDevice() string { return dc.getString(CONFIG_DISK_DEVICE) }
func (dc *DiskConfig) GetMountPoint() string { return dc.getString(CONFIG_DISK_MOUNT_POINT) }
Copy link

Choose a reason for hiding this comment

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

the code review

  1. The code looks good and is easy to read.
  2. The function getBool() was added which is a good thing as it will help in getting a boolean value from the config.
  3. The function getStrSlice() was also added which is also a good thing as it will help in getting a string slice from the config.
  4. The function GetServiceMount() was also added which is a good thing as it will help in getting a boolean value from the config.
  5. There are no obvious bugs in the code, however it would be better to add some comments to the code for better understanding.
  6. It would be better to do some error handling in case of invalid input.

Copy link

Choose a reason for hiding this comment

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

This code patch appears to be valid and should not contain any bugs. However, there is a lack of documentation on the purpose of the new getBool method which makes it difficult to understand how it contributes to the overall functionality. Adding more comments to explain the intent of the method will be helpful. It would also be beneficial to add some tests to ensure that the new code works correctly.

Copy link

Choose a reason for hiding this comment

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

with code review:

  1. The code looks good and the function getBool() is correctly implemented to return false when the value is nil.
  2. However, there is no check on the type of the value returned in each function. For example, if the return value of getString() is not of type string it will lead to a runtime exception. Therefore, it is suggested to add type checks to each getter function.
  3. The naming convention for the functions could be improved. For example, getBool could be renamed to something like getBoolValue().
  4. Also, it would be better to use descriptive variable names instead of single characters like 'v'.

Expand Down
12 changes: 10 additions & 2 deletions internal/configure/disks/dc_item.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,20 @@ var (
DEFAULT_FORMAT_PERCENT,
)

CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE = itemset.Insert(
common.DISK_SERVICE_MOUNT_DEVICE,
comm.REQUIRE_BOOL,
false,
false,
)

CONFIG_GLOBAL_HOST = itemset.Insert(
common.DISK_FILTER_HOST,
comm.REQUIRE_SLICE,
comm.REQUIRE_STRING_SLICE,
false,
nil,
)

CONFIG_DISK_DEVICE = itemset.Insert(
common.DISK_FILTER_DEVICE,
comm.REQUIRE_STRING,
Expand All @@ -70,7 +78,7 @@ var (

CONFIG_DISK_HOST_EXCLUDE = itemset.Insert(
common.DISK_EXCLUDE_HOST,
comm.REQUIRE_SLICE,
comm.REQUIRE_STRING_SLICE,
false,
nil,
)
Copy link

Choose a reason for hiding this comment

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

the code review.

Firstly, I would suggest to check the type of the variables declared in the code patch. For example, the variable CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE should be set as a boolean value, while the variables CONFIG_GLOBAL_HOST and CONFIG_DISK_HOST_EXCLUDE should be set as string slices.

Secondly, I would suggest to check the order of the variables declared in the code patch. It is better to have all the variables of the same type grouped together.

Thirdly, I would suggest to check if there are any unnecessary lines of code in the code patch. If yes, then remove them to reduce the complexity of the code.

Finally, I would suggest to check for any potential bugs that could arise due to the changes made in the code patch. For example, if the changes made in the code patch could lead to an unexpected result or an error, then appropriate measures should be taken to fix it.

Copy link

Choose a reason for hiding this comment

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

with the code review

  1. The patch adds a new variable CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE with a bool value, this is fine.
  2. The patch changes the data type of CONFIG_GLOBAL_HOST to comm.REQUIRE_STRING_SLICE, this is also fine.
  3. The patch changes the data type of CONFIG_DISK_HOST_EXCLUDE to comm.REQUIRE_STRING_SLICE, this is also fine.
    Overall, the patch looks good and there are no bugs or risks that can be seen from it. However, you should make sure that all the variables and values used in the code are correct and valid.

Copy link

Choose a reason for hiding this comment

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

with code review

  1. The code patch adds a new configuration item "CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE" with a boolean type, which is missing in the original code. This item should be added to provide more flexibility for users to customize.
  2. For CONFIG_GLOBAL_HOST, the data type is changed from comm.REQUIRE_SLICE to comm.REQUIRE_STRING_SLICE, which should improve the accuracy of the data.
  3. For CONFIG_DISK_HOST_EXCLUDE, the data type is changed from comm.REQUIRE_SLICE to comm.REQUIRE_STRING_SLICE, which should improve the accuracy of the data.

In conclusion, this code patch is well structured and should improve the accuracy of the data types. No bug risks have been identified.

Expand Down
Loading