Feature: support direct disk mounting for service container

[legionxiong]

Disk UUID will be passed to container Cmd config during curvebs deployment if disk service_mount_device was enabled, then service(chunkserver) cloud mount disk via entrypoint.sh and restart automatically after host power recover.

to #147 and #165

coordinate with curve pull request opencurve/curve#2285

global:
  format_percent: 95
  container_image: opencurvedocker/curvebs:v1.2
  service_mount_device: true # disk device will be mounted by service containers of curvebs if set "true", default "false"
  host:
    - curve-1
    - curve-2
    - curve-3

disk:
  - device: /dev/sdb1
    mount: /data/chunkserver0
  - device: /dev/sdc1
    mount: /data/chunkserver1
    format_percent: 90
  - device: /dev/sdd1
    mount: /data/chunkserver2
    exclude:  # for the use case that some hosts have not certain disk device
      - curve-3
  - device: /dev/sde1
    mount: /data/chunkserver3
    host:
      - curve-1
      - curve-2

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

.

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

with a brief code review.

1. The mergeFinalHost function is missing error handling, it should return an error value if host to be excluded is not found in the global host list.
2. A comment can be added to the GetDiskId function to explain what kind of disk uri is valid.
3. The ParseDisks function is no longer needed a parameter curveadm *cli.CurveAdm, this should be removed.
4. A comment can be added to describe the purpose of the NewDiskConfig function.
5. The checkDupHost function can have some more comments on its purpose and also to explain the logic it is using.

[cr-gpt]

by looking at the code patch

The code patch seems to have made some modifications to the struct FormatConfig. The changes include the addition of two variables FromDiskRecord and ServiceMountDevice, as well as changes to existing variables ContainerImage, Host, Device, MountPoint, and FormtPercent. It looks like the intent of these changes is to add more options for configuring the formatting of disks.

It is difficult to say whether or not there are any bugs present in the code without performing a more thorough code review. However, it is important to make sure that the changes made make sense and are properly implemented. It is also important to make sure that any new variables are properly documented and used in the appropriate places.

In terms of improvement suggestions, it would be helpful to make sure that any new variables are properly validated, and that any changes made to existing variables do not break existing code. Additionally, it would be useful to make sure that any changes are tested thoroughly before being released into production.

[cr-gpt]

with the code review

1. The code patch looks valid and there are no bugs present.
2. Suggestion: use a more descriptive variable name for REQUIRE_STRING_SLICE, such as REQUIRE_STRING_ARRAY.
3. Suggestion: use a more descriptive variable name for DEFAULT_REPORT_USAGE, such as DEFAULT_USAGE_REPORT.

[cr-gpt]

the review

1.The code looks syntactically correct.
2.For better readability, it is advisable to add comments about the purpose of adding these lines.
3.In line 544, "disk device is unformatted" instead of "disk device not formatted" should be used for clarity.
4.It is recommended to add tests to ensure that the code is behaving properly.
5.It is also recommended to add error handling to ensure that any errors are handled properly.

[cr-gpt]

with the code review:

1. The code patch adds a new column 'service_mount_device' to the table 'disk' and also adds this field to the INSERT and UPDATE queries. This is correct and helps in data integrity.

2. The code patch also changes the column name 'container_image_location' to 'container_image'. This could be done for better readability, but it should be tested to make sure there are no bugs due to this change.

3. The code patch also adds the 'service_mount_device' field to the INSERT and UPDATE queries. This is correct and helps in data integrity.

4. The code patch also changes the query for SET_DISK to include the new field. This is again correct and helps in data integrity.

5. The code patch also changes the query for SET_DISK_URI to include the new field. This is again correct and helps in data integrity.

Overall, the code patch looks correct and helps in data integrity. However, it should be tested to make sure there are no bugs due to the changes made.

[cr-gpt]

with the code review:

1. The code patch adds a new field, ServiceMountDevice, to Disk struct. This field is used to store the service mount device information.
2. In SetDisk function, the new field is added in the INSERT_DISK query and in the SET_DISK query.
3. In GetDisk function, the new field is added to the SQL query result.
4. In UpdateDiskURI function, no changes are made.

Overall, the code patch looks good and there are no major bugs or risks. However, I suggest adding an additional validation check to the SetDisk function to ensure that the ServiceMountDevice parameter is valid before inserting it into the database.

[legionxiong]

Add an UT disk_test.go.

[cr-gpt]

form the code review.

1. The code looks good, it is well organized and easy to read. It follows standard coding conventions and there are no syntax errors.

2. The code is using if-else conditions correctly which will ensure that the program runs as expected.

3. The code is using comments correctly which helps in understanding the code better.

4. The code could use some improvements, like adding more checks to ensure that the data is valid before processing it. This would help in preventing any unexpected errors in the future.

