PingCAP-QE / ci

Continue intergration tests
Apache License 2.0
19 stars 97 forks source link

chore(tiflash): update dind image version #2933

Open purelind opened 2 months ago

purelind commented 2 months ago

Update dind image version. Avoid encountering this issue in the future https://github.com/docker-library/golang/issues/467

ti-chi-bot[bot] commented 2 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from purelind, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[pipelines/OWNERS](https://github.com/PingCAP-QE/ci/blob/main/pipelines/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ti-chi-bot[bot] commented 2 months ago

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary:

This is a simple PR that updates the Docker-in-Docker (dind) image version in several YAML files. The new image version is "hub.pingcap.net/jenkins/docker:20.10.14-dind". Additionally, two environment variables have been added to the container definition, and a readiness probe has been defined to check the Docker daemon status.

Potential problems:

There are no major problems with this PR. However, there are some suggestions:

  1. The PR description could be more descriptive and informative. It would be helpful to explain why the image version is being updated and what benefits it brings.
  2. It is not clear why the two environment variables are necessary. A comment explaining their purpose would be helpful.
  3. The readiness probe checks the Docker daemon status, but it does not check if the Docker daemon is fully functional. A check that tests a basic Docker command such as "docker ps" would be more comprehensive.

Fixing suggestions:

  1. The PR description could be improved by stating the reason for the update, such as "Updating dind image version to improve security and performance".
  2. A comment explaining the purpose of the environment variables should be added. For example, "The DOCKER_TLS_CERTDIR variable disables TLS certificate verification, which is necessary for some private registries. The DOCKER_HOST variable specifies the Docker daemon address."
  3. A more comprehensive readiness probe could be added. For example, "readinessProbe: exec: command: - sh - -c - 'docker ps > /dev/null 2>&1 && docker version > /dev/null 2>&1'"
purelind commented 2 months ago

/hold

hub.pingcap.net/tiflash/tiflash-ci-base always restarting with those error

/usr/local/bin/../include/c++/v1/vector:1571: _LIBCPP_ASSERT '__n < size()' failed. vector[] index out of bounds
/usr/local/bin/../include/c++/v1/vector:1571: _LIBCPP_ASSERT '__n < size()' failed. vector[] index out of bounds
/usr/local/bin/../include/c++/v1/vector:1571: _LIBCPP_ASSERT '__n < size()' failed. vector[] index out of bounds
libc++abi: terminate_handler unexpectedly threw an exception
libc++abi: terminate_handler unexpectedly threw an exception
libc++abi: terminate_handler unexpectedly threw an exception
libc++abi: terminate_handler unexpectedly threw an exception
libc++abi: terminate_handler unexpectedly threw an exception
wuhuizuo commented 2 months ago

ref: #2820