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 #194

Closed
wants to merge 1 commit into from
Closed

Feature: support direct disk mounting for service container #194

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
71 changes: 39 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,43 @@ 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)
key := strings.Join([]string{host, dc.GetDevice()}, HOST_DEVICE_SEP)
newDiskMap[key] = true
if dc.GetServiceMount() {
serviceMountDevice = 1
}
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: dc.GetDevice(),
Size: comm.DISK_DEFAULT_NULL_SIZE,
URI: comm.DISK_DEFAULT_NULL_URI,
MountPoint: dc.GetMountPoint(),
ContainerImage: dc.GetContainerImage(),
FormatPercent: dc.GetFormatPercent(),
ChunkServerID: comm.DISK_DEFAULT_NULL_CHUNKSERVER_ID,
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 +135,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 +158,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) {
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
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. It looks like a variable serviceMountDevice was added in line 98, this variable should be initialized to 0 rather than an arbitrary number.

  2. In the function writeDiskRecord the lines if diskRecords, err := curveadm.Storage().GetDisk(common.DISK_FILTER_DEVICE, dr.Host, dr.Device); err != nil { and } else if len(diskRecords) == 0 { should be removed as they are redundant.

  3. In the function syncDiskRecords a check should be added to ensure that the number of new disk records is equal to the number of old disk records before confirming if the user wants to continue.

  4. The variable keySep in the function assambleNewDiskRecords should be declared as a constant as it has a fixed value.

  5. The code should be formatted consistently across the patch to improve readability.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Done.
  2. Done.
  3. Already done.
  4. Done.
  5. Already done, the code are formatted consistently by VSCode.

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.The constant HOST_DEVICE_SEP is added to replace the string ":" in the code, which is more versatile and easy to maintain;
2.The code in the assambleNewDiskRecords() function adds a variable serviceMountDevice to indicate whether the device is used to mount services;
3.The writeDiskRecord() function has been modified to call curveadm.Storage().SetDisk() directly;
4.Added a comparison of the length of newDiskRecords and oldDiskRecords to determine whether to prompt for confirmation before writing new 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.

Already done.

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
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

The code looks good and there are no obvious bugs. However, it would be better to add some comment in the code to provide a better explanation of why ServiceMountDevice is being set to true. Additionally, it would be helpful to add an error handling block if the ServiceMountDevice field is not valid. This can help to detect any errors at runtime and prevent any unexpected behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

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 well commented. The addition of the comment describing what "ServiceMountDevice" is, is helpful to understand the purpose of the code.
  2. The logic seems correct and there do not appear to be any risk of bugs.
  3. To improve the code, it could be broken into smaller, more readable chunks and additional comments could be added to explain the purpose of each block of code. Additionally, if the code is intended for a public repository, it should include additional documentation for other developers who may use the code.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

continue
}
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 patch review:
The code looks fine and does not have any bugs. However, there are a few improvements that can be made:

  1. Add additional comments to explain the purpose of the code and increase the readability.
  2. Modify the conditions for checking the value of dr.ServiceMountDevice and dr.ChunkServerID to make it more accurate.
  3. Refactor the code to make it more efficient.

legionxiong marked this conversation as resolved.
Show resolved Hide resolved
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 seems to be correctly written and follows good coding conventions.
  2. It is always a good practice to add comments to explain the code logic, which will help others to understand the code better.
  3. To improve the readability of the code, it is recommended to add blank lines in between logical blocks of code.
  4. There is no obvious bug risk in this code patch since the code is well written.

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 of the above code.

  1. The code is written in a readable manner and is well commented which makes it fairly easy to understand.
  2. The code is checking for ServiceMountDevice value and if it is not 0, then it is setting the ServiceMountDevice flag to true. This could be improved by using an enum or constant for the ServiceMountDevice value instead of using a hard coded integer value.
  3. There is a check for the ChunkServerID and if it is not equal to the default null value, it skips formatting the disk. This check could be improved by adding a comment or description explaining why it's necessary to skip formatting the disk with a nonempty chunkserver id.
  4. There is a potential bug risk in that the code is not checking for errors when attempting to get the disk info. This should be added to ensure that any errors are properly handled.

In summary, the code looks good but could be improved by adding better comments and checks for errors.

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) container of curvebs if set "true", default "false"
host:
- 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.

