nydus-image: add `backend-config` support to `nydus-image unpack` #1082

loheagn · 2023-02-15T06:35:23Z

The nydus-image unpack only support reading nydus blob from the blob files on local machine (by the argument blob) by now. This patch adds a new argument backend-config to this command to allow nydus-image unpack to read blob data from all kinds of backends.

anolis-bot · 2023-02-15T06:35:32Z

@loheagn , a new test job has been submitted. Please wait in patience. The test job url: https://tone.openanolis.cn/ws/nrh4nnio/test_result/50984

anolis-bot · 2023-02-15T06:39:21Z

@loheagn , the title 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/50989

anolis-bot · 2023-02-15T06:40:33Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		❌ FAIL

Sorry, your test job failed. Please get the details in the link.

anolis-bot · 2023-02-15T06:40:36Z

@loheagn , 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/50990

anolis-bot · 2023-02-15T06:47:52Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS

Congratulations, your test job passed!

anolis-bot · 2023-02-15T06:57:52Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS

Congratulations, your test job passed!

jiangliu · 2023-02-15T07:36:35Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

loheagn · 2023-02-15T07:42:59Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

jiangliu · 2023-02-15T07:48:10Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory
nydus data blobs haven't been downloaded to the local directory

loheagn · 2023-02-15T08:00:20Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory

nydus data blobs haven't been downloaded to the local directory

Yes.

For example, by now, when we use the coverter.Unpack method in nydus-snapshotter to unpack a nydus image layer, we need to extract both the bootstrap part and the blob data part to the local disk first to allow the nydus-image to read to blob data (use the localfs backend). But with this patch, we can just need to extract the bootstrap part and set up a local http server to serve the blob data part. Then the backend-config can be used to pass the information about this local http server.

jiangliu · 2023-02-15T08:29:59Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory

nydus data blobs haven't been downloaded to the local directory

Yes.

For example, by now, when we use the coverter.Unpack method in nydus-snapshotter to unpack a nydus image layer, we need to extract both the bootstrap part and the blob data part to the local disk first to allow the nydus-image to read to blob data (use the localfs backend). But with this patch, we can just need to extract the bootstrap part and set up a local http server to serve the blob data part. Then the backend-config can be used to pass the information about this local http server.

Can we let nydus-image to handle the bootstrap part too?
That's a feature I expect, then nydus-image can process remote images.

loheagn · 2023-02-15T09:21:06Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory

nydus data blobs haven't been downloaded to the local directory

Yes.
For example, by now, when we use the coverter.Unpack method in nydus-snapshotter to unpack a nydus image layer, we need to extract both the bootstrap part and the blob data part to the local disk first to allow the nydus-image to read to blob data (use the localfs backend). But with this patch, we can just need to extract the bootstrap part and set up a local http server to serve the blob data part. Then the backend-config can be used to pass the information about this local http server.

Can we let nydus-image to handle the bootstrap part too? That's a feature I expect, then nydus-image can process remote images.

@jiangliu Yes, I think we can.

We need to implement the logic of extracting the bootstrap and blob data from the whole nydus image layer (which is implemented in nydus-snapshotter by now) and let the caller tell nydus-image the loacation of the image layer by some argument (may be --image-path).

What's your option? @imeoer

jiangliu · 2023-02-15T09:28:16Z

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory

nydus data blobs haven't been downloaded to the local directory

Yes.
For example, by now, when we use the coverter.Unpack method in nydus-snapshotter to unpack a nydus image layer, we need to extract both the bootstrap part and the blob data part to the local disk first to allow the nydus-image to read to blob data (use the localfs backend). But with this patch, we can just need to extract the bootstrap part and set up a local http server to serve the blob data part. Then the backend-config can be used to pass the information about this local http server.

Can we let nydus-image to handle the bootstrap part too? That's a feature I expect, then nydus-image can process remote images.

How about using "-C, --config " instead? We are going to switch from "--backend-config" to "--config" and "--backend-config" will get deprecated.

@jiangliu But I think this will be conflict with the backend in the Config when load rafs. https://github.com/dragonflyoss/image-service/blob/master/rafs/src/metadata/mod.rs#L685

Are you going to support this usage scenario?

nydus meta blob has already been extracted to a local directory

nydus data blobs haven't been downloaded to the local directory

Yes.
For example, by now, when we use the coverter.Unpack method in nydus-snapshotter to unpack a nydus image layer, we need to extract both the bootstrap part and the blob data part to the local disk first to allow the nydus-image to read to blob data (use the localfs backend). But with this patch, we can just need to extract the bootstrap part and set up a local http server to serve the blob data part. Then the backend-config can be used to pass the information about this local http server.