5. The code could also benefit from using better variable names and a more descriptive naming convention. This would improve readability and make the code easier to maintain.

[cr-gpt]

1. The code looks correct, and it seems to add a new step for cleaning disk device of the given service id.
2. Since the code only supports one disk device per service, it is better to put a check before the code and make sure there is only one disk device associated with the service id.
3. Also, when mounting the disk device back to host, it is necessary to check if the mount point exists and create it if not.
4. Besides, please add log message at the beginning and end of the code when the disk device is being cleaned.

[cr-gpt]

with the code review

The code patch looks good and does not contain any obvious bugs. However, there are some improvements that can be made.

First of all, it would be better to separate the logic for mounting disks in a separate function, as this is something that could be reused in other parts of the code. Additionally, the parameters to the getMountVolumes() and getRestartPolicy() functions should be updated to include the serviceMountDevice boolean.

Another suggestion would be to use a switch statement instead of if statements for the role. This will make the code more readable and easier to maintain in the future.

Finally, it would be best to add some comments to explain what each block of code is doing, as this will help other developers who may need to work on the code in the future.

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

[cr-gpt]

with Code review

1. Check indentation, spacing, readability of the code
2. Confirm that the functions and variables are named properly
3. Confirm that the imported libraries are necessary
4. Check for missing return statements
5. Check for any potential vulnerabilities (e.g. SQL injection)
6. Confirm that the code is performant and efficient
7. Confirm that the code is well tested
8. Confirm that the code follows security best practices

[cr-gpt]

the code review:

1. The code looks well organized and easy to read.
2. Good practice to add comments in code to explain the purpose of each block of code.
3. There is no obvious bug risk in this code.
4. Suggestion for improvement: Consider adding validations for all input parameters to ensure their values are within an acceptable range.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

the code review:

1. The function "mergeFinalHost" should return an error type to indicate whether the exclusion of host is successful or not.
2. Check if the host to be excluded is a member of global host list before exclude it.
3. The function "returnInvalidDiskUriError" should be declared outside the "GetDiskId" function, this will make the code more readable.
4. The function "GetDiskId" should return an error type to indicate whether get disk id is successful or not.
5. Add log statement in the function "ParseDisks" to track the process.

[cr-gpt]

with the code review

1. The code seems to have a logical flow and is easy to read.
2. The use of indentation and spacing is good and consistent.
3. There is an additional field "ServiceMountDevice" added to the struct FormatConfig. We need to ensure that this field is used properly and checked for any edge cases.
4. The spelling of one field in the struct has been corrected from ContainerIamge to ContainerImage.
5. There is an additional boolean field "FromDiskRecord" added to the struct FormatConfig to indicate if the disk record should be used or not. This field should be checked for any errors or edge cases.
6. The code is well commented and easy to understand.

[cr-gpt]

with a brief code review

The patch you provided changes the type of a constant from REQUIRE_SLICE to REQUIRE_STRING_SLICE. This indicates that we are now expecting a slice of strings rather than a regular slice. This change should be thoroughly tested to ensure that it doesn't cause any unexpected behavior, and that it still provides the expected results. In addition, any other code that relies on the value of this constant should also be checked to ensure that it is not affected by the change. Finally, it would be beneficial to document this change to ensure that all future developers are aware of it.

[cr-gpt]

with code review

1. The first line of the patch adds a new error code, which is a good practice to ensure that errors are properly tracked and handled.

2. The second line changes the existing error code from "disk device not formatted" to "disk device is unformatted". This change helps to make the message more descriptive and easier to understand for the user.

3. The code looks well-structured and all necessary fields have been included. Additionally, there are no bugs or potential risks in the code.

Overall, the code looks good and is ready for deployment.

[cr-gpt]

with the first patch

The first patch adds a new field service_mount_device to the disk table. This is a good addition as it will allow us to track which devices are being used for the service mount. The data type for this field should also be checked to ensure that it is appropriate for the application.

The second patch updates the SET_DISK query to include the new service_mount_device field and updates the SET_DISK_URI query to include the uri field. This looks good, but it should be tested to make sure that the queries are working correctly and that the data is being updated correctly.

[cr-gpt]

with a code review:

1. The patch adds an additional ServiceMountDevice field to the Disk struct, which should be used to store the device id used by the service for mount the disk.

2. The SetDisk function has been updated to accept an additional argument for serviceMountDevice.

3. The UpdateDiskURI function has been updated to include a check for the existence of the ServiceMountDevice field.

4. The GetDisk function has been updated to include the ServiceMountDevice field in the query results.

Overall, the patch looks good. There are no obvious bugs or risks. However, it may be worth adding a comment to the SetDisk and UpdateDiskURI functions explaining why the ServiceMountDevice field is being used.

[cr-gpt]

with the code review:

