[DRAFT] add extension traits to run container without explicitly creating a c… #503

endertunc · 2023-06-24T11:06:20Z

This is an in progress work regarding to #386 . I just wanted to share the current prototype I have to validate my understanding.

Agree on the design and identify the breaking changes and document
Go thorough what we expose as public API and make sure that we don't expose something we don't need to.
Update tests

endertunc

Hi @thomaseizinger I created this in progress pull request to validated my understanding. Could you please take a look and let me know if what I am doing makes sense here?

testcontainers/Cargo.toml

endertunc · 2023-06-24T11:15:32Z

testcontainers/src/clients/cli.rs

-    inner: Arc<Client>,
+static CLI_DOCKER: OnceLock<Client> = OnceLock::new();
+fn docker_client() -> &'static Client {
+    CLI_DOCKER.get_or_init(|| Client::new::<env::Os>())
 }


Here we have global client which is used implicitly by Docker implementation.

endertunc · 2023-06-24T11:17:03Z

testcontainers/src/clients/cli.rs

@@ -464,6 +392,59 @@ impl Drop for Client {
    }
 }

+pub trait RunViaCli<I: Image> {
+    fn run(self) -> Container<I>;
+}


^ extension trait to use CLI as docker backend.

endertunc · 2023-06-24T11:18:27Z

testcontainers/src/clients/cli.rs

+}
+
+impl<I: Image> RunViaCli<I> for RunnableImage<I> {
+    fn run(self) -> Container<I> {


run function moved from Http to RunnableImage it can be directly called on RunnableImage

Can we move this function up in the file such that the diff is minimal? That would help reviewing a lot :)

It should be better now. 👍

endertunc · 2023-06-24T11:27:04Z

testcontainers/src/clients/http.rs

+                }
+            }
+            env::Command::Keep => {}
+        }


This is something we need to talk I believe. I recently learned that static variables don't actually call Drop so we might have to move this logic to WatchDog or find another way to create global static variable.

Arc could be useful but I am not sure how to create and Arc that behaves like static variable.

Yeah a static variable will only be dropped at the end of a program.

Rust executes each test in its own thread so we could look into using a thread-local variable for this. That would clean up a docker client at the end of each thread, i.e. at the end of each test and each test would gets its own docker client.

Now the tests are updated there are 3 failing tests and all of them about related to "networks" which were suppose to be cleaned up but it's not working due to the issue I mentioned.

failures: clients::cli::tests::should_create_network_if_image_needs_it_and_drop_it_in_the_end clients::cli::tests::should_not_delete_network_if_command_is_keep clients::http::tests::http_should_create_network_if_image_needs_it_and_drop_it_in_the_end

I see 3 options here;

The method you mentioned - (I am not very familiar with rust in that regard so if you let me know how this could be achieved I can take a look)

We can integrated with ryuk which would handle all kind of clean-up scenarios also align with other testcontainers projects. (I am not %100 sure but this would mean that some test cases can't be run in parallel) This approach is similar to watchdog feature we have in this repo but ryuk is client agnostic.

For this specific use case, we can keep track of number of containers which uses the given network and remove at when the last container is dropped.

// assume this is global variable let networks = Mutex<Map<String,i32>> // network_name -> number_of_containers_using_the_network // inside start function insert or increase the number of reference to given network key in global map // inside impl drop for container decrease the number of reference to given network. If number of reference is 0 after this operation remove the network as well.

I personally prefer option two so we can align with other implementations and also I like the ryuk approach in terms of design but I would like to hear what you think.

Essentially, what we need to achieve is that a client is only alive for as long as any of its containers are alive. Thus I'd suggest the following:

Store a Mutex<Option<Weak<Client>>> in the static variable

When you try to get a reference to the client:

Grab a lock to the mutex

If the value is None:

Make a new client

Wrap it in an Arc

Make a copy

Downgrade the copy to a Weak

Store the Weak inside the Option

Release the lock

Return the Arc you still have around

If the value is Some:

Attempt to upgrade to an Arc

If successful, return

If failed, do the same thing as for the above None case

I believe this should achieve the following:

As long is there is at least 1 Container around, we can grab a reference to it and make more containers.

If all containers are dropped, the last strong reference to the Client is also dropped which means Drop is being called on Client

I see integration with ryuk as a separate feature.

thomaseizinger · 2023-06-24T14:11:00Z

Hi @thomaseizinger I created this in progress pull request to validated my understanding. Could you please take a look and let me know if what I am doing makes sense here?

Will review next week!

thomaseizinger

Great start overall! I'd like to see some tests updated so we get an idea what the API feels like.

testcontainers/Cargo.toml

thomaseizinger · 2023-06-26T19:18:25Z

testcontainers/src/clients/cli.rs

+}
+
+impl<I: Image> RunViaCli<I> for RunnableImage<I> {
+    fn run(self) -> Container<I> {


Can we move this function up in the file such that the diff is minimal? That would help reviewing a lot :)

thomaseizinger · 2023-06-26T19:27:51Z

testcontainers/src/clients/http.rs

+                }
+            }
+            env::Command::Keep => {}
+        }