the code review

  1. The code looks correct and it should work as expected.
  2. However, it's better to add comments to explain what the new configuration does.
  3. The value of the service_mount_device parameter can be set to true or false. It is better to add a comment explaining the behaviour when each value is set.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

- 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 fo topology chunkserver service
- device: /dev/sdc1
mount: /data/chunkserver1
format_percent: 90
format_percent: 90 # use a different value of 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 tht 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.

the code review:

First of all, it is important to check that the syntax of the code is correct and there are no errors. Additionally, it is important to ensure that the code is understandable, efficient, and secure.

In this code patch, it appears that the formatting of the code is correct and the syntax is valid.

In terms of security, it is important to ensure that all user input is validated and that the code does not contain any vulnerabilities. In the code patch, it appears that all user input is taken into account, and there are no potential security vulnerabilities present.

The code also appears to be efficient, as it is written in a way that allows the program to perform its functions with minimal resource usage.

Finally, it is important to ensure that the code is understandable. In this code patch, the comments provided make the code easy to understand and explain the purpose of each line.

Overall, it appears that the code patch is well written and does not contain any issues.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

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 review.

  1. The indentation for the code looks good, but there are some extra spaces after and before the hyphens in the disk section that should be removed.

  2. The comments are very helpful, especially for the configuration fields that require specific values. However, it would be useful to add more comments to explain why certain values are used and what they mean.

  3. There are no obvious bugs in the code, but it could use some improvements. For example, the exclude field should be moved to the global section so that it applies to all disk devices. Also, the service_mount_device field should be added to the disk section so that it can be set on a per-device basis. Finally, the format_percent field should be moved to the disk section since it is only applicable to disk devices.

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 looks good and should work as intended.
  2. The code is concise and easy to read.
  3. The code does not have any obvious errors or typos.
  4. I suggest adding comments to make the code easier to understand and maintain.
  5. The indentation of the code looks good and consistent.
  6. I recommend adding a test case to ensure the code works as expected.

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 'service_mount_device' field should have a default value to avoid potential issues if the field is not set.
  2. The 'exclude' field should be used carefully and should be checked if it is necessary to exclude a host.
  3. The 'host' field should also be used carefully and should be checked if it is necessary to override the global host config for certain devices.
  4. The formatting percentage for each device should be checked and modified if necessary.
  5. The comments should provide more information about the usage of the fields and the purpose of the settings.

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 looks good, and there are no obvious bugs or issues. The new service_mount_device flag looks useful and is consistent with the rest of the configuration. The new exclude flag looks useful too, allowing users to exclude hosts from configuration.

There are some minor improvements that could be made. Firstly, it would be helpful to include a brief comment explaining what each field of the configuration does. Secondly, the formatting of the code could be improved by making sure all lines are indented correctly and using spaces instead of tabs.

Overall the code looks good and should be safe to use.

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 a brief review
Firstly, it looks like the DISK_EXCLUDE_HOST constant is not being used at all. It should be removed to avoid any confusion.

Secondly, DISK_SERVICE_MOUNT_DEVICE has been added which seems to be related to the service and mount constants. This is fine but you should provide more information about why this is needed.

Lastly, DISK_FORMAT_PERCENT has been moved from the end of the list to the middle. This could lead to some confusion in code readability as the constants are ordered in an unusual way. It would be better to put the constants into some logical order.

Overall, the code patch looks good and the changes appear to be valid. However, more information should be provided to explain why the changes were made.

I hope this helps!

Copy link
Author

@legionxiong legionxiong Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  1. DISK_EXCLUDE_HOST is being used here:
CONFIG_DISK_HOST_EXCLUDE = itemset.Insert(
		common.DISK_EXCLUDE_HOST,
		comm.REQUIRE_SLICE,
		false,
		nil,
	)
  1. The comment of DISK_SERVICE_MOUNT_DEVICE is in the actual code.

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:

The patch adds a new constant DISK_SERVICE_MOUNT_DEVICE to the existing list of constants. Its purpose is unclear but it seems to be related to the existing constant DISK_FILTER_MOUNT and DISK_FILTER_SERVICE.

The patch also renames the existing constant DISK_FORMAT_PERCENT to DISK_FORMAT_PERCENT. This ensures that there are no conflicting names in the list of constants.