1. The code looks well organized and easy to read.
2. There is a check for the "ServiceMountDevice" flag that determines whether or not to write the disk UUID into /etc/fstab for host mounting.
3. The code also checks for the "FromDiskRecord" flag, which is used to update the disk size and uri (diskId) when using disk records committed by disks yaml.
4. No bugs were spotted in the code.
5. No improvement suggestions were identified.

[cr-gpt]

with the code review

1. The code patch has added a step to clean disk service id, which is a good practice.
2. An additional step was added to mount the disk device back to the host for data cleaning. This is also a good practice.
3. Make sure to add proper checks and validation for all the input parameters.
4. To avoid any potential issues, check for potential errors/exceptions when performing operations on the disk.
5. Make sure to properly log errors or exceptions.

[cr-gpt]

with the Code Review.

1. The code appears to be importing the necessary packages for the task, and is adding steps to the task. This is good.
2. It appears that the code is attempting to unmount a disk device from host, which will then be mounted in service (chunkserver) container. This is also good, as it ensures the disk device is not being used elsewhere.
3. However, it is unclear why the device is not being unmounted in the same step as the mount occurs. It may be worthwhile to add a step to unmount the device before the mount step is executed.
4. Additionally, there appears to be no error handling for the umount step. It would be worth adding some form of error handling to ensure that the umount step is successful before proceeding with the rest of the task.
5. Furthermore, it may be worthwhile to add an additional check to ensure that the disk device is actually found before attempting to unmount it. This could be done by adding a "IgnoreNotFound" flag to the UmountFilesystem step.

Overall, the code looks good, but there are a few areas where improvements can be made to ensure the task is successful.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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.

[cr-gpt]

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

[cr-gpt]

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.

[cr-gpt]

to do a brief code review of the patch.

Firstly, I noticed that in line 69, the variable type declared for the hostExcludeMap variable is incorrect, it should be a map[string]bool instead of a map[string]string.

Secondly, in line 75, a check should be added to check if the host to be excluded is actually a member of the global host list, otherwise an error should be returned.

Thirdly, in line 97, the GetDiskId function should take the disk as an argument.

Fourthly, in line 110, the return statement for the GetDiskId function should include the diskUriProto and an error.

Lastly, in line 137, the mergeFinalHost function should be called and an error should be checked for.

I hope this helps.

[cr-gpt]

with a brief overview of the patch.

This patch is for a type declaration for the FormatConfig struct in the code. The patch adds a new field, ServiceMountDevice, and modifies the existing fields ContainerImage, Host, Device, MountPoint, FormatPercent, and FromDiskRecord. This appears to be for a configuration for formatting a disk or device.

In terms of code review, I would recommend making sure that all of the changes are necessary and that they are correctly implemented. Additionally, the naming conventions should be checked to ensure that everything is consistent. Any comments or documentation should also be checked to make sure that it accurately reflects the changes and provides enough information for other developers. Finally, if there are any performance or security considerations, those should be taken into account.

[cr-gpt]

with a brief code review

First of all, in this patch, the line at -32 and +32 have been changed from REQUIRE_SLICE to REQUIRE_STRING_SLICE. This change is valid as it ensures that only string values will be accepted in the slice.

However, there may be a risk that changing the type from a generic slice to one that only accepts strings could cause unexpected issues. It would be best to test the code to ensure that the changes don't break existing functionality.

In addition, there is a new constant at line 35, DEFAULT_REPORT_USAGE, which has a default value of true. This could lead to unnecessary overhead when running the code, so it should be tested to make sure that it is necessary and not causing any performance issues.

[cr-gpt]

the code review:

1. The first snippet of code adds a new error code to the existing list of error codes. This is a valid addition and all the error codes should have a unique code with a descriptive message.

2. The second snippet of code changes the error message for an existing error code. The change is valid as it provides a more descriptive message for the same error.

[cr-gpt]

1. Checking the variable declarations:
   The variable SET_DISK has been modified to add an extra variable service_mount_device, which is required to store the value of the device to mount the service. The variable SET_DISK_URI has also been modified to include an extra parameter for the uri. These changes look valid and there should be no issues with these.

2. Checking the query syntax:
   The syntax of the queries looks valid and there are no issues with it.

3. Checking for any potential bugs or security risks:
   There are no potential bugs or security risks that I can see in the code patch.

4. Suggestions for improvement:
   I suggest adding comments to the code to make it more readable and understandable. Additionally, I suggest adding validation checks for all the parameters being passed to the queries. This will help to prevent malicious input from being passed to the database.

[tsonglew]

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 ?

[legionxiong]

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.

[tsonglew]

Should const REQUIRE_STRING_SLICE in dc_item.go be removed since const REQUIRE_STRING_SLICE in item_set.go covers most cases ?

[legionxiong]

Yeah, it is unnecessary.

[Wine93]

LGTM!

[tsonglew]

LGTM!