Merge pull request #1525 from manics/builder-failed-fail

If build pod is deleted check status instead of assuming Succeeded

Author: consideRatio

binderhub/build.py

@@ -42,11 +42,11 @@ class BuildStatus(Enum):
         Used when `kind` is `Kind.BUILD_STATUS_CHANGE`
         """
 
-        PENDING = 1
-        RUNNING = 2
-        COMPLETED = 3
-        FAILED = 4
-        UNKNOWN = 5
+        PENDING = "pending"
+        RUNNING = "running"
+        BUILT = "built"
+        FAILED = "failed"
+        UNKNOWN = "unknown"
 
     def __init__(self, kind: Kind, payload: Union[str, BuildStatus]):
         self.kind = kind
@@ -460,11 +460,22 @@ def submit(self):
                     _request_timeout=KUBE_REQUEST_TIMEOUT,
                 ):
                     if f["type"] == "DELETED":
-                        # Assume this is a successful completion
-                        self.progress(
-                            ProgressEvent.Kind.BUILD_STATUS_CHANGE,
-                            ProgressEvent.BuildStatus.COMPLETED,
+                        phase = f["object"].status.phase
+                        app_log.debug(
+                            "Pod %s was deleted with phase %s",
+                            f["object"].metadata.name,
+                            phase,
                         )
+                        if phase == "Succeeded":
+                            self.progress(
+                                ProgressEvent.Kind.BUILD_STATUS_CHANGE,
+                                ProgressEvent.BuildStatus.BUILT,
+                            )
+                        else:
+                            self.progress(
+                                ProgressEvent.Kind.BUILD_STATUS_CHANGE,
+                                ProgressEvent.BuildStatus.FAILED,
+                            )
                         return
                     self.pod = f["object"]
                     if not self.stop_event.is_set():
@@ -699,7 +710,7 @@ def stream_logs(self):
                 ),
             )
         self.progress(
-            ProgressEvent.Kind.BUILD_STATUS_CHANGE, ProgressEvent.BuildStatus.COMPLETED
+            ProgressEvent.Kind.BUILD_STATUS_CHANGE, ProgressEvent.BuildStatus.BUILT
         )
         self.progress(
             "log",

binderhub/build_local.py

@@ -140,7 +140,7 @@ def break_callback():
                 self._handle_log(line)
             self.progress(
                 ProgressEvent.Kind.BUILD_STATUS_CHANGE,
-                ProgressEvent.BuildStatus.COMPLETED,
+                ProgressEvent.BuildStatus.BUILT,
             )
         except subprocess.CalledProcessError:
             self.progress(

binderhub/builder.py

@@ -509,12 +509,13 @@ def _check_result(future):
                 # FIXME: If pod goes into an unrecoverable stage, such as ImagePullBackoff or
                 # whatever, we should fail properly.
                 if progress.kind == ProgressEvent.Kind.BUILD_STATUS_CHANGE:
+                    phase = progress.payload.value
                     if progress.payload == ProgressEvent.BuildStatus.PENDING:
                         # nothing to do, just waiting
                         continue
-                    elif progress.payload == ProgressEvent.BuildStatus.COMPLETED:
+                    elif progress.payload == ProgressEvent.BuildStatus.BUILT:
                         event = {
-                            "phase": "built",
+                            "phase": phase,
                             "message": "Built image, launching...\n",
                             "imageName": image_name,
                         }
@@ -525,13 +526,17 @@ def _check_result(future):
                             log_future = pool.submit(build.stream_logs)
                             log_future.add_done_callback(_check_result)
                         continue
-                    elif progress.payload == ProgressEvent.BuildStatus.COMPLETED:
+                    elif progress.payload == ProgressEvent.BuildStatus.BUILT:
                         # Do nothing, is ok!
                         continue
                     elif progress.payload == ProgressEvent.BuildStatus.FAILED:
-                        event = {"phase": "failure"}
+                        event = {"phase": phase}
                     elif progress.payload == ProgressEvent.BuildStatus.UNKNOWN:
-                        event = {"phase": "unknown"}
+                        event = {"phase": phase}
+                    else:
+                        raise ValueError(
+                            f"Found unknown phase {phase} in ProgressEvent"
+                        )
                 elif progress.kind == ProgressEvent.Kind.LOG_MESSAGE:
                     # The logs are coming out of repo2docker, so we expect
                     # them to be JSON structured anyway

binderhub/tests/test_build.py

@@ -94,6 +94,30 @@ async def test_build(app, needs_build, needs_launch, always_build, slug, pytestc
     assert r.url.startswith(final["url"])
 
 
+@pytest.mark.asyncio(timeout=120)
+@pytest.mark.remote
+async def test_build_fail(app, needs_build, needs_launch, always_build, pytestconfig):
+    """
+    Test build a repo that should fail immediately.
+    """
+    slug = "gh/binderhub-ci-repos/minimal-dockerfile/failed"
+    build_url = f"{app.url}/build/{slug}"
+    r = await async_requests.get(build_url, stream=True)
+    r.raise_for_status()
+    failed_events = 0
+    async for line in async_requests.iter_lines(r):
+        line = line.decode("utf8", "replace")
+        if line.startswith("data:"):
+            event = json.loads(line.split(":", 1)[1])
+            assert event.get("phase") not in ("launching", "ready")
+            if event.get("phase") == "failed":
+                failed_events += 1
+                break
+    r.close()
+
+    assert failed_events > 0, "Should have seen phase 'failed'"
+
+
 def _list_dind_pods_mock():
     """Mock list of DIND pods"""
     mock_response = mock.MagicMock()
@@ -235,7 +259,7 @@ async def test_local_repo2docker_build():
         event = await q.get(10)
         if (
             event.kind == ProgressEvent.Kind.BUILD_STATUS_CHANGE
-            and event.payload == ProgressEvent.BuildStatus.COMPLETED
+            and event.payload == ProgressEvent.BuildStatus.BUILT
         ):
             break
         events.append(event)