build: build the docker image in teamcity

[ZhouXing19]

This commit builds the docker image based on script from build/deploy, and
saves the docker image in the artifacts directory.

Note: the associated build configuration is at Cockroach >> Scratch Project >> Build Docker Image (here).
Once this PR is merged, we will move this config to Cockroach > CI > Builds.
Please run this config upon this branch (#72394).

Release note: None

[cockroach-teamcity]

This change is 

[rail]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder and @ZhouXing19)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 14 at r1 (raw file):

tc_start_block "Copy cockroach binary and dependency files to build/deploy"

cp upstream_artifacts/cockroach build/deploy

Can you add comments where these file come from please.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 15 at r1 (raw file):

cp upstream_artifacts/cockroach build/deploy
cp lib.docker_amd64/libgeos.so lib.docker_amd64/libgeos_c.so build/deploy

Hmm, these are implicitly (not clear enough from this script) generated by build/builder.sh mkrelease linux-gnu. Can we copy these from the upstream dependency instead? Looks like they are in

bazel-bin/c-deps/libgeos/lib/libgeos.so
bazel-bin/c-deps/libgeos/lib/libgeos_c.so

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 18 at r1 (raw file):

cp -r licenses build/deploy/

chmod u+x build/deploy/cockroach

Can you set the permissions explicitly, 755 would be great.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 25 at r1 (raw file):

docker_image_tar_name="cockroach-docker-image.tar"
docker_tag="demo_docker_image/demo"

The tag usually uses the "$owner/$image_name" format. Maybe use something like "cockroachdb/cockroach-demo"?

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 31 at r1 (raw file):

  build/deploy

docker save -o "${artifacts}/${docker_image_tar_name}" "$docker_tag"

I'd also compress the image to save some space. :)
docker save "$docker_tag" | gzip > "${artifacts}/${docker_image_tar_name}".gz

Do you plan to publish these images to dockerhub instead of leaving them as artifacts? I'm a bit worried about the fact that we may significantly increase our disk usage with this change.

We can create a separate repo for this image under https://hub.docker.com/u/cockroachdb

There are some examples on how to make this script publish to dockerhub, see

cockroach/build/release/teamcity-publish-release.sh

Lines 92 to 111 in 32d82ec

	tc_start_block "Make and push docker images"
	configure_docker_creds
	docker_login_with_google
	docker_login
	# TODO: update publish-provisional-artifacts with option to leave one or more cockroach binaries in the local filesystem?
	curl -f -s -S -o- "https://${s3_download_hostname}/cockroach-${build_name}.linux-amd64.tgz" | tar ixfz - --strip-components 1
	cp cockroach lib/libgeos.so lib/libgeos_c.so build/deploy
	cp -r licenses build/deploy/
	docker build \
	--label version=$version \
	--no-cache \
	--tag=${dockerhub_repository}:{"$build_name",latest,latest-"${release_branch}"} \
	--tag=${gcr_repository}:${build_name} \
	build/deploy
	docker push "${dockerhub_repository}:${build_name}"
	docker push "${gcr_repository}:${build_name}"
	tc_end_block "Make and push docker images"

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder, @rail, and @ZhouXing19)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 14 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Can you add comments where these file come from please.

Done, thanks!

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 15 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Hmm, these are implicitly (not clear enough from this script) generated by build/builder.sh mkrelease linux-gnu. Can we copy these from the upstream dependency instead? Looks like they are in

bazel-bin/c-deps/libgeos/lib/libgeos.so
bazel-bin/c-deps/libgeos/lib/libgeos_c.so

Tried using the ones from bazel-bin but it gave

[02:06:37]W:		 [Copy cockroach binary and dependency files to build/deploy] cp: cannot stat 'bazel-bin/c-deps/libgeos/lib/libgeos.so': No such file or directory
[02:06:37]W:		 [Copy cockroach binary and dependency files to build/deploy] cp: cannot stat 'bazel-bin/c-deps/libgeos/lib/libgeos_c.so': No such file or directory

