Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ci image add mysql 5.7 #502

Merged

Conversation

purelind
Copy link
Contributor

@purelind purelind commented Dec 5, 2024

Add more tools to ci jenkins

  • mysql 5.7
  • percona-toolkit

Fix rust install package error

  • cargo-platform v0.1.9 need rustc 1.78 or newer

@ti-chi-bot ti-chi-bot bot requested a review from wuhuizuo December 5, 2024 10:42
@ti-chi-bot ti-chi-bot bot added the size/M label Dec 5, 2024
Copy link

ti-chi-bot bot commented Dec 5, 2024

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

Based on the pull request title and description, the changes involve adding MySQL 5.7 and percona-toolkit to the CI Jenkins image. Looking at the diff, the changes seem straightforward and follow best practices for Dockerfile creation.

However, there are a few potential issues that can be addressed in this pull request:

  1. The use of yum instead of dnf: The Dockerfile currently uses dnf for package installation, but the new MySQL installation steps use yum. It's generally better to use a consistent package manager throughout the Dockerfile, so it's recommended to switch the MySQL installation to use dnf instead.

  2. Use of gpgcheck=0: The MySQL installation steps disable GPG checking using gpgcheck=0. This can be a security risk since it allows for the installation of unsigned packages, so it's recommended to keep GPG checking enabled and ensure that the MySQL RPM is signed by a trusted source.

  3. Lack of version pinning: The MySQL installation steps install the latest version of MySQL 5.7, which can potentially introduce compatibility issues with other parts of the CI pipeline. It's recommended to pin the version to a specific release to ensure consistency.

To address these issues, here are some suggested changes:

diff --git a/dockerfiles/ci/base/Dockerfile b/dockerfiles/ci/base/Dockerfile
index 663a1193..773948e2 100644
--- a/dockerfiles/ci/base/Dockerfile
+++ b/dockerfiles/ci/base/Dockerfile
@@ -17,18 +17,21 @@ RUN --mount=type=cache,target=/var/cache/dnf \
     pip install s3cmd==2.3.0 requests==2.26.0 certifi==2021.10.8 && \
     pip3 install requests==2.27.1
 
-ENV MARIADB_VERSION=5.5.68
-ENV PATH=$PATH:/usr/local/mariadb/bin
-# linux amd64 only
-RUN if [ "$(arch)" = "x86_64" ]; then \
-        echo "Detected amd64 architecture, proceeding with installation" && \
-        wget https://archive.mariadb.org/mariadb-${MARIADB_VERSION}/bintar-linux-systemd-x86_64/mariadb-${MARIADB_VERSION}-linux-systemd-x86_64.tar.gz && \
-        tar -xvf mariadb-${MARIADB_VERSION}-linux-systemd-x86_64.tar.gz && \
-        mv mariadb-${MARIADB_VERSION}-linux-systemd-x86_64 /usr/local/mariadb && \
-        rm mariadb-${MARIADB_VERSION}-linux-systemd-x86_64.tar.gz; \
-    else \
-        echo "Architecture not supported: $(arch)"; \
-    fi
+# Install gbk support
+RUN --mount=type=cache,target=/var/cache/dnf \
+    dnf install -y glibc-locale-source && \
+    localedef -f GBK -i zh_CN zh_CN.gbk
 
+# Install MySQL 5.7
+RUN dnf module disable mysql -y && \
+    echo '[mysql57-community]' > /etc/yum.repos.d/mysql57.repo && \
+    echo 'name=MySQL 5.7 Community Server' >> /etc/yum.repos.d/mysql57.repo && \
+    echo 'baseurl=http://repo.mysql.com/yum/mysql-5.7-community/el/7/$basearch/' >> /etc/yum.repos.d/mysql57.repo && \
+    echo 'enabled=1' >> /etc/yum.repos.d/mysql57.repo && \
+    rpm --import https://repo.mysql.com/RPM-GPG-KEY-mysql && \
+    dnf install -y mysql-community-server-5.7.35 && \
+    dnf clean all && \
+    rm -rf /var/cache/dnf
 
 # install golang toolchain
 # renovate: datasource=docker depName=golang
@@ -109,3 +112,12 @@ RUN if [ "$(arch)" = "x86_64" ]; then \
         echo "Architecture not supported: $(arch)"; \
     fi
 ENV PATH=$PATH:/usr/local/pulsar/bin
+
+# install percona-toolkit
+# linux amd64 only
+RUN if [ "$(arch)" = "x86_64" ]; then \
+        echo "Detected amd64 architecture, proceeding with installation" && \
+        dnf install -y https://repo.percona.com/testing/centos/7/RPMS/x86_64/percona-toolkit-3.0.12-1.el7.x86_64.rpm; \
+    else \
+        echo "Architecture not supported: $(arch)"; \
+    fi

The changes include:

  1. Removing the yum installation steps for MySQL and replacing them with dnf commands.
  2. Adding GPG key import and enabling GPG checking for the MySQL installation steps.
  3. Pinning the MySQL version to 5.7.35 to ensure consistency.

Overall, the changes seem appropriate and the suggested fixes should address the potential issues.

Copy link

ti-chi-bot bot commented Dec 6, 2024

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

Based on the pull request title and description, the key changes are adding MySQL 5.7 and Percona Toolkit to the CI Jenkins image. The pull request also includes some changes to the Dockerfile to accommodate the new tools.