Can we let nydus-image to handle the bootstrap part too? That's a feature I expect, then nydus-image can process remote images.

@jiangliu Yes, I think we can.

We need to implement the logic of extracting the bootstrap and blob data from the whole nydus image layer (which is implemented in nydus-snapshotter by now) and let the caller tell nydus-image the loacation of the image layer by some argument (may be --image-path).

What's your option? @imeoer

We may discuss offline. I hope we can enhance nydus-image to directly handle remote image, the same to nydusd.

jiangliu · 2023-02-15T12:00:39Z

Does this work for you?

nydus-image unpack -C nydusd.conf --outputb image.tar.gz path/to/bootstrap.file

jiangliu · 2023-02-15T12:01:35Z

api/src/config.rs

@@ -250,6 +250,7 @@ pub struct BackendConfigV2 {
    /// Configuration for container registry backend.
    pub registry: Option<RegistryConfig>,
    /// Configuration for local http proxy.
+    #[serde(rename = "http-proxy")]
    pub http_proxy: Option<HttpProxyConfig>,


This deserves a dedicated PR:)

Here's the pr #1086 .

jiangliu · 2023-02-15T12:01:59Z

src/bin/nydus-image/main.rs

@@ -17,16 +17,19 @@ extern crate lazy_static;
 use std::fs::{self, metadata, DirEntry, File, OpenOptions};
 use std::os::unix::fs::FileTypeExt;
 use std::path::{Path, PathBuf};
+use std::result::Result::Ok;


This may be unnecessary.

Yes. Fixed :)

anolis-bot · 2023-02-16T02:52:30Z

@loheagn , 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/51269

loheagn · 2023-02-16T03:06:37Z

Does this work for you?

nydus-image unpack -C nydusd.conf --outputb image.tar.gz path/to/bootstrap.file

NO. The argument --bootstrap is required.

anolis-bot · 2023-02-16T03:10:00Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS

Congratulations, your test job passed!

anolis-bot · 2023-02-16T03:22:48Z

@loheagn , 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/51285

cr-gpt · 2023-02-16T03:22:55Z

src/bin/nydus-image/main.rs

+
+        Ok(backend)
+    }
+
    fn get_blob_id(matches: &ArgMatches) -> Result<String> {
        let mut blob_id = String::new();



our review

The code contains unused imports like nix and nydus_storage.

The code is missing type annotations, which makes it difficult to understand what is going on in the code.

The code does not have any comments, which can help explain the purpose of the code.

In the prepare_cmd_args function, the argument for the backend-config should be made required.

In the get_backend function, the backend-config argument should be checked for validity before being used.

In the get_blob_id function, the blob_id variable should be initialized with a meaningful value before being used.

You are so smart 💯

anolis-bot · 2023-02-16T03:29:33Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS

Congratulations, your test job passed!

The `nydus-image unpack` only support reading nydus blob from the blob files on local machine (by the argument `blob`) by now. This patch adds a new argument `backend-config` to this command to allow `nydus-image unpack` to read blob data from all kinds of backends. Signed-off-by: Nan Li <loheagn@icloud.com>

anolis-bot · 2023-02-16T03:41:28Z

@loheagn , 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/51290

cr-gpt · 2023-02-16T03:41:44Z

src/bin/nydus-image/main.rs

+
+        Ok(backend)
+    }
+
    fn get_blob_id(matches: &ArgMatches) -> Result<String> {
        let mut blob_id = String::new();



with a brief introduction about the code,

The code is a part of an application written in Rust programming language. It implements a Command struct and Command trait which provides functionality for configuring backend from config file, importing chunk dictionary and unpack metadata. The code is also responsible for validating user provided arguments such as --output and --blob.

Now, let's move on to the code review.

First, the code contains some missing error handling for the LocalFs::new() method call. This should be added to prevent any unexpected errors.

Second, it would be better to use pattern matching when checking the blob argument instead of using if-else statements. This will make the code more readable and maintainable.

Third, the get_backend() function can be simplified by using the shorthand notation for destructuring the Option type. This will make the code more concise.

Fourth, the get_blob_id() method can be improved by using the Result type instead of returning a String. This will help in better error handling.

Finally, it would be better to use the Result combinators such as map_err() and map() to improve the readability of the code and make it more concise.

anolis-bot · 2023-02-16T03:47:14Z

@loheagn , The CI test is completed, please check result:

Test Case		Test Result
build rust golang image		✅ SUCCESS
compile nydusd		✅ SUCCESS
compile ctr remote		✅ SUCCESS
compile nydus snapshotter		✅ SUCCESS
run container with rafs		✅ SUCCESS
run container with zran		✅ SUCCESS

Congratulations, your test job passed!

jiangliu · 2023-02-16T09:11:28Z

Does this work for you?
nydus-image unpack -C nydusd.conf --outputb image.tar.gz path/to/bootstrap.file
NO. The argument --bootstrap is required.

With nydus 2.2, the "--bootstrap" option for nydus-image unpack should be optional:)
I simplified it by making "--bootstrap" optional.

