Skip to content

Fix missing logs from docker executor

Lyubomir Raykov requested to merge wait-for-logs-docker into master

I was writing integration tests for !1963 (merged) when I stumbled on docker ones not passing due to missing output.

This change fixed it. What I think happens is that the container exits, the function ends, so the reader closes (defer hijacked.Close()) and nothing waits to make sure that all logs are copied to the job trace.

What does this MR do?

Adds a channel which signals that logs have finished writing to the job trace.

Why was this MR needed?

Might be related to gitlab#217199 (moved)

Are there points in the code the reviewer needs to double check?

This is a quick fix of the issue, there's an edge-case that it doesn't handle well:

  • If stdcopy.StdCopy produces an error after the container has exited, it won't be handled (this is true before this fix as well)

Also I couldn't write a reliable test for it. Seems like we need multiple steps support to do it (see discussion). Added a followup issue: #25675.

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Added tests for this feature/bug
  • In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Edited by Steve Xuereb

Merge request reports