Skip to content
Snippets Groups Projects

WIP: Use docker networks for connecting to services

Closed Carlo Mion requested to merge mion00/gitlab-ci-multi-runner:add-network-per-build into master
6 unresolved threads

What does this MR do?

This MR is build on the previous work on !451 (closed) by @dooferlad, to use Docker networks instead of links for inter-container communication.

I added a function connectToBuildNetwork to connect the created containers to a special build network, that is removed after the job finishes.

Question: do we want to support both legacy links and network based on the API version of Docker? See here for the conditional switch between the two.

I saw this commit 226ecfe0, so if it is merged we can safely deprecate links, given that with that minimum API of Docker we assume to have networking available.

Why was this MR needed?

Container links are deprecated and discouraged by Docker

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

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Tests
    • Added for this feature/bug
    • All builds are passing
  • Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

#1042 (closed) #2511 (closed) #3173 (closed) gitlab-org/gitlab-ce#20350

Edited by Carlo Mion

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
  • Nick Thomas
  • Nick Thomas
  • 595 616 },
    596 617 }
    597 618
    619 //Define the network aliases used inside the network to reach this service
    620 networkAliases := make([]string, 0)
  • 748 // If there is anything left from "min" but not from "version", ensure it is 0
    749 for i++; i < len(m); i++ {
    750 fMin, err := strconv.ParseFloat(m[i], 32)
    751 if err != nil {
    752 return false, err
    753 }
    754 if fMin > 0 {
    755 return false, nil
    756 }
    757 }
    758
    759 return true, nil
    760 }
    761
    762 func (s *executor) createBuildNetwork() (err error) {
    763 version, err := s.client.ServerVersion(s.Context)
  • Nick Thomas
  • Nick Thomas
  • Nick Thomas
  • Nick Thomas
  • 686 719 return
    687 720 }
    688 721
    722 // minVersion returns a bool of if version >= min when two . separated versions strings are parsed.
  • Nick Thomas
  • 1015 1156 return err
    1016 1157 }
    1017 1158
    1159 s.Debugln("Creating build network...")
    1160 err = s.createBuildNetwork()
  • 265 265 }
    266 266 build.Variables = append(build.Variables, common.JobVariable{
    267 267 Key: "GIT_STRATEGY", Value: strategy,
    268 }, common.JobVariable{
    269 //We need to specify how to reach the docker:dind service,
    • Will using networks then break non-test users of the docker executor as well? Is there any way we can preserve the current behaviour?

      Edited by Nick Thomas
    • The side effect of using links between containers was the creation of these environment variables. In the case of docker:dind service, the docker build container gets injected the DOCKER_PORT_2375_TCP environment variable, that is automatically picked up in the entrypoint script of the official Docker image (see here) and transformed in the DOCKER_HOST environment, used by the docker client.

      This is why a custom name for the docker:dind service does not work out of the box, as seen in gitlab-org/gitlab-ce#19487

      With networks, there is no more environment variable or /etc/hosts file manipulation.

      We can check if there is a docker:dind in the services, and add the aforementioned env variable, but this feels like an hack, because there can be other builds that will break because they assumed the presence of the environment variables.

    • @nick.thomas Currently user also need to set DOCKER_HOST: tcp://docker:2375/ variable if he want to use docker:dind service. Even we in this project are doing this: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/blob/master/.gitlab-ci.yml#L24 ;)

      Edited by Tomasz Maczukin
    • Please register or sign in to reply
  • Thanks @mion00 - i've made a bunch of style comments but there are a couple of architectural ones that should probably be resolved before worrying about that. https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/merge_requests/609#note_32408860 and https://gitlab.com/gitlab-org/gitlab-ci-multi-runner/merge_requests/609#note_32409027 especially.

  • Carlo Mion added 1 commit

    added 1 commit

    Compare with previous version

  • Carlo Mion added 1 commit

    added 1 commit

    Compare with previous version

  • Carlo Mion added 1 commit

    added 1 commit

    Compare with previous version

  • @nick.thomas does anything else need to be done here? I think @mion00 has addressed those 2 comments you wanted to discuss but might be missing something.

    Would be really good to get this in at some point because we've been playing with some truly horrible hacks to try to get services talking across each other that we could drop once this is shipped.

  • Sorry, this one completely fell off my radar. @tmaczukin can you adopt?

  • Are there any further blockers to merging this change? I have a use-case for gitlab CI which I cannot resolve without the ability to connect from one service to another by name.

  • @tmaczukin sorry to pester about this but we're starting to accumulate some horrible hacks that I'd love to start taking out of our code bases and standardising on service properly instead. Is there anything else that needs to be done here?

  • Any luck here? It looks forgotten but can greatly improve situation for end2end tests in CI

  • Does this relates to #3173 (closed) ?

    Edited by Anton
  • Carlo Mion changed the description

    changed the description

  • Carlo Mion added 605 commits

    added 605 commits

    Compare with previous version

  • Eric Eastwood mentioned in merge request !451 (closed)

    mentioned in merge request !451 (closed)

  • Carlo Mion added 1 commit

    added 1 commit

    Compare with previous version

  • Ghost User mentioned in merge request !895 (closed)

    mentioned in merge request !895 (closed)

  • Guys, I've re-implemented this MR with some additions and tested it successfully in our company's build environment, see !895 (closed). Let us please consolidate effort on merging this feature in coming Gitlab releases.

  • mentioned in issue #1042 (closed)

  • This is heavely needed to test docker images that rely on services like mysql. When will this be integrated as it seems to work?

  • Michael Krotscheck mentioned in merge request !1041 (closed)

    mentioned in merge request !1041 (closed)

  • Can we please have an update on this? Feels like pretty important thing counting that it removes usage of deprecated functionality and is very important to the community ( see #1042 (closed), #2127 (closed) ).

  • I believe, this merge request should be closed in favor for !1041 (closed)

  • You're right, !1041 (closed) is the current MR for this functionality. Could someone close this one?

  • closed

  • Steve Exley mentioned in merge request !1569 (merged)

    mentioned in merge request !1569 (merged)

  • Please register or sign in to reply
    Loading