One potential problem that I see is that the Dockerfile has some duplicated code, specifically the code that checks for the architecture and proceeds with the installation based on that. It might be better to extract that into a separate script that can be reused across multiple Dockerfiles.

Another potential problem is that the code for installing MySQL 5.7 is not very explicit. It disables the existing mysql module and installs a new repository for mysql57-community. This could potentially cause issues if there are other dependencies or packages that rely on the existing mysql module.

To address these issues, I would suggest refactoring the architecture detection code into a separate script that can be reused across Dockerfiles. I would also suggest being more explicit in the code for installing MySQL 5.7 and considering alternative installation methods that might be more stable.

Overall, the pull request looks good and the changes seem reasonable.

Copy link

ti-chi-bot bot commented Dec 6, 2024

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

From the pull request, the key changes made are adding MySQL 5.7 and percona-toolkit to the ci Jenkins Docker image. The pull request seems to be straightforward and does not introduce any potential problems.

However, there are a few suggestions I would like to make:

  • Instead of disabling the MySQL module and using the repository configuration method, it is better to use the mysql57-community-release-el7.rpm package to install the MySQL 5.7 community server.
  • Installing the glibc-locale-source package and running localedef command to add GBK support is not recommended. Instead, the glibc-common package can be installed to provide GBK support.

Here's the modified diff for the pull request:

diff --git a/dockerfiles/ci/base/Dockerfile b/dockerfiles/ci/base/Dockerfile
index 663a1193..b91a9f4e 100644
--- a/dockerfiles/ci/base/Dockerfile
+++ b/dockerfiles/ci/base/Dockerfile
@@ -17,18 +17,28 @@ RUN --mount=type=cache,target=/var/cache/dnf \
     pip install s3cmd==2.3.0 requests==2.26.0 certifi==2021.10.8 && \
     pip3 install requests==2.27.1
 
-ENV MARIADB_VERSION=5.5.68
-ENV PATH=$PATH:/usr/local/mariadb/bin
+# Install MySQL 5.7
+# linux amd64 only
+RUN if [ "$(arch)" = "x86_64" ]; then \
+        echo "Detected amd64 architecture, proceeding with installation" && \
+        dnf install -y https://dev.mysql.com/get/mysql57-community-release-el7-11.noarch.rpm && \
+        dnf install -y mysql-community-server && \
+        dnf clean all && \
+        rm -rf /var/cache/dnf; \
+    else \
+        echo "Architecture not supported: $(arch)"; \
+    fi
 
 # Install golang toolchain
 # renovate: datasource=docker depName=golang
 ARG GOLANG_VERSION=1.23.3
 RUN curl -Ls https://golang.org/dl/go${GOLANG_VERSION}.linux-amd64.tar.gz | tar -C /usr/local -xz
 
+# Install GBK support
+RUN dnf install -y glibc-common && \
+    localedef -f GBK -i zh_CN zh_CN.gbk
+
 # install pulsar
 # linux amd64 only
 RUN if [ "$(arch)" = "x86_64" ]; then \
         echo "Detected amd64 architecture, proceeding with installation" && \
         curl -Ls https://archive.apache.org/dist/pulsar/pulsar-2.9.2/apache-pulsar-2.9.2-bin.tar.gz | tar -xz && \
         mv apache-pulsar-2.9.2 /usr/local/pulsar; \
     else \
         echo "Architecture not supported: $(arch)"; \
     fi
 ENV PATH=$PATH:/usr/local/pulsar/bin
+
 # Install percona-toolkit
 # linux amd64 only
 RUN if [ "$(arch)" = "x86_64" ]; then \

Copy link

ti-chi-bot bot commented Dec 6, 2024

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

Key Changes:

  • MySQL 5.7 has been added to the ci jenkins Docker image.
  • Percona-toolkit has also been added to the image.
  • GBK support has been added to the image.
  • The Rustup profile has been set to minimal and stable has been updated.

Potential Problems:

  • It is unclear if the MySQL 5.7 version being installed is compatible with the rest of the CI system.
  • There might be compatibility issues with the new MySQL 5.7 version and the rest of the toolchain.
  • The dnf module disable mysql -y command might have unwanted effects on the system.
  • The gpgcheck=0 line in the mysql57.repo file is a potential security issue.
  • The GBK locale might not be necessary and might cause issues if not used properly.

Fixing suggestions:

  • Check if the MySQL 5.7 version being installed is compatible with other tools in the CI system.
  • Test the compatibility of the new MySQL 5.7 version with the rest of the toolchain.
  • Avoid using dnf module disable mysql -y. Instead, use dnf module reset mysql -y to revert to the default version.
  • Use a GPG key to verify the repository and remove the gpgcheck=0 line.
  • Consider removing the GBK locale if it is not necessary. If needed, ensure it is properly configured and tested.

@ti-chi-bot ti-chi-bot bot added the lgtm label Dec 9, 2024
Copy link

ti-chi-bot bot commented Dec 9, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-12-09 09:10:11.1877059 +0000 UTC m=+256801.276508442: ☑️ agreed by wuhuizuo.

Copy link

ti-chi-bot bot commented Dec 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 ti-chi-bot bot added the approved label Dec 9, 2024
@ti-chi-bot ti-chi-bot bot merged commit 25862e2 into PingCAP-QE:main Dec 9, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants