Docker/entrypoint: support disk mounting in service container #2285
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
docker/debian9/entrypoint.sh
Outdated
| [[ ! -d $disk_mount_point ]] && die "disk mount point $disk_mount_point does not exist.\n" | ||
| mount -o rw,errors=remount-ro $g_disk $disk_mount_point | ||
| [[ $? -ne 0 ]] && die "mount disk device $g_disk to $disk_mount_point failed.\n" | ||
| fi | ||
| g_binary="$g_prefix/sbin/curvebs-chunkserver" | ||
| g_start_args="--conf=$conf_path" | ||
| ;; |
There was a problem hiding this comment.
from the code structure,
The code structure looks good with comments and syntax highlighting to make it easier for readers to understand. There are several global variables declared at the beginning, followed by the function definitions and finally the main logic.
In terms of potential bugs, it looks like the code is missing some validation and error-handling. For example, the -d or --disk argument is not being validated and it is not clear what is expected as an input here. Additionally, the code is not checking if the provided device is mounted correctly before using it. It would be a good idea to add some validations and error handling around these parts of the code.
It would also be helpful to add some logging statements to the code to help with debugging. This could include log statements for the disk mount process, for any errors encountered, and for other important events in the program's execution.
Overall, the code looks good and is easy to read. However, it can be improved with some validations and error-handling. Additionally, adding some logging statements would be helpful for debugging purposes.
docker/debian9/entrypoint.sh
Outdated
| [[ ! -d $disk_mount_point ]] && die "disk mount point $disk_mount_point does not exist.\n" | ||
| mount -o rw,errors=remount-ro $g_disk $disk_mount_point | ||
| [[ $? -ne 0 ]] && die "mount disk device $g_disk to $disk_mount_point failed.\n" | ||
| fi | ||
| g_binary="$g_prefix/sbin/curvebs-chunkserver" | ||
| g_start_args="--conf=$conf_path" | ||
| ;; |
There was a problem hiding this comment.
the code review.
From the patch, it seems that you have added some lines of code for role chunkserver, to mount the disk device to a mount point.
The code looks good but there are a few improvement suggestions that can be made.
- Check if the mount point already exists before trying to remount it
- Check if the disk device is already mounted before attempting to mount it
- Add a timeout to the mount command
- Add a force flag to the mount command in case of any errors
- Add a logging statement to track the mount command execution status and any errors
- Add a check to make sure the mount command executed successfully
There was a problem hiding this comment.
- Done.
- The disk device will be mounted in container, and umounted from host by curveadm. It is guaranteed by curveadm.
- Unnecessary for local disk device.
- The service(chunkserver) container should exit if any errors encounted during disk mounting.
- The is tracked by container's log.
- Cannot make sure the mount command to be executed successfully, it may fail.
docker/debian9/entrypoint.sh
Outdated
| [[ ! -d $disk_mount_point ]] && die "disk mount point $disk_mount_point does not exist.\n" | ||
| mount -o rw,errors=remount-ro $g_disk $disk_mount_point | ||
| [[ $? -ne 0 ]] && die "mount disk device $g_disk to $disk_mount_point failed.\n" | ||
| fi | ||
| g_binary="$g_prefix/sbin/curvebs-chunkserver" | ||
| g_start_args="--conf=$conf_path" | ||
| ;; |
There was a problem hiding this comment.
with a brief overview:
This patch adds a new command line parameter, --disk, to the entrypoint.sh script. This parameter is used for chunkserver role and allows for mounting a disk device to a specified mount point. It also adds a global variable, disk_mount_point, which specifies the mount point for the device.
Now let's look at the code review:
- The code checks if the --disk parameter is present and exits with an error message if not.
- The global variable, disk_mount_point, is properly set.
- The code checks if the mount point exists and exits with an error message if it doesn't.
- The code uses the mount command to mount the device.
- The code checks the return value of the mount command and exits with an error message if necessary.
Overall, this patch looks good. There are no obvious bugs in the code, and it should work as intended. However, there are a few areas that could be improved:
- The code does not check if the device being mounted is valid. It should check if the device is accessible and if it is a valid block device before attempting to mount it.
- The code does not check the return value of the mount command to determine if the mount was successful. It should check the return value and print an appropriate error message if the mount fails.
- The code should log all mount operations, including failed attempts. This will help with debugging any issues that may arise.
|
cicheck |
|
@legionxiong continue？ |
|
@legionxiong continue？ |
Signed-off-by: Lijin Xiong <[email protected]>
Resubmitted. |
1 similar comment
Resubmitted. |
|
cicheck |
What problem does this PR solve?
Issue Number: #2283
Problem Summary:
What is changed and how it works?
What's Changed: docker/debian9/entrypoint.sh
How it Works: disk device will be mounted by entrypoint.sh if --disk parameter was injected when deploy curvebs.
Side effects(Breaking backward compatibility? Performance regression?):
Check List