Yeah a static variable will only be dropped at the end of a program.

Rust executes each test in its own thread so we could look into using a thread-local variable for this. That would clean up a docker client at the end of each thread, i.e. at the end of each test and each test would gets its own docker client.

testcontainers/src/clients/http.rs

…lient

…ap with RunnableImage::from

testcontainers/src/clients/cli.rs

testcontainers/src/clients/http.rs

thomaseizinger

Sorry for the late review, I don't get to spend much time with this repository.

Great progress though, I left some comments :)

thomaseizinger · 2023-09-04T04:12:00Z

testcontainers/src/clients/cli.rs

        }
    }
 }

-impl Docker for Cli {
+impl Docker for &Client {


Do we still need the Docker trait now? By virtue of the RunViaCli trait, the implementation is no longer pluggable so I believe we could just remove it now.

thomaseizinger · 2023-09-04T04:13:26Z

testcontainers/src/clients/http.rs

@@ -276,7 +269,7 @@ impl Drop for Client {
 }

 #[async_trait]
-impl DockerAsync for Http {
+impl DockerAsync for &Client {


Same here, I think we can remove this trait and just say impl Client here.

thomaseizinger · 2023-09-04T04:14:49Z

testcontainers/tests/cli_client.rs

    let generic = GenericImage::new("simple_web_server", "latest").with_wait_for(msg.clone());

-    let node = docker.run(generic);
+    let node = generic.start();


What I like about this new API is that we can chain it, can we rewrite the tests to do that?

testcontainers/src/clients/http.rs

thomaseizinger · 2023-09-04T04:16:53Z

testcontainers/src/clients/http.rs

        let image = GenericImage::new("hello-world", "latest");
-        let image = RunnableImage::from(image).with_container_name("hello_container");
-        let container = docker.run(image).await;
+        let container = RunnableImage::from(image)
+            .with_container_name("hello_container")


Can we figure out a way to make this more ergonomic? Ideally, I'd like users to never need to construct RunnableImage but only Images. with_container_name is defined in a trait, right? Does it already work to just write:

GenericImage::new().with_container_name().start().await;

endertunc changed the title ~~add extension traits to run container without explicitly creating a c…~~ [DRAFT] add extension traits to run container without explicitly creating a c… Jun 24, 2023

endertunc commented Jun 24, 2023

View reviewed changes

thomaseizinger reviewed Jun 26, 2023

View reviewed changes

add extension traits to run container without explicitly creating a c…

f6d456f

…lient

endertunc force-pushed the add-extension-traits-for-clients branch from b534af7 to f6d456f Compare July 1, 2023 18:39

Ender Tunc added 2 commits July 2, 2023 14:16

update tests to use singleton docker client

cd95e61

add ability start container directly from Image without needing to wr…

ccc896b

…ap with RunnableImage::from

endertunc commented Jul 2, 2023

View reviewed changes

testcontainers/src/clients/cli.rs Show resolved Hide resolved

endertunc commented Jul 2, 2023

View reviewed changes

testcontainers/src/clients/http.rs Show resolved Hide resolved

endertunc mentioned this pull request Jul 8, 2023

Support accessing container logs #502

Closed

thomaseizinger reviewed Sep 4, 2023

View reviewed changes

blandger mentioned this pull request Nov 2, 2023

Re-work public API to be Container centric #386

Closed

DDtKey closed this Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] add extension traits to run container without explicitly creating a c… #503

[DRAFT] add extension traits to run container without explicitly creating a c… #503

endertunc commented Jun 24, 2023 •

edited

Loading

endertunc left a comment

endertunc Jun 24, 2023

endertunc Jun 24, 2023

endertunc Jun 24, 2023

thomaseizinger Jun 26, 2023

endertunc Jul 2, 2023

endertunc Jun 24, 2023

thomaseizinger Jun 26, 2023

endertunc Jul 2, 2023 •

edited

Loading

thomaseizinger Sep 4, 2023

thomaseizinger commented Jun 24, 2023

thomaseizinger left a comment

thomaseizinger Jun 26, 2023

thomaseizinger Jun 26, 2023

thomaseizinger left a comment

thomaseizinger Sep 4, 2023

thomaseizinger Sep 4, 2023

thomaseizinger Sep 4, 2023

thomaseizinger Sep 4, 2023

[DRAFT] add extension traits to run container without explicitly creating a c… #503

[DRAFT] add extension traits to run container without explicitly creating a c… #503

Conversation

endertunc commented Jun 24, 2023 • edited Loading

endertunc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endertunc Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Jun 24, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a 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

endertunc commented Jun 24, 2023 •

edited

Loading

endertunc Jul 2, 2023 •

edited

Loading