WIP: Use docker networks for connecting to services
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
Merge request reports
Activity
- Resolved by Carlo Mion
- Resolved by Carlo Mion
- Resolved by Carlo Mion
595 616 }, 596 617 } 597 618 619 //Define the network aliases used inside the network to reach this service 620 networkAliases := make([]string, 0) networkAliases := []string{networkAlias}
Edited by Nick ThomasDocker always sets its own DNS as default inside each container when connecting them to a custom network (as specified here it listens to 127.0.0.11).
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) - Resolved by Carlo Mion
- Resolved by Carlo Mion
- Resolved by Carlo Mion
- Resolved by Carlo Mion
- Resolved by Carlo Mion
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 ThomasThe side effect of using links between containers was the creation of these environment variables. In the case of
docker:dind
service, thedocker
build container gets injected theDOCKER_PORT_2375_TCP
environment variable, that is automatically picked up in the entrypoint script of the official Docker image (see here) and transformed in theDOCKER_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#19487With 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 usedocker: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
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.
@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?
@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?
Does this relates to #3173 (closed) ?
Edited by AntonThis also relates to #1042 (closed) and https://gitlab.com/gitlab-org/gitlab-ce/issues/20350
added 605 commits
-
69251b85...51af6972 - 603 commits from branch
gitlab-org:master
- b53a5a3a - Merge branch 'master' into add-network-per-build
- 5f09a449 - Fix after merge with master
-
69251b85...51af6972 - 603 commits from branch
mentioned in merge request !451 (closed)
added Community contribution label
@mion00 Can you retry the pipeline (or create a new one for this branch, https://gitlab.com/mion00/gitlab-ci-multi-runner/pipelines)? It seems like the shell coverage timed out.
added executordocker label
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)
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?
mentioned in merge request !1569 (merged)