Introduce crt imds client. #184

eslrahc-swa · 2023-03-28T22:23:01Z

The main goal for using crt imds client is to query ec2 instance type mountpoint-s3 running on.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

monthonk

Great work! left a few comments that I think we should address.

.gitignore

mountpoint-s3/src/main.rs

monthonk · 2023-03-30T12:54:49Z

mountpoint-s3-client/src/imds_crt_client.rs

+    }
+
+    /// Make an HTTP request using this IMDS client that invokes the given callbacks.
+    pub fn make_instance_type_query(&self) -> oneshot::Receiver<Result<OsString, error::Error>> {


We probably want to map CRT error into something else likeImdsCrtClientError before returning, so that whoever call this function don't have to know CRT types when they do error handling. Also, the result should be String instead of OsString.

Also, the result should be String instead of OsString.
* will fix that.

However, I do not see the benefit of mapping mountpoint_s3_crt::common::error::Error to other type of error given it's either success or failure with no other reason.

The difference is that if we return mountpoint_s3_crt::common::error::Error and we want to use it at higher level (mountpoint-s3) like using match on the result, then we also have to add mountpoint_s3_crt as a dependency instead of just mountpoint_s3_client.

So, I would avoid having CRT types in the interface if possible.

monthonk · 2023-03-30T13:06:33Z

mountpoint-s3-client/src/imds_crt_client.rs

+    }
+
+    /// Make an HTTP request using this IMDS client that invokes the given callbacks.
+    pub fn make_instance_type_query(&self) -> oneshot::Receiver<Result<OsString, error::Error>> {


Could we add some tests around this function?

dannycjones

Nice! I have a few feedback.

.gitignore

mountpoint-s3/src/main.rs

mountpoint-s3-crt/src/auth/imds_client.rs

dannycjones · 2023-03-30T14:13:50Z

mountpoint-s3-client/src/imds_crt_client.rs

+    }
+
+    /// Make an HTTP request using this IMDS client that invokes the given callbacks.
+    pub fn make_instance_type_query(&self) -> oneshot::Receiver<Result<OsString, error::Error>> {


Does this function signature make sense?

I would expect something that looks more like pub async fn get_instance_type(&self) -> Result<String, CrtError>. No clue how to get there. :)

This function should not be async, just like s3_crt_client.rs -> fn make_meta_request<T: Send + 'static, E: Send + 'static>

Hmmmm, I see. And we do use .await later with this function. I will read into this a bit more again.

mountpoint-s3-crt/src/auth/imds_client.rs

dannycjones · 2023-03-31T09:19:56Z

Maybe we considered this already: should we be using CRT for this or should we use a Rust-based HTTP client?

CRT of course will make things easier in terms of how to use IMDS, but it does mean we have to put thought into the safety interacting with its C code.

mountpoint-s3-client/src/imds_crt_client.rs

mountpoint-s3-crt/src/auth/imds_client.rs

mountpoint-s3-client/src/imds_crt_client.rs

mountpoint-s3-crt/src/auth/imds_client.rs

mountpoint-s3/src/main.rs

mountpoint-s3-client/src/imds_crt_client.rs

mountpoint-s3-crt/src/auth/imds_client.rs

mountpoint-s3-client/src/imds_crt_client.rs

mountpoint-s3-crt/src/auth/imds_client.rs

mountpoint-s3/src/main.rs

mountpoint-s3-client/src/imds_crt_client.rs

The main goal of using crt imds client is to query ec2 instance type mountpoint-s3 running on. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

done

dannycjones · 2023-04-04T08:39:09Z

Nice work!

eslrahc-swa temporarily deployed to PR integration tests March 28, 2023 22:23 — with GitHub Actions Inactive

eslrahc-swa had a problem deploying to PR integration tests March 28, 2023 22:23 — with GitHub Actions Error

eslrahc-swa had a problem deploying to PR integration tests March 28, 2023 22:23 — with GitHub Actions Failure

monthonk reviewed Mar 30, 2023

View reviewed changes

dannycjones requested changes Mar 30, 2023

View reviewed changes

eslrahc-swa had a problem deploying to PR integration tests March 31, 2023 08:38 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests March 31, 2023 08:39 — with GitHub Actions Failure

dannycjones requested changes Mar 31, 2023

View reviewed changes

jamesbornholt reviewed Mar 31, 2023

View reviewed changes

eslrahc-swa had a problem deploying to PR integration tests March 31, 2023 19:06 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 2, 2023 03:15 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 2, 2023 03:16 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 2, 2023 20:30 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 2, 2023 20:32 — with GitHub Actions Failure

eslrahc-swa temporarily deployed to PR integration tests April 2, 2023 20:36 — with GitHub Actions Inactive

eslrahc-swa temporarily deployed to PR integration tests April 3, 2023 20:14 — with GitHub Actions Inactive

eslrahc-swa had a problem deploying to PR integration tests April 3, 2023 22:46 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 3, 2023 23:09 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 3, 2023 23:09 — with GitHub Actions Error

eslrahc-swa had a problem deploying to PR integration tests April 3, 2023 23:21 — with GitHub Actions Failure

awslabs deleted a comment from monthonk Apr 3, 2023

eslrahc-swa had a problem deploying to PR integration tests April 3, 2023 23:53 — with GitHub Actions Failure

eslrahc-swa temporarily deployed to PR integration tests April 4, 2023 00:04 — with GitHub Actions Inactive

jamesbornholt reviewed Apr 4, 2023

View reviewed changes

Introduce Instance Metadata Serivce(IMDS) client.

cc04cc5

The main goal of using crt imds client is to query ec2 instance type mountpoint-s3 running on. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

eslrahc-swa temporarily deployed to PR integration tests April 4, 2023 01:24 — with GitHub Actions Inactive

jamesbornholt approved these changes Apr 4, 2023

View reviewed changes

jamesbornholt enabled auto-merge (squash) April 4, 2023 01:29

jamesbornholt merged commit 2fade8b into awslabs:main Apr 4, 2023

eslrahc-swa linked an issue Apr 14, 2023 that may be closed by this pull request

Auto-configure Network Interface Card (NIC) throughput #3

Closed

jamesbornholt mentioned this pull request Apr 27, 2023

Auto-configure Network Interface Card (NIC) throughput #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce crt imds client. #184

Introduce crt imds client. #184

eslrahc-swa commented Mar 28, 2023

monthonk left a comment

monthonk Mar 30, 2023

eslrahc-swa Mar 31, 2023 •

edited

Loading

monthonk Mar 31, 2023

monthonk Mar 30, 2023

dannycjones left a comment

dannycjones Mar 30, 2023

eslrahc-swa Mar 31, 2023

dannycjones Mar 31, 2023 •

edited

Loading

dannycjones commented Mar 31, 2023

dannycjones commented Apr 4, 2023

Introduce crt imds client. #184

Introduce crt imds client. #184

Conversation

eslrahc-swa commented Mar 28, 2023

monthonk left a comment

Choose a reason for hiding this comment

monthonk Mar 30, 2023

Choose a reason for hiding this comment

eslrahc-swa Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

monthonk Mar 31, 2023

Choose a reason for hiding this comment

monthonk Mar 30, 2023

Choose a reason for hiding this comment

dannycjones left a comment

Choose a reason for hiding this comment

dannycjones Mar 30, 2023

Choose a reason for hiding this comment

eslrahc-swa Mar 31, 2023

Choose a reason for hiding this comment

dannycjones Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

dannycjones commented Mar 31, 2023

dannycjones commented Apr 4, 2023

eslrahc-swa Mar 31, 2023 •

edited

Loading

dannycjones Mar 31, 2023 •

edited

Loading