-
Notifications
You must be signed in to change notification settings - Fork 203
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.
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
Enhance localdisk storage backend to manually add blob objects #1337
Conversation
@jiangliu , a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/80071 |
Codecov Report
@@ Coverage Diff @@
## master #1337 +/- ##
==========================================
+ Coverage 46.01% 46.16% +0.15%
==========================================
Files 122 122
Lines 37807 37913 +106
Branches 37807 37913 +106
==========================================
+ Hits 17396 17503 +107
+ Misses 19501 19490 -11
- Partials 910 920 +10
|
@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/80080 |
@jiangliu , The CI test is completed, please check result:
Congratulations, your test job passed! |
@jiangliu Unit tests on older versions of the localdisk image have passed, which means your code is compatible. For the test code, please refer to https://github.com/adamqqqplay/image-service/tree/localdisk-test Regarding the review of the code itself, I still need some time. Thanks for your work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work, I made some small comments on the code changes, mainly focusing on the maintainability of the module.
@@ -45,7 +45,8 @@ tar = "0.4.38" | |||
regex = "1.7.0" | |||
|
|||
[features] | |||
backend-localdisk = ["gpt"] | |||
backend-localdisk = [] | |||
backend-localdisk-gpt = ["gpt", "backend-localdisk"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about this new feature, can you explain what it's for? I only see one feature on other backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to support localdisk without gpt. Gpt is just one way to discover nydus blobs inside a disk, but not the only way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it.
} else { | ||
v.name.clone() + guid.to_simple().to_string().as_str() // The 64-byte blob_id is stored in two parts | ||
// The 64-byte blob_id is stored in two parts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case should also belong to gpt mode.
The reason for the judgment of part_type_guid above comes from a compatibility issue.
- When part_type_guid == gpt::partition_types::BASIC, the blob_id only exists in the name field of the partition.
- When part_type_guid == gpt::partition_types::LINUX_FS, the blob_id exists in the name field and in the guid field.
For the corresponding changes, please refer to: adamqqqplay/nydus-localdisk@4812dcb . BTW, the design of the image building did have some hacks before. If you have any suggestions, I could refactor them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed your point here. This piece of code is enabled for gpt
only.
For the backward compatibility, it would be better to remove if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will refactor this part later. Now, I think the code might look like this to be compatible.
let name = if v.part_type_guid == gpt::partition_types::BASIC {
is_gpt_mode = true;
// Compatible with old versions of localdisk image
v.name.clone()
} else if v.part_type_guid == gpt::partition_types::LINUX_FS {
is_gpt_mode = true;
// The 64-byte blob_id is stored in two parts
v.name.clone() + guid.to_simple().to_string().as_str()
} else { // **Your new code block. The part_type_guid not found**
// The 64-byte blob_id is stored in two parts
v.name.clone() + guid.to_simple().to_string().as_str()
}
@@ -443,6 +443,9 @@ pub struct LocalDiskConfig { | |||
/// Mounted block device path or original localdisk image file path. | |||
#[serde(default)] | |||
pub device_path: String, | |||
/// Disable discover blob objects by scanning GPT table. | |||
#[serde(default)] | |||
pub disable_gpt: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable disable_gpt has been used here to control the localdisk mode. Is it necessary to use the feature backend-localdisk-gpt
for conditional compilation at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it for simplicity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/81810 |
@jiangliu , The CI test is completed, please check result:
Congratulations, your test job passed! |
I have no other comments, @imeoer please take a look. Thanks! |
Introduce feature `backend-localdisk-gpt` for localdisk storage backend, so it can be optionally disabled. Signed-off-by: Jiang Liu <[email protected]>
Use File instead of RawFd in struct LocalDiskBlob to avoid possible race conditions. Signed-off-by: Jiang Liu <[email protected]>
Enhance the localdisk storage backend, so we can manually add blob objects in the disk, in addition to discovering blob objects by scanning GPT partition table. Signed-off-by: Jiang Liu <[email protected]>
@jiangliu , the code has been updated, so a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/85262 |
@jiangliu , The CI test is completed, please check result:
Congratulations, your test job passed! |
Relevant Issue (if applicable)
If there are Issues related to this PullRequest, please list it.
Details
Types of changes
What types of changes does your PullRequest introduce? Put an
x
in all the boxes that apply:Checklist
Go over all the following points, and put an
x
in all the boxes that apply.