This is an automated email from the ASF dual-hosted git repository.
tbonelee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new b572a07156 [ZEPPELIN-5876] Implement
DockerInterpreterProcess.isAlive() using container state
b572a07156 is described below
commit b572a07156324c6ae9c08da6ac82c9215877741f
Author: HyeonUk Kang <[email protected]>
AuthorDate: Mon Jul 20 23:03:00 2026 +0900
[ZEPPELIN-5876] Implement DockerInterpreterProcess.isAlive() using
container state
### What is this PR for?
The Docker launcher's isAlive() isn't really implemented. It just calls
isRunning(), which only checks whether the interpreter's Thrift port accepts a
connection (checkIfRemoteEndpointAccessible). That tells you the port is open,
not that the process behind it is alive. A container can be OOMKilled or
already gone and still look alive if the port happens to answer for a moment.
It also doesn't match the InterpreterClient contract, which says isAlive should
reflect process status and s [...]
While I was there I filled in getErrorMessage(), which always returned
null. When the container isn't running it now says why: OOMKilled, or a
non-zero exit code. That logic lives in a small describeContainerFailure()
helper to keep getErrorMessage() short.
One change is test-only: the docker field is now 'VisibleForTesting', so
tests can inject a mock DockerClient and skip needing a real daemon.
### What type of PR is it?
Feature
### Todos
- [x] Implement isAlive() from the container's actual state (running/paused)
- [x] Implement getErrorMessage() (OOMKilled / exit code)
- [x] Unit tests with a mocked DockerClient
### What is the Jira issue?
[[ZEPPELIN-5876]](https://issues.apache.org/jira/browse/ZEPPELIN-5876)
### How should this be tested?
Automated unit tests in DockerInterpreterProcessTest :
- isAlive_trueWhenContainerRunning - running == true → alive
- isAlive_falseWhenContainerExitedOrOomKilled — running == false → not alive
- getErrorMessage_reportsOomKilled - oomKilled == true → message contains
the reason
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? - no
* Is there breaking changes for older versions? - no
* Does this needs documentation? - no
Closes #5316 from hyunw9/ZEPPELIN-5876.
Signed-off-by: ChanHo Lee <[email protected]>
---
.../launcher/DockerInterpreterProcess.java | 47 +++++++++++++-
.../launcher/DockerInterpreterProcessTest.java | 71 ++++++++++++++++++----
2 files changed, 104 insertions(+), 14 deletions(-)
diff --git
a/zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java
b/zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java
index 3004ae13f7..9c86a67608 100644
---
a/zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java
+++
b/zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java
@@ -46,6 +46,7 @@ import com.spotify.docker.client.exceptions.DockerException;
import com.spotify.docker.client.messages.Container;
import com.spotify.docker.client.messages.ContainerConfig;
import com.spotify.docker.client.messages.ContainerCreation;
+import com.spotify.docker.client.messages.ContainerState;
import com.spotify.docker.client.messages.ExecCreation;
import com.spotify.docker.client.messages.HostConfig;
import com.spotify.docker.client.messages.PortBinding;
@@ -466,8 +467,20 @@ public class DockerInterpreterProcess extends
RemoteInterpreterProcess {
@Override
public boolean isAlive() {
- //TODO(ZEPPELIN-5876): Implement it more accurately
- return isRunning();
+ DockerClient client = docker;
+ if (client == null) {
+ return false;
+ }
+ try {
+ ContainerState state = client.inspectContainer(containerName).state();
+ return Boolean.TRUE.equals(state.running()) ||
Boolean.TRUE.equals(state.paused());
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return false;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to inspect container {} for liveness",
containerName, e);
+ return false;
+ }
}
@Override
@@ -480,6 +493,36 @@ public class DockerInterpreterProcess extends
RemoteInterpreterProcess {
@Override
public String getErrorMessage() {
+ DockerClient client = docker;
+ if (client == null) {
+ return null;
+ }
+ try {
+ ContainerState state = client.inspectContainer(containerName).state();
+ return describeContainerFailure(state);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return null;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to inspect container {} for error message",
containerName, e);
+ return null;
+ }
+ }
+
+ // Returns null when the container is still running or exited cleanly,
+ // otherwise a human-readable reason for the failure.
+ private String describeContainerFailure(ContainerState state) {
+ if (Boolean.TRUE.equals(state.running())) {
+ return null;
+ }
+ if (Boolean.TRUE.equals(state.oomKilled())) {
+ return "Interpreter container " + containerName
+ + " was OOMKilled (exitCode=" + state.exitCode() + ")";
+ }
+ Long exitCode = state.exitCode();
+ if (exitCode != null && exitCode != 0) {
+ return "Interpreter container " + containerName + " exited with code " +
exitCode;
+ }
return null;
}
diff --git
a/zeppelin-plugins/launcher/docker/src/test/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcessTest.java
b/zeppelin-plugins/launcher/docker/src/test/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcessTest.java
index ea5b5bd84a..a4b0b4910b 100644
---
a/zeppelin-plugins/launcher/docker/src/test/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcessTest.java
+++
b/zeppelin-plugins/launcher/docker/src/test/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcessTest.java
@@ -20,6 +20,8 @@ import com.spotify.docker.client.DockerClient;
import com.spotify.docker.client.exceptions.DockerException;
import com.spotify.docker.client.messages.ContainerConfig;
import com.spotify.docker.client.messages.ContainerCreation;
+import com.spotify.docker.client.messages.ContainerInfo;
+import com.spotify.docker.client.messages.ContainerState;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
import org.apache.zeppelin.conf.ZeppelinConfiguration.ConfVars;
import org.apache.zeppelin.interpreter.InterpreterOption;
@@ -69,7 +71,17 @@ class DockerInterpreterProcessTest {
5000, 10);
}
- // stop() must always close the DockerClient, even when killing the container
+ // Stubs docker.inspectContainer(...).state() and returns the ContainerState
mock
+ // so each test can set running/paused/oomKilled/exitCode as needed.
+ private ContainerState stubContainerState(DockerClient mockDocker) throws
Exception {
+ ContainerInfo info = mock(ContainerInfo.class);
+ ContainerState state = mock(ContainerState.class);
+ when(mockDocker.inspectContainer(anyString())).thenReturn(info);
+ when(info.state()).thenReturn(state);
+ return state;
+ }
+
+ // #1: stop() must always close the DockerClient, even when killing the
container
// fails, so the underlying HTTP socket / file descriptors are never leaked.
@Test
void stop_alwaysClosesDockerClient_evenWhenKillContainerFails() throws
Exception {
@@ -83,26 +95,23 @@ class DockerInterpreterProcessTest {
verify(mockDocker, times(1)).close();
}
- // When start() fails after the container has been started (e.g. file copy /
exec
- // fails), the container must be cleaned up instead of being left orphaned.
+ // #2: when start() fails after the container has been started, it must be
rolled
+ // back (kill + remove) instead of being left orphaned.
@Test
void start_removesContainer_whenContainerPreparationFails() throws Exception
{
DockerInterpreterProcess intp = spy(newProcess());
DockerClient mockDocker = mock(DockerClient.class);
doReturn(mockDocker).when(intp).createDockerClient(anyString());
- // No pre-existing container to remove.
when(mockDocker.listContainers(any())).thenReturn(Collections.emptyList());
- // Container is created and started successfully...
when(mockDocker.createContainer(any(ContainerConfig.class), anyString()))
.thenReturn(ContainerCreation.builder().id("test-container-id").build());
- // ...but preparing it (the first exec inside the container) fails.
+ // Container is created and started, then preparation (the first exec)
fails.
doThrow(new DockerException("exec failed"))
.when(mockDocker).execCreate(anyString(), any(String[].class), any());
assertThrows(IOException.class, () -> intp.start("user1"));
- // The container was started, so start() must roll it back before
returning.
verify(mockDocker).startContainer("test-container-id");
verify(mockDocker).killContainer(anyString());
verify(mockDocker).removeContainer(anyString());
@@ -115,19 +124,57 @@ class DockerInterpreterProcessTest {
doReturn(mockDocker).when(intp).createDockerClient(anyString());
when(mockDocker.listContainers(any())).thenReturn(Collections.emptyList());
- // Container is created...
when(mockDocker.createContainer(any(ContainerConfig.class), anyString()))
.thenReturn(ContainerCreation.builder().id("test-container-id").build());
- // ...but fails to start, so it is created-but-not-running.
- doThrow(new DockerException("start
failed")).when(mockDocker).startContainer(anyString());
- // Killing a non-running container fails, but removeContainer must still
fire.
- doThrow(new DockerException("not
running")).when(mockDocker).killContainer(anyString());
+ // Container is created and started, then preparation (the first exec)
fails.
+ doThrow(new DockerException("exec failed"))
+ .when(mockDocker).execCreate(anyString(), any(String[].class), any());
+ // ...and killing the container during cleanup fails too.
+ doThrow(new DockerException("kill
failed")).when(mockDocker).killContainer(anyString());
assertThrows(IOException.class, () -> intp.start("user1"));
+ // removeContainer must still fire despite the kill failure.
+ verify(mockDocker).killContainer(anyString());
verify(mockDocker).removeContainer(anyString());
}
+ // isAlive() reflects the container's actual state from the Docker daemon,
+ // not just whether the Thrift port is reachable.
+ @Test
+ void isAlive_trueWhenContainerRunning() throws Exception {
+ DockerInterpreterProcess intp = newProcess();
+ DockerClient mockDocker = mock(DockerClient.class);
+ intp.docker = mockDocker;
+ ContainerState state = stubContainerState(mockDocker);
+ when(state.running()).thenReturn(true);
+
+ assertTrue(intp.isAlive());
+ }
+
+ @Test
+ void isAlive_falseWhenContainerNotRunning() throws Exception {
+ DockerInterpreterProcess intp = newProcess();
+ DockerClient mockDocker = mock(DockerClient.class);
+ intp.docker = mockDocker;
+ ContainerState state = stubContainerState(mockDocker);
+ when(state.running()).thenReturn(false);
+
+ assertFalse(intp.isAlive());
+ }
+
+ @Test
+ void getErrorMessage_reportsOomKilled() throws Exception {
+ DockerInterpreterProcess intp = newProcess();
+ DockerClient mockDocker = mock(DockerClient.class);
+ intp.docker = mockDocker;
+ ContainerState state = stubContainerState(mockDocker);
+ when(state.oomKilled()).thenReturn(true);
+ when(state.exitCode()).thenReturn(137L);
+
+ assertTrue(intp.getErrorMessage().contains("OOMKilled"));
+ }
+
@Test
void testCreateIntpProcess() throws IOException {
DockerInterpreterLauncher launcher