Finally, the patch also adds two new constants DISK_FORMAT_MOUNT_POINT and DISK_FORMAT_CONTAINER_IMAGE which appear to be related to disk formatting.

Overall, the patch looks to be well thought out and technically sound. No obvious bugs or risks have been identified. There may be opportunities for improvement, such as adding additional documentation to explain the purpose of the new constants.

Copy link

Choose a reason for hiding this comment

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

.

The patch adds two constants DISK_SERVICE_MOUNT_DEVICE and DISK_FORMAT_PERCENT to the existing list of constants. The constants have been added in the right order(alphabetical) and the formatting is in place.

However, a brief code review suggests that there could be some bug risks in this patch. For example, if the constants are used in some other part of the code without proper initialization, then it can lead to bugs. Additionally, if the constants are used in comparison with other constants, then it could lead to unexpected results.

Also, the naming of the constants needs to be checked. The names should be self-explanatory and should convey the purpose of the constant.

Lastly, it would be better to add proper comments to the constants so that other developers can understand the purpose of the constants quickly.

legionxiong marked this conversation as resolved.
Show resolved Hide resolved
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 added line, DISK_SERVICE_MOUNT_DEVICE = "service_mount_device" seems to be missing a comma after it.
  2. It's not clear from the context of the code what this constant is used for so it's hard to assess if the name DISK_SERVICE_MOUNT_DEVICE accurately describes its purpose.
  3. The existing line, DISK_FORMAT_PERCENT = "format_percent" could be improved by using a more descriptive constant name, such as DISK_FORMAT_PERCENTAGE. This would make it easier to understand the purpose of the constant at a glance.

Copy link

Choose a reason for hiding this comment

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

:

  1. The variable names are descriptive, which is good.
  2. The code looks well-formatted and easy to read, that's great.
  3. The code adds two new constants, DISK_SERVICE_MOUNT_DEVICE, DISK_FORMAT_PERCENT, which is good.
  4. There is no bug risk identified in this patch.
  5. No improvement suggestion for 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.

  1. The code contains a bug, in the convertSlice[T int | string | any](key, value any) ([]T, error) function, the switch statement should use T instead of anySlice[0].(type).
  2. Another improvement recommendation is to add comments to explain the purpose of the convertSlice function.

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.

with the code review

The code looks good, but there are a few points to consider.

  1. Proper naming of variables should be used to improve readability.
  2. The error handling should be improved to catch any unexpected errors.
  3. Make sure to use comments to explain the logic of the code.
  4. Make sure the code is properly tested for any bugs or unexpected behavior.
  5. Consider using constants for any hardcoded values instead of directly inserting them in the code.

Copy link
Author

Choose a reason for hiding this comment

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

  1. The variables IS named properly.
  2. It has value type checking.
  3. The code logic is simple.
  4. self-tested.
  5. Proper constants are used.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need some unit tests according to the 4th suggestion

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we need some unit tests according to the 4th suggestion

Agree. Does curveadm have a unit test framework? We indeed need to add some UT with every change.

Copy link
Collaborator

@Wine93 Wine93 Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we need some unit tests according to the 4th suggestion

Agree. Does curveadm have a unit test framework? We indeed need to add some UT with every change.

We use go built-in unit testing and testify package to testifying our codes, there are a few test code:

We are so much lacking of unit tests and integration tests, it leads the curveadm often unstable. It would be a huge improvement if we had unit tests and integration tests to guarantee curveadm stability. but then again, i think the integration testing is not so easy to implement.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I noticed these test files. I thought they were just part of checker.

Copy link
Author

Choose a reason for hiding this comment

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

That's true, integration test is more complicated, especially the automatic one. Some mocking mechanism need to be introduced for curveadm.

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 didn't have any syntax errors.
  2. The code implements a function getBool(), which will be used to get boolean values from the config file, so it can provide a better user experience.
  3. The function getBool() should also have a parameter to specify the variable name, or a reference to the variable to get the value of.
  4. The code should also include proper error handling code in case the variable is not found or the value is not in the expected format.

Copy link
Author

Choose a reason for hiding this comment

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

Already done.

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:

The code patch looks good. The getBool() function has been added to the DiskConfig struct, which is used to retrieve a boolean value from the given item. The GetServiceMount() function uses the new getBool() function to get the service mount device status. Additionally, the GetHost() function has been modified to use the getStrSlice() function instead of the getString() function. This ensures that the host list is retrieved as a slice of strings rather than a single string.

