feat(jstzd): implement image #553

ryutamago · 2024-09-06T13:54:49Z

Context

Implement traits / struct to run docker container that can later be used for jstzd

The related tasks are the following but not all of them are fully covered/tested yet (e.g. mounts/hosts etc)
image
runnable image

Description

Implement the docker images traits and structs

Manually testing the PR

cargo test --package jstzd --lib

codecov · 2024-09-06T14:08:31Z

Codecov Report

Attention: Patch coverage is 84.17722% with 25 lines in your changes missing coverage. Please review.

Project coverage is 34.82%. Comparing base (027eeb5) to head (8a664b6).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/jstzd/src/docker/image.rs	72.88%	16 Missing ⚠️
crates/jstzd/src/docker/runnable_image.rs	90.90%	5 Missing and 4 partials ⚠️

Files with missing lines	Coverage Δ
crates/jstzd/src/docker/runnable_image.rs	`90.90% <90.90%> (ø)`
crates/jstzd/src/docker/image.rs	`72.88% <72.88%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 027eeb5...8a664b6. Read the comment docs.

crates/jstzd/src/container.rs

crates/jstzd/src/image.rs

zcabter · 2024-09-10T10:49:33Z

The manual test fails with Error: Docker responded with status code 404: No such image: busybox:latest. Seems like the pull_image step is missing.

zcabter · 2024-09-10T10:57:28Z

Commit messages are missing scope. Prefer to include it if possible. feat(jstzd) is a good one for this PR.

crates/jstzd/src/runnable_image/mod.rs

Cargo.toml

crates/jstzd/Cargo.toml

crates/jstzd/src/container.rs

crates/jstzd/src/image.rs

crates/jstzd/src/network.rs

crates/jstzd/src/runnable_image/mod.rs

johnyob · 2024-09-10T11:23:18Z

Additionally in future, I suggest we stick to 1 linear task per a PR. This single PR partially closes 4 (some support is missing here and there). This will not only make task tracking easier (and fits more nicely into linear's workflow), but will make reviews easier

ryutamago · 2024-09-10T15:47:04Z

The manual test fails with Error: Docker responded with status code 404: No such image: busybox:latest. Seems like the pull_image step is missing.

I accidentally removed the pull image code in the example since it was pulled in my local machin, i've added it back so should work now!

crates/jstzd/src/docker/runnable_image.rs

jasbir-dhillon · 2024-09-12T09:52:28Z

What was the reason/advantage for using rust to generate docker image rather thank just using a Dockerfile and docker-compose?

zcabter · 2024-09-12T10:00:44Z

What was the reason/advantage for using rust to generate docker image rather thank just using a Dockerfile and docker-compose?

This work doesn't generate docker images but automates pulling and running them in containers @jasbir-dhillon. These APIs will ultimately be used to orchestrate the jstzd sandbox environment.

jasbir-dhillon · 2024-09-12T10:15:43Z

What was the reason/advantage for using rust to generate docker image rather thank just using a Dockerfile and docker-compose?

This work doesn't generate docker images but automates pulling and running them in containers @jasbir-dhillon. These APIs will ultimately be used to orchestrate the jstzd sandbox environment.

@zcabter Could that have been done by just using a docker-compose file. You would of needed less lines of code and it would have been more readable?

huancheng-trili · 2024-09-12T10:18:42Z

crates/jstzd/src/docker/container.rs

+            let mut this = std::mem::take(self);
+            // Prevent the original `self` to drop again
+            self.dropped = true;
+            TokioScope::scope_and_block(|s| {


Just a note about scope_and_block: this only works with a multi-threaded runtime. Otherwise, the code will panic here. Sample error message:

thread '...' panicked at ...: can call blocking only when running on the multi-threaded runtime

oh didn't notice that, the number of threads in tokio runtime defaults to the number of cpus on the system so i guess we can assume it is multithreaded ?

I realised that this is not just for scope_and_block but for blocking async in general, e.g. tokio::task::block_in_place.

I guess we can keep that assumption for now. For unit tests this might fail since single-threaded runtime is used by default. This can be resolved with #[tokio::test(flavor = "multi_thread")].

zcabter · 2024-09-12T11:02:08Z

What was the reason/advantage for using rust to generate docker image rather thank just using a Dockerfile and docker-compose?

This work doesn't generate docker images but automates pulling and running them in containers @jasbir-dhillon. These APIs will ultimately be used to orchestrate the jstzd sandbox environment.

@zcabter Could that have been done by just using a docker-compose file. You would of needed less lines of code and it would have been more readable?

It could but we'd still need to write an api over it as Docker is just one of Tasks target backends. The jstzd orchestrator would still need to work with tasks in a backend agnostic way.

zcabter · 2024-09-12T11:56:35Z

The testing coverage is a bit low partly because integration tests are missing. I know you put a lot of effort into this but I think this PR needs to be broken down into 4 PRs, 1 per issue. Where we are running docker commands, it needs to be
integration tested. This will make it easier to merge the parts of your code that is ready.

Additionally, I suggest removing the async dropper and custom drop logic for now since it will be re-thought (per #555 (comment)) but keeping the clean up logic.

ryutamago · 2024-09-12T13:07:51Z

The testing coverage is a bit low partly because integration tests are missing. I know you put a lot of effort into this but I think this PR needs to be broken down into 4 PRs, 1 per issue. Where we are running docker commands, it needs to be integration tested. This will make it easier to merge the parts of your code that is ready.

Additionally, I suggest removing the async dropper and custom drop logic for now since it will be re-thought (per #555 (comment)) but keeping the clean up logic.

I've split into three PRs, the first one doesn't require integration test so it could be merged. The 2 and 3 pr is tested manually and it only does the basic stuff so I personally don't think there's much problem merging it so we can see something is working. I've also removed the drop for now in the 3rd pr.

image / runnable image
pull-image
container

@johnyob @zcabter

johnyob · 2024-09-17T10:42:10Z

crates/jstzd/src/docker/runnable_image.rs

+        self
+    }
+
+    pub async fn create_container(&self, client: Arc<Docker>) -> Result<String> {


I would move all asssociated logic for create_container (aka start) under a different PR (will help with your testing).
The task is: https://linear.app/tezos/issue/JSTZ-62/implement-start-for-runnableimage

done, i will add this back after the pull image PR

No, this should be a separate PR -- it is a different task to pull_image

Cargo.toml

codecov.yml

johnyob

lgtm 🚀

ryutamago force-pushed the leo/docker-image branch 2 times, most recently from e3ef180 to 254bca3 Compare September 6, 2024 13:58

ryutamago changed the title ~~[WIP] Docker network/image~~ Jstzd: Add basic docker image / container Sep 6, 2024

ryutamago force-pushed the leo/docker-image branch 2 times, most recently from 08c563d to 7eda38d Compare September 9, 2024 12:21

ryutamago requested review from johnyob and zcabter September 9, 2024 13:26

johnyob changed the title ~~Jstzd: Add basic docker image / container~~ feat(jstzd): add basic Docker image / container Sep 10, 2024

johnyob self-assigned this Sep 10, 2024

zcabter reviewed Sep 10, 2024

View reviewed changes

crates/jstzd/src/runnable_image/mod.rs Outdated Show resolved Hide resolved

crates/jstzd/src/runnable_image/mod.rs Outdated Show resolved Hide resolved

crates/jstzd/src/runnable_image/mod.rs Outdated Show resolved Hide resolved

johnyob reviewed Sep 10, 2024

View reviewed changes

zcabter reviewed Sep 10, 2024

View reviewed changes

crates/jstzd/src/runnable_image/mod.rs Outdated Show resolved Hide resolved

johnyob assigned ryutamago and unassigned johnyob Sep 10, 2024

ryutamago force-pushed the leo/docker-image branch from 7eda38d to c63d91c Compare September 10, 2024 15:41

ryutamago requested review from johnyob and zcabter September 10, 2024 15:48

ryutamago force-pushed the leo/docker-image branch from c63d91c to c6f4151 Compare September 10, 2024 15:53

huancheng-trili mentioned this pull request Sep 10, 2024

feat(jstzd): define Task trait and type JoinHandle #555

Closed

zcabter reviewed Sep 10, 2024

View reviewed changes

crates/jstzd/src/docker/runnable_image.rs Outdated Show resolved Hide resolved

ryutamago force-pushed the leo/docker-image branch 2 times, most recently from feae871 to 1d068e4 Compare September 12, 2024 09:23

ryutamago requested a review from zcabter September 12, 2024 09:24

ryutamago force-pushed the leo/docker-image branch from 1d068e4 to 9e13d12 Compare September 12, 2024 09:32

ryutamago force-pushed the leo/docker-image branch from 9e13d12 to b330d0f Compare September 12, 2024 09:37

huancheng-trili reviewed Sep 12, 2024

View reviewed changes

ryutamago force-pushed the leo/docker-image branch from b330d0f to 605ae93 Compare September 12, 2024 12:39

ryutamago changed the title ~~feat(jstzd): add basic Docker image / container~~ JSTZD: implement image Sep 12, 2024

johnyob changed the title ~~JSTZD: implement image~~ feat(jstzd): implement image Sep 13, 2024

johnyob reviewed Sep 17, 2024

View reviewed changes

feat(jstzd): add docker image

fc310d6

ryutamago force-pushed the leo/docker-image branch from 605ae93 to fc310d6 Compare September 17, 2024 10:50

refactor(jstzd): remove create_container

8a664b6

ryutamago force-pushed the leo/docker-image branch from 0bc953f to 8a664b6 Compare September 17, 2024 11:07

johnyob assigned johnyob and unassigned ryutamago Sep 17, 2024

johnyob approved these changes Sep 17, 2024

View reviewed changes

johnyob merged commit a6aaa2c into main Sep 17, 2024
5 checks passed

johnyob deleted the leo/docker-image branch September 17, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jstzd): implement image #553

feat(jstzd): implement image #553

ryutamago commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

zcabter commented Sep 10, 2024

zcabter commented Sep 10, 2024

johnyob commented Sep 10, 2024

ryutamago commented Sep 10, 2024

jasbir-dhillon commented Sep 12, 2024

zcabter commented Sep 12, 2024 •

edited

Loading

jasbir-dhillon commented Sep 12, 2024

huancheng-trili Sep 12, 2024

fizikarubi Sep 12, 2024 •

edited

Loading

huancheng-trili Sep 12, 2024

zcabter commented Sep 12, 2024

zcabter commented Sep 12, 2024

ryutamago commented Sep 12, 2024

johnyob Sep 17, 2024 •

edited

Loading

ryutamago Sep 17, 2024 •

edited

Loading

johnyob Sep 17, 2024

johnyob left a comment

feat(jstzd): implement image #553

feat(jstzd): implement image #553

Conversation

ryutamago commented Sep 6, 2024 • edited Loading

Context

Description

Manually testing the PR

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

zcabter commented Sep 10, 2024

zcabter commented Sep 10, 2024

johnyob commented Sep 10, 2024

ryutamago commented Sep 10, 2024

jasbir-dhillon commented Sep 12, 2024

zcabter commented Sep 12, 2024 • edited Loading

jasbir-dhillon commented Sep 12, 2024

huancheng-trili Sep 12, 2024

Choose a reason for hiding this comment

fizikarubi Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

huancheng-trili Sep 12, 2024

Choose a reason for hiding this comment

zcabter commented Sep 12, 2024

zcabter commented Sep 12, 2024

ryutamago commented Sep 12, 2024

johnyob Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

ryutamago Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

johnyob Sep 17, 2024

Choose a reason for hiding this comment

johnyob left a comment

Choose a reason for hiding this comment

ryutamago commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

zcabter commented Sep 12, 2024 •

edited

Loading

fizikarubi Sep 12, 2024 •

edited

Loading

johnyob Sep 17, 2024 •

edited

Loading

ryutamago Sep 17, 2024 •

edited

Loading