conan-io / conan-package-tools

Conan Package Tools. Helps with massive package creation and CI integration (Travis CI, Appveyor...)
MIT License
165 stars 70 forks source link

Custom docker container name #495

Open nicopernas opened 4 years ago

nicopernas commented 4 years ago

Hi, we use conan for our packaging and it's great. Thanks for maintaining it!

We use the ConanMultiPackager packager which goes over docker, running through Jenkins. When we get to release time, there might be 2 or 3 different packages being built at the same time so chances that those run on the same Jenkins slave is high. In that case, docker barfs at us because we try to run two containers with the same image and name: conan_runner.

It looks like the conan_runner name is actually hard-coded. See runner.py +263. I would imagine there'd be no problem allowing the user to provide a custom name via ConanMultiPackager constructor, although such option is not currently available.

Is there a strong reason for that name to be hardcoded? Would you be open to accept a change like that?

Many thanks.

uilianries commented 4 years ago

Is there a strong reason for that name to be hardcoded? Would you be open to accept a change like that?

To be easier for management, but in your case, you have multiple containers at same environment. That's interesting because I used Gitlab + Docker + CPT in the past and hadn't this problem when building multiple jobs at same time.

db4 commented 3 years ago

I'm second to this. I'm trying to run two concurrent build jobs in Docker containers on the same Docker host. Each job, in turn, uses ConanMultiPackager(use_docker=True) to create a sibling Docker container for the actual work. The second one fails with

>>  docker run -e CONAN_ARCHS="x86" -e CONAN_CHANNEL="_" -e CONAN_DOCKER_ENTRY_SCRIPT="conan config set general.parallel_download=8 && conan user" -e CONAN_DOCKER_IMAGE="git:4567/docker/images/vs:2017" -e CONAN_DOCKER_RUN_OPTIONS="-e CI_JOB_TOKEN=[MASKED]" -e CONAN_LOCAL_REPO="http://artifactory/artifactory/api/conan/conan-local" -e CONAN_LOGIN_USERNAME="xxxxxxxx" -e CONAN_PASSWORD="xxxxxxxx" -e CONAN_REVISIONS_ENABLED="1" -e CONAN_STABLE_BRANCH_PATTERN="stable/*" -e CONAN_SYSREQUIRES_MODE="enabled" -e CONAN_UPLOAD="http://artifactory/artifactory/api/conan/conan-local@True@conan-local" -e CONAN_UPLOAD_DEPENDENCIES="all" -e CONAN_USERNAME="_" -e CONAN_VISUAL_RUNTIMES="MD,MDd" -e CONAN_VISUAL_VERSIONS="15" -e CONAN_REFERENCE="opencv/4.5.3@_/_" -e CPT_PROFILE="@@include(default)@@@@[settings]@@arch=x86@@build_type=Release@@compiler=Visual Studio@@compiler.runtime=MD@@compiler.version=15@@os=Windows@@[options]@@opencv:shared=True@@opencv:with_ipp=True@@[env]@@@@[build_requires]@@@@" -e CONAN_TEMP_TEST_FOLDER="1" -e CPT_UPLOAD_ENABLED="True" -e CPT_UPLOAD_RETRY="3" -e CPT_UPLOAD_ONLY_RECIPE="True" -e CPT_UPLOAD_FORCE="True" -e CPT_BUILD_POLICY="opencv,outdated" -e CPT_UPLOAD_DEPENDENCIES="all" -e CPT_CONANFILE="recipes\opencv\4.x\conanfile.py" --name conan_runner  -e CI_JOB_TOKEN=[MASKED] git:4567/docker/images/vs:2017 cmd /C " pip install conan_package_tools==0.35.1 --upgrade --no-cache &&  pip install conan==1.40.1 --no-cache"
____________________________________________________________________________________________________
docker: Error response from daemon: Conflict. The container name "/conan_runner" is already in use by container "b87c80b55924efa5a9dd506cfca2f8e7232544d8c8bc6ac88738fd8f6f820c50". You have to remove (or rename) that container to be able to reuse that name.
See 'docker run --help'.

Maybe just add a random suffix to conan_runner?

uilianries commented 3 years ago

@db4 It will require a new feature.

db4 commented 3 years ago

What "feature"? I've just (hopefully) fixed it locally as follows:

--- runner.py.orig  2021-09-15 11:39:01.997868300 +0300
+++ runner.py   2021-09-15 11:39:02.086886100 +0300
@@ -1,3 +1,4 @@
+import uuid
 import os
 import sys
 import subprocess
@@ -271,9 +272,11 @@
                 # Update the downloaded image
                 with self.printer.foldable_output("update conan"):
                     try:
-                        command = '%s docker run %s --name conan_runner ' \
+                        conan_runner = "conan_runner_" + str(uuid.uuid1())
+                        command = '%s docker run %s --name %s ' \
                                   ' %s %s %s "%s"' % (self._sudo_docker_command,
                                                    env_vars_text,
+                                                   conan_runner,
                                                    self._docker_run_options,
                                                    self._docker_image,
                                                    self._docker_shell,
@@ -284,13 +287,14 @@
                             raise Exception("Error updating the image: %s" % command)
                         # Save the image with the updated installed
                         # packages and remove the intermediate container
-                        command = "%s docker commit conan_runner %s" % (self._sudo_docker_command,
+                        command = "%s docker commit %s %s" % (self._sudo_docker_command,
+                                                                        conan_runner,
                                                                         self._docker_image)
                         ret = self._runner(command)
                         if ret != 0:
                             raise Exception("Error commiting the image: %s" % command)
                     finally:
-                        command = "%s docker rm conan_runner" % self._sudo_docker_command
+                        command = "%s docker rm %s" % (self._sudo_docker_command, conan_runner)
                         ret = self._runner(command)
                         if ret != 0:
                             raise Exception("Error removing the temp container: %s" % command)
uilianries commented 3 years ago

@db4 users should be able to enable/disable that suffix, also, it require a new test. If you are interested on this feature, please, open a PR. Plus, UUID1 is too long, it could be reduced to str(uuid.uuid1())[:4]