Overall, the code looks good. However, it would be beneficial to add some more tests to ensure that the new getBool() and getStrSlice() functions are working correctly. Additionally, it would be beneficial to have a comment explaining why the changes are being made.

legionxiong marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

code review

The code looks mostly correct, however there are a few points to consider:

  1. Is the getBool function really necessary? Can the getInt function be used instead?
  2. Make sure that the getStrSlice function is properly guarded against nil values.
  3. Consider adding some validation checks for the parameters passed to the functions.
  4. Add some logging or error handling to the functions in case of unexpected scenarios.

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:

The code patch is relatively clean and easy to understand. It is adding a new function which returns a boolean value to the existing DiskConfig class. The name of the new function is getBool, which is appropriate for what it is doing. The code is properly indented and the logic is easy to follow.

There are no obvious bugs in the code. However, it would be better to add some checks to make sure that the value being returned is actually a boolean type before returning it. This could be done by using an assertion or by using type switches.

Overall, the code is well written and should not have any major issues.

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,
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 variable CONFIG_GLOBAL_SERVICE_MOUNT looks like it should be a boolean value, however, the code is setting it to false which does not make sense.
  2. If the variable CONFIG_GLOBAL_HOST is intended as a slice, then it should also require a slice value in the itemset.Insert call.
  3. The CONFIG_DISK_DEVICE variable should have more descriptive name.
  4. The code should check if the values inserted into the itemset are valid before proceeding.

Copy link
Author

Choose a reason for hiding this comment

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

  1. it should be a boolean value, however, the code is setting it to false which does not make sense, an AI bug?
  2. itemset.Insert accepts interface{}.
  3. CONFIG_DISK_DEVICE is not an ambiguous name.
  4. The value is validated in the logic of item.Build.

Copy link
Member

Choose a reason for hiding this comment

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

it should be a AI bug, since there are no such variable called CONFIG_GLOBAL_SERVICE_MOUNT

Copy link
Author

Choose a reason for hiding this comment

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

OK.

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 patch looks good and there are no obvious bugs in the patch.
  2. There is an extra configuration field called CONFIG_GLOBAL_SERVICE_MOUNT added which is useful for enabling or disabling disk service mount.
  3. The code is well organized and properly indented.
  4. The data types of the configuration fields have been appropriately set.
  5. The code patch is properly commented.

Copy link

Choose a reason for hiding this comment

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

The code looks good. It seems that you are adding a configuration setting for the global service mount device. The code looks clean and well written.

As for improvements, I suggest adding a comment above the new configuration setting to explain what it does and why it is needed. This will help make the code more readable and easier to maintain in the future.

If there is any risk of bugs, it would be good to add some unit tests to cover the new code. This will ensure that the new code works as expected and will reduce the risk of introducing bugs.

Overall, this looks like a good patch and I think it should work as expected.

legionxiong marked this conversation as resolved.
Show resolved Hide resolved
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. There is no error checking for the value of CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE, which could lead to potential errors in the program.
  2. The variable names should be more descriptive to help understand the purpose of each variable.
  3. The code can be improved by adding comments to explain the purpose of each variable and what it represents.
  4. It is recommended to use the constants in place of the literals so that the code is more maintainable.

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.

with Code Review:

  1. We can see that the code is trying to set up global variables for the application, and it looks like the variables are being set up correctly according to their specified data types.
  2. It looks like the code is using "itemset.Insert" to set up the variables. This is a good practice as it ensures that the variables are properly initialized and stored in a safe and secure way.
  3. It looks like the code is using the correct data types for each of the variables, such as using "comm.REQUIRE_BOOL" for the CONFIG_GLOBAL_SERVICE_MOUNT_DEVICE variable.
  4. The code also uses a "comm.REQUIRE_STRING_SLICE" for the CONFIG_GLOBAL_HOST and CONFIG_DISK_HOST_EXCLUDE variables. This is good practice, as it ensures that the host and exclude list will contain only strings.
  5. Overall, the code looks good and there does not appear to be any major bugs or risks present. If any improvements are desired, it might be beneficial to add additional comments to explain what each variable is used for. Additionally, it might be beneficial to add more documentation to the code to make it easier to read and understand.

Expand Down
Loading