Ublk #1245
Ublk #1245
Conversation
Signed-off-by: kampadais <csdp1295@csd.uoc.gr>
|
This pull request is now in conflict. Could you fix it @Kampadais? 🙏 |
|
This PR is stale because it has been open for 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
WalkthroughThe changes introduce a new command-line flag Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
pkg/frontend/ublk/frontend.go (2)
163-167: Redundant file existence checksThe code checks for the existence of
socketPathand removes it if it exists. Usingos.Removewithout prior checks is safe since it will return an error if the file doesn't exist, which can be ignored if not critical.Simplify the code by removing the redundant check:
- if st, err := os.Stat(socketPath); err == nil && !st.IsDir() { - if err := os.Remove(socketPath); err != nil { - return err - } - } + _ = os.Remove(socketPath)
240-242: ImplementPingResponsemethod according to the interfaceThe
PingResponsemethod currently returnsnil. If the interface expects specific behavior or for health checking, consider implementing it accordingly or adding a comment explaining why it's a no-op.If no action is needed, you can document the method:
// PingResponse is a no-op for Ublk as no action is required. func (d DataProcessorWrapper) PingResponse() error { return nil }pkg/controller/init_frontend.go (1)
Line range hint
25-36: Update function documentation forNewFrontendThe
NewFrontendfunction signature has changed with the addition of thefrontendQueuesparameter. The function's documentation should be updated to reflect this new parameter, explaining its purpose and how it affects the different frontend types.Add or update the function comment:
// NewFrontend creates a new frontend based on the provided type. // The frontendQueues parameter specifies the number of queues for the frontend, // applicable only to certain frontend types like "ublk". func NewFrontend(frontendType string, iscsiTargetRequestTimeout time.Duration, frontendQueues int) (types.Frontend, error) { // ... }package/Dockerfile (1)
12-13: Install build dependencies in a singleRUNcommandCombining installation commands reduces the number of layers in the Docker image and can improve build efficiency.
Combine the installation commands:
- RUN zypper -n install autoconf automake libtool gcc-c++ - - RUN zypper -n install cmake curl git gcc wget xsltproc docbook-xsl-stylesheets && \ + RUN zypper -n install autoconf automake libtool gcc-c++ cmake curl git gcc wget xsltproc docbook-xsl-stylesheets && \ rm -rf /var/cache/zypp/*app/cmd/controller.go (3)
95-100: Clarify usage information forfrontend-queuesflagThe usage message for the
frontend-queuesflag contains a minor typo and lacks clarity on its applicability.Update the usage message:
cli.IntFlag{ Name: "frontend-queues", Required: false, Value: 1, - Usage: "Number of frontend queues , only available in ublk frontend", + Usage: "Number of frontend queues, applicable only for the 'ublk' frontend", },
132-132: MovefrontendQueuesdeclaration closer to related flagsFor better readability and maintainability, consider declaring
frontendQueuesimmediately after retrieving the frontend-related flags.Move
frontendQueuesretrieval code:frontendName := c.String("frontend") + frontendQueues := c.Int("frontend-queues") isUpgrade := c.Bool("upgrade") disableRevCounter := c.Bool("disableRevCounter")
205-206: UpdateNewControllercall with correct parameter alignmentEnsure that the parameters passed to
controller.NewControllerare correctly aligned and documented, especially after adding the newfrontendQueuesparameter.Double-check the parameter order and update documentation if necessary.
pkg/controller/control.go (1)
69-70: Consider adding performance monitoring for frontend queuesGiven that this change aims to improve IOPS through multiple frontend queues, consider:
- Adding metrics to track per-queue performance
- Implementing adaptive queue scaling based on load
- Adding documentation about optimal queue count configuration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/cmd/controller.go(4 hunks)package/Dockerfile(2 hunks)pkg/controller/control.go(4 hunks)pkg/controller/init_frontend.go(3 hunks)pkg/frontend/ublk/frontend.go(1 hunks)
🔇 Additional comments (5)
pkg/controller/init_frontend.go (1)
35-36: Ensure frontendQueues is applicable for the frontend type
In the case for "ublk", frontendQueues is passed to ublk.New(frontendQueues). For other frontend types, frontendQueues might not be relevant. Consider validating frontendQueues specifically for ublk or documenting its usage to avoid confusion.
Check if frontendQueues impacts other frontend types:
package/Dockerfile (1)
35-50: Verify the availability of repositories and package versions
Ensure that the repositories and specific commit IDs used for liburing and ubdsrv are accessible and compatible. This is crucial for reproducible builds and future maintenance.
Run the following script to check the repositories and commit IDs:
✅ Verification successful
Repository and commit accessibility verified successfully
Both repositories and their specified versions are accessible:
- liburing repository at branch
liburing-2.5was successfully cloned - ubdsrv repository at commit
19d3b2133baf1af8ae3a5fe300c962567fb7b0cewas successfully checked out
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the repositories and commit IDs are accessible.
# Test: Clone liburing repository at specified branch and commit.
git clone --depth 1 --branch liburing-2.5 https://github.com/axboe/liburing.git /tmp/liburing && \
cd /tmp/liburing
# Test: Clone ubdsrv repository at specified commit.
git clone https://github.com/Kampadais/ubdsrv.git /tmp/ubdsrv && \
cd /tmp/ubdsrv && \
git checkout 19d3b2133baf1af8ae3a5fe300c962567fb7b0ce
Length of output: 1660
app/cmd/controller.go (1)
185-186: Handle missing or unsupported frontend types gracefully
When initializing the frontend, if frontendName is provided, ensure that unsupported or invalid frontend types are handled gracefully, possibly with a user-friendly error message.
Check the handling of unsupported frontend types:
✅ Verification successful
Frontend initialization has proper error handling for unsupported types
The code properly handles unsupported frontend types. The NewFrontend function in pkg/controller/init_frontend.go implements a clear switch statement that:
- Explicitly handles supported frontend types: "rest", "socket", "tgt-blockdev", "tgt-iscsi", and "ublk"
- Returns a descriptive error message for any unsupported frontend type:
fmt.Errorf("unsupported frontend type: %v", frontendType)
The error handling is appropriate and user-friendly, making it clear when an invalid frontend type is provided.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the application handles unsupported frontend types.
# Test: Attempt to start the controller with an invalid frontend type.
./longhorn controller --frontend invalid_frontend_name
Length of output: 165
Script:
#!/bin/bash
# Let's examine the NewFrontend function implementation and its error handling
ast-grep --pattern 'func NewFrontend($$$) ($_, error) {
$$$
}'
# Also search for frontend-related error messages and validations
rg -A 5 "unsupported frontend|invalid frontend|frontend.*error" --type go
Length of output: 5548
pkg/controller/control.go (2)
69-70: LGTM: Clean integration of frontendQueues field
The addition of the frontendQueues field and its integration into the constructor is clean and maintains backward compatibility.
Also applies to: 80-80, 100-100
584-587: Verify frontend queue handling implementation
The integration of frontendQueues into the frontend initialization looks correct. However, we should verify that the frontend implementation (particularly ublk) properly handles the queue configuration.
✅ Verification successful
Let me gather more information about the ublk frontend implementation to verify how it handles the queue configuration.
Queue configuration is properly implemented in the ublk frontend
The ublk frontend correctly handles the queue configuration:
- The
Queuesfield is properly defined in theUblkstruct - The queue count is correctly passed to the ublk device during initialization via the
StartUblkmethod using-qparameter - The implementation maintains proper synchronization through socket server handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the ublk frontend implementation for queue handling
# Search for ublk frontend implementation
ast-grep --pattern 'type ublk struct {
$$$
}'
# Search for queue configuration usage
rg -A 5 'frontendQueues'
Length of output: 3317
Script:
#!/bin/bash
# Search for ublk struct definition and its usage of Queues field
ast-grep --pattern 'type Ublk struct {
$$$
Queues $_
$$$
}'
# Search for methods in ublk that might use the Queues field
rg -A 10 "func \(.*Ublk\)" pkg/frontend/ublk/
Length of output: 7365
| if err != nil { | ||
| logrus.Error("Error starting ublk:", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Incorrect error handling in StartUblk
When cmd.CombinedOutput() returns an error, the function logs the error but returns nil, which may mislead the caller into thinking the operation succeeded. It's important to return the actual error to allow proper error handling by the caller.
Apply this diff to fix the error handling:
if err != nil {
logrus.Error("Error starting ublk:", err)
- return nil
+ return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| logrus.Error("Error starting ublk:", err) | |
| return nil | |
| } | |
| if err != nil { | |
| logrus.Error("Error starting ublk:", err) | |
| return err | |
| } |
| go func() { | ||
| u.ShutdownUblk() | ||
| }() |
There was a problem hiding this comment.
Possible race condition in Shutdown method
Spawning a goroutine to call ShutdownUblk() without synchronization might lead to unexpected behavior, especially if the program exits before the goroutine completes. Consider calling ShutdownUblk() directly or synchronizing the goroutine to ensure proper shutdown.
Apply this diff to call ShutdownUblk() directly:
- go func() {
- u.ShutdownUblk()
- }()
+ u.ShutdownUblk()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| u.ShutdownUblk() | |
| }() | |
| u.ShutdownUblk() |
| var jsonOutput map[string]interface{} | ||
| err = json.Unmarshal(output, &jsonOutput) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| u.UblkID = int(jsonOutput["dev_id"].(float64)) | ||
| u.DaemonPId = int(jsonOutput["daemon_pid"].(float64)) | ||
| u.Queues = int(jsonOutput["nr_hw_queues"].(float64)) | ||
| u.QueueDepth = int(jsonOutput["queue_depth"].(float64)) | ||
| u.BlockSize = int(jsonOutput["block_size"].(float64)) | ||
|
|
There was a problem hiding this comment.
Potential panic due to unchecked type assertions
The code uses unchecked type assertions like jsonOutput["dev_id"].(float64) without verifying if the key exists or if the type assertion succeeds. This can lead to a runtime panic if the expected keys are missing or of an unexpected type. It's safer to use the two-value form of type assertions and handle the case where the assertion fails.
Apply this diff to safely handle type assertions:
- u.UblkID = int(jsonOutput["dev_id"].(float64))
+ devIDValue, ok := jsonOutput["dev_id"]
+ if !ok {
+ return fmt.Errorf("'dev_id' not found in ublk output")
+ }
+ devID, ok := devIDValue.(float64)
+ if !ok {
+ return fmt.Errorf("unexpected type for 'dev_id' in ublk output")
+ }
+ u.UblkID = int(devID)
- u.DaemonPId = int(jsonOutput["daemon_pid"].(float64))
+ daemonPIDValue, ok := jsonOutput["daemon_pid"]
+ if !ok {
+ return fmt.Errorf("'daemon_pid' not found in ublk output")
+ }
+ daemonPID, ok := daemonPIDValue.(float64)
+ if !ok {
+ return fmt.Errorf("unexpected type for 'daemon_pid' in ublk output")
+ }
+ u.DaemonPId = int(daemonPID)
# Repeat the pattern for 'nr_hw_queues', 'queue_depth', and 'block_size'Committable suggestion skipped: line range outside the PR's diff.
| if u.Volume == "" { | ||
| panic("Invalid volume name") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using panic for control flow
The GetSocketPath method calls panic("Invalid volume name") if u.Volume is empty. Using panic for control flow is discouraged as it can crash the program. Instead, return an error to be handled appropriately.
Apply this diff to return an error instead of panicking:
func (u *Ublk) GetSocketPath() (string, error) {
if u.Volume == "" {
- panic("Invalid volume name")
+ return "", fmt.Errorf("invalid volume name")
}
return filepath.Join(SocketDirectory, "longhorn-"+u.Volume+".sock"), nil
}Update callers to handle the error:
- socketPath := u.GetSocketPath()
+ socketPath, err := u.GetSocketPath()
+ if err != nil {
+ return err
+ }Committable suggestion skipped: line range outside the PR's diff.
| make installgit clone https://github.com/Kampadais/ubdsrv.git && \ | ||
| cd ubdsrv && \ | ||
| git checkout ${UBD_COMMIT_ID} && \ | ||
| export LIBURING_CFLAGS="-I/usr/include/liburing" && \ | ||
| export LIBURING_LIBS="-Lusr/lib/pkgconfig -luring"cd /usr/src && \ | ||
| ls;autoreconf -i && \ | ||
| ./configure && \ | ||
| make; \ | ||
| make install |
There was a problem hiding this comment.
Fix syntax errors and ordering in ubdsrv build steps
There are syntax errors and misordered commands in the ubdsrv build steps. For example, missing && \ at the end of lines, and misplaced commands. This will cause the Docker build to fail.
Apply this diff to correct the build steps:
RUN git clone --depth 1 --branch liburing-2.5 https://github.com/axboe/liburing.git /usr/src/liburing && \
cd /usr/src/liburing && \
./configure --cc=gcc --cxx=g++ && \
make -j$(nproc) && \
make install
- git clone https://github.com/Kampadais/ubdsrv.git && \
- cd ubdsrv && \
+ && git clone https://github.com/Kampadais/ubdsrv.git /usr/src/ubdsrv && \
+ cd /usr/src/ubdsrv && \
git checkout ${UBD_COMMIT_ID} && \
export LIBURING_CFLAGS="-I/usr/include/liburing" && \
export LIBURING_LIBS="-L/usr/lib/pkgconfig -luring" && \
- cd /usr/src && \
- ls;autoreconf -i && \
+ autoreconf -i && \
./configure && \
make && \
make installCommittable suggestion skipped: line range outside the PR's diff.
|
Adding the PR in backlog milestone so that it won't stale |
Which issue(s) this PR fixes:
Issues longhorn/longhorn#5159 , longhorn/longhorn#6600
I am reposting this PR because i think it is a solid solution on integrating ublk with longhorn updated with more benchmarks on a more real-life enviroment
What this PR does / why we need it:
This PR uses @derekbit 's POC ubdsrv to integrate ublk with longhorn-engine and adds support for multiple frontend queues.
Using ublk as a frontend option significantly boosts the IOPS in the frontend part of the engine. In our tests, we measure up to ~280k IOPS at the frontend (before the controller communicates with the replicas). Those IOPS are bounded because of my setup.
For the benchmarks i used the folowing setup :
CPU : Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz
RAM: 128GB
Disk: Samsung 860 Evo SSD 250GB
Network : 10Gbps
One replica , one controller , 2 machines in a local cluster
On the ublk :
Ublk frontend queues : 6
Controller-Replica connections : 6
The fio command used for testing IOPS:
sudo fio --name=read_iops --filename=/dev/ublkb0 --size=5G --numjobs=12 --time_based --runtime=30s --ramp_time=2s --ioengine=libaio --direct=1 --verify=0 --bs=4K --iodepth=64 --rw=randwrite --group_reporting=1fio command used for testing Bandwidth:
sudo fio --name=bandwidth --filename=/dev/ublkb0 --size=5G --numjobs=12--time_based --runtime=30s --ramp_time=2s --ioengine=libaio --direct=1 --verify=0 --bs=1M --iodepth=64 --rw=read --group_reporting=1*Replica R/W disabled means that the replica code is altered in order to answer dummy replies and not do the actual R/W in the Linux sparse files.
Note that the frontend performance was bounded by my pc's process power. Using a more powerful pc , ublk can achieve close to raw disk speeds. Based on these findings we can see that there is also need to improve other parts of the system too in order to utilize ublk fully.
Special notes for your reviewer:
In order to replicate the results you will need my version of ubdsrv installed: https://github.com/Kampadais/ubdsrv
To run the controller with ublk frontend, 6 frontend queues and 6 replica-streams:
./longhorn controller test --frontend ublk --size 1g --current-size 1g --frontend-queues 6 --replica-streams 6 --replica tcp://....:9502We are still the need of a go-ublk-helper because the whole startup procedure is within the frontend.go file and monitoring status is limited. There is also a limitation regarding the name of the block device provided by ublk.
Additional documentation or context
Summary by CodeRabbit
New Features
frontend-queuesfor specifying the number of frontend queues.ublkfrontend package for managing ublk devices, including methods for initialization, starting, and shutting down the device.Bug Fixes
Documentation
Chores
ubdsrvcomponent.