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

bump go to 1.22.3, fix make compose and optimize ut script for 1.2-dev #16644

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

guguducken
Copy link
Contributor

@guguducken guguducken commented Jun 4, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue # https://github.com/matrixorigin/MO-Cloud/issues/3413 https://github.com/matrixorigin/MO-Cloud/issues/3397

What this PR does / why we need it:

  1. bump go version from 1.21.5 to 1.22.3
  2. fix make compose to make it work
  3. make ut will read UT_WORKDIR env variable to store report, it will be $HOME if UT_WORKDIR is empty

PR Type

Enhancement, Bug fix


Description

  • Bumped Go version from 1.21.5 to 1.22.3.
  • Fixed and optimized Docker Compose BVT script.
  • Improved MySQL readiness check and updated paths in entrypoint script.
  • Added support for UT_WORKDIR environment variable.
  • Updated .dockerignore to exclude scratch directories.
  • Changed default COMPOSE_LAUNCH profile and added additional cleanup commands in Makefile.
  • Removed version declaration from Docker Compose file.
  • Updated base images in Dockerfiles to use Go 1.22.3.

Changes walkthrough 📝

Relevant files
Bug fix
1 files
compose_bvt.sh
Fix and optimize Docker Compose BVT script.                           

optools/compose_bvt/compose_bvt.sh

  • Removed .bak file deletion.
  • Updated file extensions for stack dumps.
  • Fixed Docker image name and volume removal command.
  • Removed redundant exit $? statement.
  • +5/-8     
    Enhancement
    3 files
    entrypoint.sh
    Improve MySQL readiness check and update paths.                   

    optools/compose_bvt/entrypoint.sh

  • Improved MySQL readiness check.
  • Updated paths for test resources.
  • +5/-3     
    utilities.sh
    Add support for `UT_WORKDIR` environment variable.             

    optools/utilities.sh

    • Added support for UT_WORKDIR environment variable.
    +1/-1     
    Makefile
    Update Docker Compose launch profile and cleanup commands.

    Makefile

  • Changed default COMPOSE_LAUNCH profile.
  • Added force removal of Docker volumes and additional cleanup commands.

  • +5/-2     
    Configuration changes
    2 files
    .dockerignore
    Update `.dockerignore` to exclude scratch directories.     

    .dockerignore

    • Added **/scratch/ to ignore list.
    +1/-0     
    compose.yaml
    Remove version declaration from Docker Compose file.         

    etc/launch-tae-compose/compose.yaml

    • Removed version declaration.
    +0/-2     
    Dependencies
    4 files
    go.mod
    Bump Go version to 1.22.3.                                                             

    go.mod

    • Bumped Go version from 1.21.5 to 1.22.3.
    +1/-1     
    Dockerfile
    Update base image to Go 1.22.3.                                                   

    optools/bvt_ut/Dockerfile

    • Updated base image to use Go 1.22.3.
    +1/-1     
    Dockerfile.tester
    Update base image and remove redundant installation.         

    optools/compose_bvt/Dockerfile.tester

  • Updated base image to use Go 1.22.3.
  • Removed redundant apt installation.
  • +2/-3     
    Dockerfile
    Update base image to Go 1.22.3.                                                   

    optools/images/Dockerfile

    • Updated base image to use Go 1.22.3.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    1. bump go version from 1.21.5 to 1.22.3
    2. fix `make compose` to make it work
    3. `make ut` will read `UT_WORKDIR` env variable to store report, it will be `$HOME` if `UT_WORKDIR` is empty
    
    Approved by: @zhangxu19830126, @sukki37
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR includes multiple file changes across scripts, Dockerfiles, and configuration files. The changes involve version upgrades, script optimizations, and environmental adjustments which require a careful review to ensure compatibility and correctness.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The removal of the .bak file deletion command in compose_bvt.sh might lead to unintended consequences if these backup files are expected to be cleaned up by other processes or scripts.

    Performance Concern: The change in the entrypoint.sh script to continuously check database availability without a timeout could potentially lead to infinite loops if the database does not become available.

    🔒 Security concerns

    No

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for curl commands to ensure failures are caught and handled

    Add error handling for the curl commands to ensure that the script can handle failures
    gracefully. This will help in debugging and ensure the script doesn't silently fail.

    optools/compose_bvt/compose_bvt.sh [10-11]

    -curl http://localhost:12345/debug/pprof/goroutine\?debug=2 -o docker-compose-log/cn-0-dump-stacks
    -curl http://localhost:22345/debug/pprof/goroutine\?debug=2 -o docker-compose-log/cn-1-dump-stacks
    +curl http://localhost:12345/debug/pprof/goroutine\?debug=2 -o docker-compose-log/cn-0-dump-stacks || { echo "Failed to fetch goroutine dump from cn-0"; exit 1; }
    +curl http://localhost:22345/debug/pprof/goroutine\?debug=2 -o docker-compose-log/cn-1-dump-stacks || { echo "Failed to fetch goroutine dump from cn-1"; exit 1; }
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for curl commands is crucial for robustness and debugging. The suggestion correctly identifies a potential failure point and offers a practical improvement.

    8
    Add a timeout mechanism to the while true loop to prevent indefinite execution

    Add a timeout mechanism to the while true loop to ensure the script doesn't run
    indefinitely if the condition is never met.

    optools/compose_bvt/entrypoint.sh [19-25]

    +TIMEOUT=300
     while true; do
       if mysql -h cn0 -P 6001 -u dump -p111 --execute "show databases;"; then
         break;
       fi
    -  if [ $i -ge 300 ]; then
    +  if [ $i -ge $TIMEOUT ]; then
         echo "wait for $i seconds, mo init not finish, so exit 1"
         docker ps
    +    exit 1
    +  fi
    +  sleep 1
    +  i=$(($i+1))
    +done
     
    Suggestion importance[1-10]: 7

    Why: Implementing a timeout mechanism is a good practice to avoid potential infinite loops. This suggestion enhances the script's reliability and prevents hanging processes.

    7
    Re-add the apt update and apt install -y mariadb-client commands to ensure necessary dependencies are installed

    Re-add the apt update and apt install -y mariadb-client commands to ensure that the
    necessary dependencies are installed in the Docker image.

    optools/compose_bvt/Dockerfile.tester [1-8]

     FROM matrixorigin/tester:go1.22.3-jdk8
     
     WORKDIR /
     
    -RUN git clone https://github.com/matrixorigin/mo-tester.git
    +RUN git clone https://github.com/matrixorigin/mo-tester.git && apt update && apt install -y mariadb-client
     COPY . /matrixone
     
     RUN cd mo-tester && sed -i 's/127.0.0.1/cn0/g' mo.yml
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to re-add package installation commands is relevant for ensuring all dependencies are available. However, it's a moderate improvement as it mainly addresses the setup completeness rather than a critical functionality or security issue.

    6
    Performance
    Add a sleep interval inside the while true loop to avoid excessive CPU usage

    Add a sleep interval inside the while true loop to avoid a tight loop that can consume
    excessive CPU resources.

    optools/compose_bvt/entrypoint.sh [19-25]

     while true; do
       if mysql -h cn0 -P 6001 -u dump -p111 --execute "show databases;"; then
         break;
       fi
       if [ $i -ge 300 ]; then
         echo "wait for $i seconds, mo init not finish, so exit 1"
         docker ps
    +    exit 1
    +  fi
    +  sleep 1
    +  i=$(($i+1))
    +done
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to add a sleep interval in the loop is valid to prevent high CPU usage. It addresses a performance issue effectively.

    7

    @sukki37 sukki37 merged commit 001fa02 into matrixorigin:1.2-dev Jun 5, 2024
    16 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    @guguducken guguducken deleted the upgrade-go-version branch June 6, 2024 06:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Enhancement Review effort [1-5]: 3 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants