clean up debugging and function response

lib/managed_docker/container.ex

@@ -28,11 +28,16 @@ defmodule ManagedDocker.Container do
 
   @impl true
   def handle_call({:run, args}, {from, _ref}, state) do
-    {id, _} = System.cmd("docker", ["run", "-dt", args], stderr_to_stdout: true)
-    container_id = String.trim(id)
-    monitor_ref = Process.monitor(from)
-    state = Map.put(state, from, {container_id, monitor_ref})
-    {:reply, {:ok, container_id}, state}
+    case System.cmd("docker", ["run", "-dt", args], stderr_to_stdout: true) do
+      {id, 0} ->
+        container_id = String.trim(id)
+        monitor_ref = Process.monitor(from)
+        state = Map.put(state, from, {container_id, monitor_ref})
+        {:reply, {:ok, container_id}, state}
+
+      {msg, _} ->
+        {:reply, {:error, msg}, state}
+    end
   end
 
   @impl true
@@ -44,54 +49,68 @@ defmodule ManagedDocker.Container do
         container_id == state_container_id
       end)
 
-    updated_state = cleanup(container_id, monitor_ref, pid, state)
-    {:reply, :ok, updated_state}
+    case remove_container(container_id, monitor_ref, pid) do
+      :ok -> {:reply, :ok, Map.delete(state, pid)}
+      error -> {:reply, error, state}
+    end
   end
 
   @impl true
-  def handle_call(:list, {from, _ref}, state) do
+  def handle_call(:list, {_from, _ref}, state) do
     {:reply, {:ok, state}, state}
   end
 
   @impl true
-  def handle_info({:DOWN, _ref, :process, pid, reason}, state) do
-    Logger.debug("Got dow")
+  def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do
+    Logger.debug("Received down from #{inspect(pid)}")
     {container_id, monitor_ref} = Map.fetch!(state, pid)
-    updated_state = cleanup(container_id, monitor_ref, pid, state)
 
-    {:noreply, updated_state}
+    case remove_container(container_id, monitor_ref, pid) do
+      :ok -> {:noreply, Map.delete(state, pid)}
+      # TODO how to handle containers that failed to shutdown
+      :error -> {:noreply, state}
+    end
   end
 
-  def handle_info({:EXIT, _ref, reason}, state) do
-    Logger.debug(inspect(reason))
-    Logger.debug(inspect(_ref))
-    # We will ignore port shutdown from calling System.cmd
+  # We needs to ignore port shutdown when calling System.cmd
+  def handle_info({:EXIT, port, _reason}, state) when is_port(port) do
     {:noreply, state}
   end
 
   @impl true
   def terminate(reason, state) do
-    Logger.debug("state #{inspect(state)}")
-    Logger.debug("Now terminating #{__MODULE__} #{inspect(reason)}")
-
-    updated_state =
-      Enum.reduce(state, state, fn {pid, {container_id, monitor_ref}}, state ->
-        cleanup(container_id, monitor_ref, pid, state)
-      end)
-
-    Logger.debug("Terminated #{__MODULE__}")
-
-    updated_state
+    Logger.debug(
+      "Now terminating #{__MODULE__} because of #{inspect(reason)} with state: #{inspect(state)}"
+    )
+
+    # TODO how to handle containers that failed to shutdown
+    Enum.reduce(state, state, fn {pid, {container_id, monitor_ref}}, state ->
+      case remove_container(container_id, monitor_ref, pid) do
+        :ok -> Map.delete(state, pid)
+        :error -> state
+      end
+    end)
+
+    Logger.debug("#{__MODULE__} Terminated")
   end
 
-  defp cleanup(container_id, monitor_ref, pid, state) do
-    Logger.debug("Shuting down #{inspect(container_id)} #{inspect(pid)}")
+  defp remove_container(container_id, monitor_ref, pid) do
+    Logger.debug(
+      "Shuting down container with id #{inspect(container_id)} controlled by process #{
+        inspect(pid)
+      }"
+    )
 
     Process.demonitor(monitor_ref)
-    {data, 0} = System.cmd("docker", ["rm", "-f", container_id])
 
-    Logger.debug(data)
+    case System.cmd("docker", ["rm", "-f", container_id]) do
+      {container_id, 0} ->
+        Logger.debug("#{container_id} shutdown")
+        :ok
 
-    Map.delete(state, pid)
+      {msg, _} ->
+        Logger.debug("#{container_id} failed to shutdown")
+        {:error, msg}
+    end
   end
 end

test/container_test.exs

@@ -8,38 +8,60 @@ defmodule ContainerTest do
     assert {:ok, container_id} = Container.run(:test, "alpine")
 
     process_id = self()
-
     assert {:ok, %{^process_id => {^container_id, _ref}}} = Container.list(:test)
 
+    resp = fetch_docker_ps(container_id)
+    # docker ps returns 11 char shortcode for the container id
+    assert String.contains?(resp, String.slice(container_id, 0..11))
+
     assert :ok = Container.stop(:test, container_id)
 
     assert {:ok, %{}} == Container.list(:test)
   end
 
-  test "If process is stopped then container is stopped" do
+  test "If owner process is stopped then container is stopped" do
     :net_kernel.start([:"test@127.0.0.1"])
     start_supervised!(Container)
 
     container_pid = Process.whereis(Container)
     {:ok, pid} = Agent.start(fn -> Container.run(:test, "alpine") end)
     {:ok, pid2} = Agent.start(fn -> Container.run(:test, "alpine") end)
 
+    # Both containers should exists
     assert {:ok, container_id} = Agent.get(pid, fn state -> state end)
     assert {:ok, container_id2} = Agent.get(pid2, fn state -> state end)
-
-    assert {:ok, %{^pid => {^container_id, _ref}}} = Container.list(:test)
+    assert {:ok, containers} = Container.list(:test)
+    assert %{^pid => {^container_id, _ref}, ^pid2 => {^container_id2, _ref2}} = containers
 
     Agent.stop(pid)
-
-    assert :ok = Container.stop(:test, container_id2)
-
+    # Container 1 should be stoped, but 2. container and container process should keep running
+    assert {:ok, state} = Container.list(:test)
+    assert pid2 in Map.keys(state)
+    assert pid not in Map.keys(state)
     assert container_pid == Process.whereis(Container)
   end
 
-  test "Start on remote node" do
-    pid = start_supervised!({Sidekick, {:test2, [Container]}})
+  test "Should work on remote node" do
+    sidekick_pid = start_supervised!({Sidekick, {:test2, [Container]}})
     assert {:ok, container_id} = Container.run(:test2, "alpine")
 
-    GenServer.stop(pid)
+    pid = self()
+    assert {:ok, %{^pid => {^container_id, _ref}}} = Container.list(:test2)
+
+    # Killing sidekick should clean up
+    :net_kernel.monitor_nodes(true)
+    resp = fetch_docker_ps(container_id)
+    # docker ps returns 11 char shortcode for the container id
+    assert String.contains?(resp, String.slice(container_id, 0..11))
+
+    GenServer.stop(sidekick_pid)
+    assert_receive {:nodedown, :"test2@127.0.0.1"}, 5000
+    resp = fetch_docker_ps(container_id)
+    refute String.contains?(resp, String.slice(container_id, 0..11))
+  end
+
+  defp fetch_docker_ps(container_id) do
+    {resp, 0} = System.cmd("docker", ["ps", "-f", "id=#{container_id}"])
+    resp
   end
 end