Skip to content
Snippets Groups Projects

Introduce new docker strategy for build container

What does this MR do?

The way the build script is executing on the build container is by starting the container with the script as ps1, as soon as the script is done, the container exists.

Follow the same flow the kubernetes executor, where the build container is started in detached mode and the build script are executed on top of the running build container.

This is done so interactive web terminal have the same behavior as kubernetes, where even after the main script is done the user stays connected to the web terminal.

e as kubernetes and shell executor

Limitations for DinD

When using docker in docker the --link does not seem to work at all when we run docker exec on the container. I have opened a question in https://forums.docker.com/t/cant-access-docker-socket-with-docker-exec/59190 but I might open an issue upstream for this. Even though I'm pretty sure it will not be supported since --link is deprecated and docker in docker is being phased out.

Why was this MR needed?

For context please read the following issue

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

  • docker-ssh shouldn't need any changes right?
  • I wonder if we should have tests around this.
  • Is there any docuemntation that needs to be updated? I can't find it anywhere

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?

closes #3605

Edited by Wayne Haber

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Steve Xuereb added 1 commit

    added 1 commit

    • 26c640df - Start build container in detached with tty

    Compare with previous version

  • Steve Xuereb assigned to @SteveAzz

    assigned to @SteveAzz

  • Steve Xuereb added 1 commit

    added 1 commit

    • d2566758 - Start build container detached with tty

    Compare with previous version

  • Steve Xuereb added 1 commit

    added 1 commit

    • 6f1816bd - Start build container detached with tty

    Compare with previous version

  • @SteveAzz

    This in general looks nice. However, I did not dive deep yet. I think that we should finish Terminal Docker first, as the other MR seems way simpler in terms of complexity.

    My biggest takeaway of this work:

    1. We cannot change today's behavior, so we have to introduce a feature flag (environment variable?) to use a new method,
    2. Once we introduce we enable that on GitLab.com to measure the impact, and then switch a default to always use docker/exec/attach approach, instead of today's docker/run/attach.

    My proposal is that we finish the !1008 (merged), even though we know limitations. It seems pretty straightforward otherwise. Then we try to get this MR merged for %11.4 in our best effort.

    Edited by Kamil Trzciński
  • Steve Xuereb mentioned in issue #3605

    mentioned in issue #3605

  • Steve Xuereb marked as a Work In Progress

    marked as a Work In Progress

  • Steve Xuereb changed title from Start build container detached with tty to WIP: Introduce new docker strategy for build container

    changed title from Start build container detached with tty to WIP: Introduce new docker strategy for build container

  • Steve Xuereb changed the description

    changed the description

  • Steve Xuereb resolved all discussions

    resolved all discussions

  • Steve Xuereb added Stretch label and removed Deliverable label

    added Stretch label and removed Deliverable label

  • Steve Xuereb added 1 commit

    added 1 commit

    • 92ac642c - Add DOCKER_STRATEGY variable for docker executor

    Compare with previous version

  • Steve Xuereb marked the checklist item Added tests for this feature/bug as completed

    marked the checklist item Added tests for this feature/bug as completed

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading