Implement nydusd and nydus-snapshotter to deliver config through the socket API #1379

DarkMountain-wyz · 2023-07-20T02:50:19Z

Relevant Issue (if applicable)

containerd/nydus-snapshotter#388

Details

The previous PR has been closed, so a new PR has been submitted
Command line parameter --config now represents the URL to request daemon config from nydus-snapshotter.
New command line parameter --config-source, representing the unix socket path for communication between nydus snapshot and nydusd.
Nydusd is now requesting config information from nydus-snapshotter instead of obtaining config by reading the config dump file.

Types of changes

What types of changes does your PullRequest introduce? Put an x in all the boxes that apply:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation Update (if none of the other choices apply)

Checklist

Go over all the following points, and put an x in all the boxes that apply.

I have updated the documentation accordingly.
I have added tests to cover my changes.

anolis-bot · 2023-07-20T02:50:27Z

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

codecov · 2023-07-20T02:55:16Z

Codecov Report

Merging #1379 (ba3357d) into master (9ab1ec1) will decrease coverage by 0.12%.
Report is 1 commits behind head on master.
The diff coverage is 1.98%.

@@            Coverage Diff             @@
##           master    #1379      +/-   ##
==========================================
- Coverage   46.29%   46.18%   -0.12%     
==========================================
  Files         123      123              
  Lines       38872    38960      +88     
  Branches    38872    38960      +88     
==========================================
- Hits        17997    17992       -5     
- Misses      19906    19994      +88     
- Partials      969      974       +5

Files	Coverage Δ
src/bin/nydusd/main.rs	`0.18% <0.00%> (-0.01%)`	⬇️
api/src/config.rs	`69.37% <0.00%> (-2.07%)`	⬇️
storage/src/backend/registry.rs	`31.38% <4.00%> (-1.56%)`	⬇️

... and 2 files with indirect coverage changes

anolis-bot · 2023-07-20T03:02:43Z

@DarkMountain-wyz , 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
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

anolis-bot · 2023-07-26T07:00:32Z

@DarkMountain-wyz , 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/87020

anolis-bot · 2023-07-26T09:05:18Z

@DarkMountain-wyz , 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
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

imeoer · 2023-07-31T02:04:31Z

src/bin/nydusd/main.rs