I also did find . -ls and libgeos.so and libgeos_c.so are only in

[02:06:36] :	 [Step 3/4]   2847419  11844 -rwxr-xr-x    1 agent    agent     12126064 Nov  9 01:58 ./lib.docker_amd64/libgeos.so
[02:06:36] :	 [Step 3/4]   2847407    464 -rwxr-xr-x    1 agent    agent       471920 Nov  9 01:58 ./lib.docker_amd64/libgeos_c.so

See build log here.

Is it because I didn't add the correct build config to the dependency?

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 18 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Can you set the permissions explicitly, 755 would be great.

Done.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 31 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

I'd also compress the image to save some space. :)
docker save "$docker_tag" | gzip > "${artifacts}/${docker_image_tar_name}".gz

Do you plan to publish these images to dockerhub instead of leaving them as artifacts? I'm a bit worried about the fact that we may significantly increase our disk usage with this change.

We can create a separate repo for this image under https://hub.docker.com/u/cockroachdb

There are some examples on how to make this script publish to dockerhub, see

cockroach/build/release/teamcity-publish-release.sh

Lines 92 to 111 in 32d82ec

	tc_start_block "Make and push docker images"
	configure_docker_creds
	docker_login_with_google
	docker_login
	# TODO: update publish-provisional-artifacts with option to leave one or more cockroach binaries in the local filesystem?
	curl -f -s -S -o- "https://${s3_download_hostname}/cockroach-${build_name}.linux-amd64.tgz" | tar ixfz - --strip-components 1
	cp cockroach lib/libgeos.so lib/libgeos_c.so build/deploy
	cp -r licenses build/deploy/
	docker build \
	--label version=$version \
	--no-cache \
	--tag=${dockerhub_repository}:{"$build_name",latest,latest-"${release_branch}"} \
	--tag=${gcr_repository}:${build_name} \
	build/deploy
	docker push "${dockerhub_repository}:${build_name}"
	docker push "${gcr_repository}:${build_name}"
	tc_end_block "Make and push docker images"

Do we need to create a repo in the dockerhub page before pushing?
I tried following teamcity-publish-release.sh and changed the docker_tag to the one with dockerhub_repository.
Just left docker push commented out because I wanted to make sure everything is ok before pushing.

[rail]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder and @ZhouXing19)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 14 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Done, thanks!

I just added 2 more, can you also add them :)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 15 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Tried using the ones from bazel-bin but it gave

[02:06:37]W:		 [Copy cockroach binary and dependency files to build/deploy] cp: cannot stat 'bazel-bin/c-deps/libgeos/lib/libgeos.so': No such file or directory
[02:06:37]W:		 [Copy cockroach binary and dependency files to build/deploy] cp: cannot stat 'bazel-bin/c-deps/libgeos/lib/libgeos_c.so': No such file or directory

I also did find . -ls and libgeos.so and libgeos_c.so are only in

[02:06:36] :	 [Step 3/4]   2847419  11844 -rwxr-xr-x    1 agent    agent     12126064 Nov  9 01:58 ./lib.docker_amd64/libgeos.so
[02:06:36] :	 [Step 3/4]   2847407    464 -rwxr-xr-x    1 agent    agent       471920 Nov  9 01:58 ./lib.docker_amd64/libgeos_c.so

See build log here.

Is it because I didn't add the correct build config to the dependency?

Sorry, should have been more specific. You needed to add those files as upstream artifacts explicitly in the build configuration. I added them and now the rules look like this:

bazel-bin/pkg/cmd/cockroach/cockroach_/cockroach=>upstream_artifacts
bazel-bin/c-deps/libgeos/lib/libgeos.so=>upstream_artifacts
bazel-bin/c-deps/libgeos/lib/libgeos_c.so=>upstream_artifacts

Now you can copy everything as:

cp \
  upstream_artifacts/cockroach \
  upstream_artifacts/libgeos.so \
  upstream_artifacts/libgeos_c.so \
  build/deploy/

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 31 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Do we need to create a repo in the dockerhub page before pushing?
I tried following teamcity-publish-release.sh and changed the docker_tag to the one with dockerhub_repository.
Just left docker push commented out because I wanted to make sure everything is ok before pushing.

Yeah, let's create a new repository for this one - it may become very spammy :)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 4 at r2 (raw file):

set -euo pipefail

find . -ls

This can be removed after we are done :)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 10 at r2 (raw file):

# `+` in the tag name.
# https://github.com/cockroachdb/cockroach/blob/4c6864b44b9044874488cfedee3a31e6b23a6790/pkg/util/version/version.go#L75
build_name="$(echo "${NAME}" | grep -E -o '^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[-.0-9A-Za-z]+)?$')"

$NAME is something that we manually set as a part of the release process and it won't work for CI. I wonder if we should just use git rev-parse HEAD or git rev-parse --short HEAD as the tag instead.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 55 at r2 (raw file):

configure_docker_creds
docker_login_with_google

You can remove this one, this is for Google's GCR, not Dockerhub.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 56 at r2 (raw file):

configure_docker_creds
docker_login_with_google
docker_login

Oooh. Now that you added this here it made me think that maybe it's not a good idea to push to dockerhub as a part of CI. Anyone can open a PR and get their job use these credentials to not only push stuff, but also echo the credentials. I wish the artifacts were super slim :)
🤔
@jlinder what do you think?

[ZhouXing19]

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 15 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

Sorry, should have been more specific. You needed to add those files as upstream artifacts explicitly in the build configuration. I added them and now the rules look like this:

bazel-bin/pkg/cmd/cockroach/cockroach_/cockroach=>upstream_artifacts
bazel-bin/c-deps/libgeos/lib/libgeos.so=>upstream_artifacts
bazel-bin/c-deps/libgeos/lib/libgeos_c.so=>upstream_artifacts

Now you can copy everything as:

cp \
  upstream_artifacts/cockroach \
  upstream_artifacts/libgeos.so \
  upstream_artifacts/libgeos_c.so \
  build/deploy/

Thanks! Made the changes.

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder, @rail, and @ZhouXing19)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 25 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

The tag usually uses the "$owner/$image_name" format. Maybe use something like "cockroachdb/cockroach-demo"?

Done.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 55 at r2 (raw file):

Previously, rail (Rail Aliiev) wrote…

You can remove this one, this is for Google's GCR, not Dockerhub.

Done.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 56 at r2 (raw file):

Previously, rail (Rail Aliiev) wrote…

Oooh. Now that you added this here it made me think that maybe it's not a good idea to push to dockerhub as a part of CI. Anyone can open a PR and get their job use these credentials to not only push stuff, but also echo the credentials. I wish the artifacts were super slim :)
🤔
@jlinder what do you think?

For now I've removed the push to dockerhub :) It should be easy if later we decide to add the push.

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder, @rail, and @ZhouXing19)

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 14 at r1 (raw file):

Previously, rail (Rail Aliiev) wrote…

I just added 2 more, can you also add them :)

Done, thank you!!

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 4 at r2 (raw file):

Previously, rail (Rail Aliiev) wrote…

This can be removed after we are done :)

Done.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 10 at r2 (raw file):

Previously, rail (Rail Aliiev) wrote…

$NAME is something that we manually set as a part of the release process and it won't work for CI. I wonder if we should just use git rev-parse HEAD or git rev-parse --short HEAD as the tag instead.

Removed the part defining $NAME for now since we're not sure if this image should be pushed or not.

[ZhouXing19]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jlinder, @rail, and @ZhouXing19)

---

build/deploy/Dockerfile, line 21 at r8 (raw file):

Previously, jlinder (James H. Linder) wrote…

Since this is Dockerfile for the production / release docker image, it should not include testing / development tools like inotify-toolsand all the tools needed to support building it. Images like this should contain the bare minimum of what is needed to run cockroach in production. I followed up on a related comment from Rail on an alternative approach in #72586 (review).

Yep got it removed. Using a go project with fsnotify in #72586 now.

---

build/teamcity/cockroach/ci/builds/build-docker-image.sh, line 31 at r1 (raw file):

Previously, jlinder (James H. Linder) wrote…

Noted below, but we shouldn't push this to docker hub.

If disk space is a worry, we can configure the artifact cleanup job to remove this build configuration's artifacts after two days.

Alternatively, configure build configs like this one (i.e. build configs that produce only a few huge artifacts) to save their artifacts in a Google Cloud Storage bucket instead of on the local TeamCity server disk. (Build configs like this one won't have the problem that made us turn GCS-backed storage off in the past. The problem was that the GCS integration uploaded all files as binary to GCS which made it so any downloaded text artifact files couldn't be opened by the browser, which made error investigations much more difficult.)

Thanks James for the detailed explanation!

---

build/teamcity/cockroach/ci/builds/build_docker_image.sh, line 33 at r8 (raw file):

Previously, jlinder (James H. Linder) wrote…

Could we name the tag like this?

build_name="$(git describe --tags --dirty --match=v[0-9]* 2> /dev/null || git rev-parse --short HEAD)"
docker_tag="cockroachdb/cockroach-ci:${build_name}"

I'm making this suggestion for these reasons:

• Adding information about which commit the cockroach build comes from makes it so the image can be easily understood where it came from.
• cockroach-demo could be confused with cockroach demo. ci does more to indicate the image was created as part of the CI process.

If including the $build_name in the tag makes it difficult to use the image in subsequent build configurations (by making the name not easily guessable), then it seems fine to leave that off the tag since the version can still be determined by running cockroach version.

yeah adding the $build_name can be a bit tricky for the test. I delete for now, but will try to make this improvement later (on a flex Friday?).

[jlinder]

Left one idea for a future PR.

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jlinder, @rail, and @ZhouXing19)

---

build/deploy/Dockerfile, line 27 at r9 (raw file):

ENV PATH=/cockroach:$PATH

ENV COCKROACH_CHANNEL=official-docker

It would be great if we could make only the images built via the final release process have the env var COCKROACH_CHANNEL=official-docker. That doesn't need to block this PR being merged.

[ZhouXing19]

TFTR!!
bors r=jlinder

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[jlinder]

Oh! I was surprised to see this was merged with many new changes and no re-review after the initial LGTM.

When I make substantial changes to a PR after getting a 👍, I ask the reviewer(s) to re-review before merging because of the new, non-reviewed code. The only time I merge with changes after an approval is when the changes fix a nit the reviewer left and approved of the PR merging with or without the fix or if a rebase to latest master is needed (not normally needed in this repo).

I left some notes below on the new code. Two ways you might act on them: incorporate the changes in #72586 or get #72586 merged and follow it up with a new PR with the changes.

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

go.mod, line 182 at r10 (raw file):

)

require github.com/fsnotify/fsnotify v1.5.1

I'm curious, why not place this in the above require() since that block looks to be requiring (mostly) directly used libs?

---

build/teamcity/cockroach/ci/builds/build_docker_image.sh, line 45 at r10 (raw file):

cp upstream_artifacts/cockroach "${artifacts}"/cockroach

docker_fsnotify_dir="$(dirname "${0}")/docker-fsnotify"

Building the docker-fsnotify binary should happen as a dependency of //pkg/testutils/docker:docker_test so that when bazel run //pkg/testutils/docker:docker_test is executed in the build/teamcity/cockroach/ci/tests/docker_image.sh script, bazel will make sure the binary is built and available for use (in the bazel sandbox) and the test code could use it from there (or move it somewhere else to mount it into the container).

The things to do to make this happen are:

• Move the docker_fsnotify code back under pkg/testutils/docker/docker-fsnotify/, including the BUILD.bazel file
• In the deps field for //pkg/testutils/docker/:docker_test, add this line:
  • //pkg/testutils/docker/docker-fsnotify:docker-fsnotify
• Change the test code to find and use the docker-fsnotify binary from the bazel sandbox