loheagn requested a review from a team as a code owner February 15, 2023 06:35

loheagn requested review from jiangliu, changweige and adamqqqplay and removed request for a team February 15, 2023 06:35

anolis-bot added the anolis_testing label Feb 15, 2023

loheagn changed the title ~~Add backend-config support to nydus-image unpack~~ nydus-image: add backend-config support to nydus-image unpack Feb 15, 2023

loheagn force-pushed the unpack-stream branch from 353033a to 0543499 Compare February 15, 2023 06:40

anolis-bot added anolis_test_fail and removed anolis_testing labels Feb 15, 2023

anolis-bot added anolis_testing and removed anolis_test_fail labels Feb 15, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Feb 15, 2023

loheagn mentioned this pull request Feb 15, 2023

converter: add stream mode to Unpack containerd/nydus-snapshotter#294

Merged

jiangliu reviewed Feb 15, 2023

View reviewed changes

loheagn force-pushed the unpack-stream branch from 0543499 to 10fedc2 Compare February 16, 2023 02:52

anolis-bot added anolis_testing and removed anolis_test_pass labels Feb 16, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Feb 16, 2023

loheagn force-pushed the unpack-stream branch from 10fedc2 to d6821a6 Compare February 16, 2023 03:22

anolis-bot added anolis_testing and removed anolis_test_pass labels Feb 16, 2023

cr-gpt bot reviewed Feb 16, 2023

View reviewed changes

anolis-bot added anolis_test_pass and removed anolis_testing labels Feb 16, 2023

loheagn force-pushed the unpack-stream branch from d6821a6 to 081c8f9 Compare February 16, 2023 03:41

anolis-bot added anolis_testing and removed anolis_test_pass labels Feb 16, 2023

cr-gpt bot reviewed Feb 16, 2023

View reviewed changes

anolis-bot added anolis_test_pass and removed anolis_testing labels Feb 16, 2023

imeoer approved these changes Feb 20, 2023

View reviewed changes

imeoer merged commit aea56eb into dragonflyoss:master Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nydus-image: add `backend-config` support to `nydus-image unpack` #1082

nydus-image: add `backend-config` support to `nydus-image unpack` #1082

loheagn commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

jiangliu commented Feb 15, 2023

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023 •

edited

Loading

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023

jiangliu commented Feb 15, 2023

jiangliu Feb 15, 2023

loheagn Feb 16, 2023

jiangliu Feb 15, 2023

loheagn Feb 15, 2023

anolis-bot commented Feb 16, 2023

loheagn commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

cr-gpt bot Feb 16, 2023

loheagn Feb 16, 2023

anolis-bot commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

cr-gpt bot Feb 16, 2023

anolis-bot commented Feb 16, 2023

jiangliu commented Feb 16, 2023

nydus-image: add backend-config support to nydus-image unpack #1082

nydus-image: add backend-config support to nydus-image unpack #1082

Conversation

loheagn commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

anolis-bot commented Feb 15, 2023

jiangliu commented Feb 15, 2023

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023 • edited Loading

loheagn commented Feb 15, 2023

jiangliu commented Feb 15, 2023

jiangliu commented Feb 15, 2023

jiangliu Feb 15, 2023

Choose a reason for hiding this comment

loheagn Feb 16, 2023

Choose a reason for hiding this comment

jiangliu Feb 15, 2023

Choose a reason for hiding this comment

loheagn Feb 15, 2023

Choose a reason for hiding this comment

anolis-bot commented Feb 16, 2023

loheagn commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

cr-gpt bot Feb 16, 2023

Choose a reason for hiding this comment

loheagn Feb 16, 2023

Choose a reason for hiding this comment

anolis-bot commented Feb 16, 2023

anolis-bot commented Feb 16, 2023

cr-gpt bot Feb 16, 2023

Choose a reason for hiding this comment

anolis-bot commented Feb 16, 2023

jiangliu commented Feb 16, 2023

nydus-image: add `backend-config` support to `nydus-image unpack` #1082

nydus-image: add `backend-config` support to `nydus-image unpack` #1082

jiangliu commented Feb 15, 2023 •

edited

Loading