@@ -418,7 +505,28 @@ fn process_fs_service(
                )
            }
            None => match args.value_of("config") {
-                Some(v) => std::fs::read_to_string(v)?,
+                Some(v) => {
+                    let socket_path = match args.value_of("config-source") {


Does the option --config-source should be conflict with --config, right?

imeoer · 2023-07-31T02:09:43Z

src/bin/nydusd/main.rs

@@ -363,6 +373,83 @@ fn handle_rlimit_nofile_option(args: &ArgMatches, option_name: &str) -> Result<(
    Ok(())
 }

+fn send_unix_socket_request(socket_path: &str, method: &str, url: &str) -> Option<String> {


How about putting this into api/src/config.rs, and naming it fn from_source(...).

imeoer · 2023-07-31T02:10:58Z

src/bin/nydusd/main.rs

+fn send_unix_socket_request(socket_path: &str, method: &str, url: &str) -> Option<String> {
+    let mut stream = match UnixStream::connect(socket_path).ok() {
+        Some(stream) => stream,
+        None => return None,


We'd better return a Result instead of an Option, and handle the Result outside.

imeoer · 2023-07-31T02:14:42Z

src/bin/nydusd/main.rs

+        None => return None,
+    };
+
+    let request = format!("{} {} HTTP/1.1\r\nHost: localhost\r\n\r\n", method, url);


How about using the hyper::Client crate directly, instead of implementing the HTTP over the unix domain socket client by self?

sctb512 · 2023-07-31T13:46:36Z

src/bin/nydusd/main.rs

+    let mut total_read = 0;
+
+    loop {
+        match stream.read(&mut buffer) {


I find it strange to actively read configuration from nydus-snapshotter. This means nydus daemon depends on nydus-snapshoter. But nydus-snapshotter is the manager of nydus daemon. So, I think nydus-snapshotter should update the configuration as needed instead of providing an API for the daemon.

anolis-bot · 2023-08-05T06:05:36Z

@DarkMountain-wyz , 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/89250

anolis-bot · 2023-08-05T06:18:37Z

@DarkMountain-wyz , 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
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

imeoer · 2023-08-07T02:49:11Z

api/src/config.rs

@@ -218,6 +220,32 @@ impl ConfigV2 {
            }
        }
    }
+
+    /// Fill backend config from nydus-snapshotter


Not only for nydus snapshotter in the future.

imeoer · 2023-08-07T02:50:03Z

api/src/config.rs

+        let uri = Uri::new(sock, endpoint).into();
+        let response = client.get(uri).await?;
+        let status_code = response.status().as_u16();
+        let buf = hyper::body::to_bytes(response).await?;


Should read the response body after the status code check.

imeoer · 2023-08-07T02:51:29Z

api/src/config.rs

+        let buf = hyper::body::to_bytes(response).await?;
+        if status_code != 200 {
+            return Err(anyhow::anyhow!(
+                "Request failed with status code: {}",


failed to request backend config from source, status code: {}

imeoer · 2023-08-07T02:52:04Z

api/src/config.rs

@@ -245,6 +273,7 @@ impl FromStr for ConfigV2 {
                }
            }
        }
+        info!("你到这了吗");


Remove this.

imeoer · 2023-08-07T02:53:15Z

src/bin/nydusd/main.rs

+            .global(true),
+        )
+        .arg(
+        Arg::new("credential-source")


How about renaming it to backend-source, and combining the url with systemsock?

imeoer · 2023-08-08T02:00:47Z

@DarkMountain-wyz Please refine the commits and add some commit messages to explain why & how to do this.

DarkMountain-wyz · 2023-08-08T02:04:38Z

@DarkMountain-wyz Please refine the commits and add some commit messages to explain why & how to do this.
I will do it

anolis-bot · 2023-08-08T02:31:39Z

@DarkMountain-wyz , 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/89600

anolis-bot · 2023-08-08T02:43:39Z

@DarkMountain-wyz , 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
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

anolis-bot · 2023-08-08T04:17:41Z

@DarkMountain-wyz , 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/89652

anolis-bot · 2023-08-08T04:29:56Z

@DarkMountain-wyz , 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
run container with rafs and compile linux		✅ SUCCESS

Congratulations, your test job passed!

imeoer · 2023-09-25T03:37:37Z

@DarkMountain-wyz We'd better refresh the backend config when the registry auth expires.

imeoer · 2023-10-16T03:42:45Z

api/Cargo.toml

@@ -13,6 +13,9 @@ libc = "0.2"
 log = "0.4.8"
 serde_json = "1.0.53"
 toml = "0.5"
+hyper = "0.14.11"
+hyperlocal = "0.8.0"
+anyhow = "1"


Maybe use the exact version 1.0 here?

imeoer · 2023-10-16T03:44:23Z

api/Cargo.toml

@@ -21,9 +24,11 @@ lazy_static = { version = "1.4.0", optional = true }
 mio = { version = "0.8", features = ["os-poll", "os-ext"], optional = true }
 serde = { version = "1.0.110", features = ["rc", "serde_derive"] }
 url = { version = "2.1.1", optional = true }
+tokio = { version = "1.24", features = ["full"] }


It seems the using of full feature will import a large number of dependencies, can we only use the necessary feature?

imeoer · 2023-10-16T03:49:18Z

storage/src/backend/registry.rs

@@ -37,6 +39,7 @@ const REDIRECTED_STATUS_CODE: [StatusCode; 2] = [
 ];

 const REGISTRY_DEFAULT_TOKEN_EXPIRATION: u64 = 10 * 60; // in seconds
+const MAX_RETRIES: u32 = 1;


It is unnecessary to add the retry mechanism because the upper caller will retry if any error happens.

imeoer · 2023-10-16T04:04:56Z

api/src/config.rs


 use serde::Deserialize;
 use serde_json::Value;

+lazy_static! {
+    pub static ref BACKEND_SOURCE_PATH: Mutex<Option<String>> = Mutex::new(None);


How about putting this into BackendConfigV2 struct like this (as a private field, do not participate in Deserialize/Serialize):

pub struct BackendConfigV2 { /// Type of storage backend. #[serde(rename = "type")] pub backend_type: String, /// Source of storage backend configuration. #[serde(skip_serializing, skip_deserializing)] backend_source: Option<String>, }

imeoer · 2023-10-16T04:07:19Z

storage/src/backend/registry.rs

+
+            if token_resp.status() == StatusCode::OK {
+                return Ok(token_resp);
+            } else if token_resp.status() == StatusCode::FORBIDDEN && retries < MAX_RETRIES {


Should be vec![StatusCode::UNAUTHORIZED, StatusCode::FORBIDDEN].contains(&token_resp.status()).

imeoer · 2023-10-16T04:09:01Z

storage/src/backend/registry.rs

+                            let registry_auth = trim(registry_config.auth.clone());
+                            let (new_username, new_password) =
+                                Registry::get_authorization_info(&registry_auth)?;
+                            username = new_username;


self.username & self.password should be updated.

ccx1024cc · 2023-10-30T03:02:59Z

src/bin/nydusd/main.rs

@@ -276,6 +276,13 @@ fn prepare_commandline_options() -> Command {
                .required(false)
                .global(true),
        )
+        .arg(
+            Arg::new("backend-source")


Smoke test is required. Please also update doc.

bergwolf

The PR title mentions config delivery but it actually is only about auth right? We already support configurating nydusd backend (which including auth info) via API. It is just that we need to amend nydus-snapshotter to stopping passing config via local file but via API instead. @DarkMountain-wyz Are you planning to look at the nydus-snapshotter part as well?

bergwolf · 2023-11-01T03:57:48Z

src/bin/nydusd/main.rs

@@ -425,7 +432,11 @@ fn process_fs_service(
                        config.update_registry_auth_info(&auth);
                        serde_json::to_string(&config)?
                    } else {
-                        std::fs::read_to_string(v)?
+                        let mut config = ConfigV2::from_file(v)?;
+                        if let Some(source) = args.value_of("backend-source") {


Is it a backend-source or an auth-source? It looks like that we only use it to get auth info.

bergwolf · 2023-11-01T03:59:54Z

api/src/config.rs

+            })
+            .ok()?;
+        let backend_config_v2 = BackendConfigV2::try_from(&backend_config).ok()?;
+        let mut backend_source = BACKEND_SOURCE_PATH.lock().unwrap();


This would deadlock as we call update_auth_from_source after taking the BACKEND_SOURCE_PATH mutex when dealing with backend auth failures, which also indicates that the code path has not been tested.

BACKEND_SOURCE_PATH should actually be only set when starting nydusd. There is no need to update it from time to time.

bergwolf · 2023-11-01T04:11:34Z

And where is the corresponding nydus-snapshotter part of the change? I assume we need a new API from snapshotter as well, no?

DarkMountain-wyz · 2023-11-01T06:55:10Z

And where is the corresponding nydus-snapshotter part of the change? I assume we need a new API from snapshotter as well, no?

The corresponding modification for the nydus-snapshotter can be found in the pull request here. The approach taken involves providing a domain socket API for nydusd to request and retrieve backend information. The decision to pass all backend information rather than just authentication details is to ensure flexibility in case new types of backends are added in the future, without needing to modify this interface.

DarkMountain-wyz requested a review from a team as a code owner July 20, 2023 02:50

DarkMountain-wyz requested review from liubogithub, imeoer and hsiangkao and removed request for a team July 20, 2023 02:50

anolis-bot added the anolis_testing label Jul 20, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Jul 20, 2023

jiangliu mentioned this pull request Jul 25, 2023

parse image pull auth from env #1382

Merged

6 tasks

anolis-bot added anolis_testing and removed anolis_test_pass labels Jul 26, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Jul 26, 2023

imeoer reviewed Jul 31, 2023

View reviewed changes

sctb512 reviewed Jul 31, 2023

View reviewed changes

DarkMountain-wyz closed this Aug 5, 2023

anolis-bot added anolis_testing and removed anolis_test_pass labels Aug 5, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Aug 5, 2023

DarkMountain-wyz reopened this Aug 6, 2023

imeoer reviewed Aug 7, 2023

View reviewed changes

anolis-bot removed the anolis_test_pass label Aug 7, 2023

DarkMountain-wyz closed this Aug 8, 2023

DarkMountain-wyz reopened this Aug 8, 2023

DarkMountain-wyz closed this Aug 8, 2023

anolis-bot added anolis_testing and removed anolis_test_pass labels Aug 8, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Aug 8, 2023

DarkMountain-wyz reopened this Aug 8, 2023

anolis-bot added anolis_testing and removed anolis_test_pass labels Aug 8, 2023

anolis-bot added anolis_test_pass and removed anolis_testing labels Aug 8, 2023

imeoer reviewed Oct 16, 2023

View reviewed changes

ccx1024cc suggested changes Oct 30, 2023

View reviewed changes

bergwolf reviewed Nov 1, 2023

View reviewed changes

DarkMountain-wyz closed this Nov 9, 2023

Implement nydusd and nydus-snapshotter to deliver config through the socket API #1379

Implement nydusd and nydus-snapshotter to deliver config through the socket API #1379

Conversation

DarkMountain-wyz commented Jul 20, 2023

Relevant Issue (if applicable)

Details

Types of changes

Checklist

anolis-bot commented Jul 20, 2023

codecov bot commented Jul 20, 2023 • edited Loading

Codecov Report

anolis-bot commented Jul 20, 2023

anolis-bot commented Jul 26, 2023

anolis-bot commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anolis-bot commented Aug 5, 2023

anolis-bot commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imeoer commented Aug 8, 2023

DarkMountain-wyz commented Aug 8, 2023 • edited Loading

anolis-bot commented Aug 8, 2023

anolis-bot commented Aug 8, 2023

anolis-bot commented Aug 8, 2023

anolis-bot commented Aug 8, 2023

imeoer commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergwolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergwolf commented Nov 1, 2023

DarkMountain-wyz commented Nov 1, 2023

codecov bot commented Jul 20, 2023 •

edited

Loading

DarkMountain-wyz commented Aug 8, 2023 •

edited

Loading