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

[build][test]: Release docker images for aarch64 platform #2266

Merged
merged 1 commit into from
May 21, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented May 21, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Part of #1847

Now, the fork seleniumhq-community/docker-seleniarm was merged.
From image tag based 4.21.0 onwards, the architectures supported by this project as below.

Architecture Available
x86_64 (aka amd64)
aarch64 (aka arm64/armv8)
armhf (aka arm32/armv7l)

Build the multi-arch images locally

Recommend to enable the experimental feature containerd image store in Docker Engine.
containerd understands multiplatform images, where a single image tag can refer to different variants covering a range of OS and hardware architectures.
It simplifies the process of building, storing, and distributing images across different platforms.

A single command in project to enable that feature

make set_containerd_image_store

To build all the images for multiplatform, run the following command:

PLATFORMS=linux/amd64,linux/arm64 make build

To build the images for a specific platform, run the following command:

PLATFORMS=linux/arm64 make build

By default, without specifying the PLATFORMS variable, the images are built for the linux/amd64 platform.

Browser images in multi-arch

  • Google does not build Chrome (google-chrome) for Linux/ARM platforms. Hence, the Chrome (node and standalone) images are only available for AMD64.
    Similarly, Microsoft does not build Edge (microsoft-edge) for Linux/ARM platforms.

  • Instead, the open source Chromium browser is used, which is built for Linux/ARM. The Chromium (node and standalone) images are available in multi-arch.

$ docker run --rm -it -p 4444:4444 -p 5900:5900 -p 7900:7900 --shm-size 2g selenium/standalone-chromium:latest
  • Mozilla Firefox now is available for Linux/ARM64 via Nightly channel.
    The Firefox version in ARM64 will be different with the AMD64 until the stable release is available. The Firefox (node and standalone) images are available in multi-arch.

Multi-arch images are tested on CircleCI with resource class Linux/ARM64. See the status below.

CircleCI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests


Description

  • Unified Docker and dependencies setup for AMD64 and ARM64 in chart_setup_env.sh.
  • Enhanced release notes generation with ARM64 support in generate_release_notes.sh.
  • Added tagging and pushing logic for Chromium images in tag_and_push_browser_images.sh.
  • Updated scripts to use Python 3 in various shell scripts.
  • Added a Chrome cleanup script in NodeChromium/chrome-cleanup.sh.
  • Enhanced chart tests for multi-platform support in chart_test.sh.
  • Enhanced Selenium tests for multi-platform support in tests/SeleniumTests/__init__.py.
  • Enhanced test script with platform detection and Docker network improvements in tests/test.py.
  • Enhanced Makefile for multi-platform builds and Chromium support.
  • Enhanced CircleCI configuration for ARM64 and multi-platform support.

Changes walkthrough 📝

Relevant files
Enhancement
11 files
chart_setup_env.sh
Unified Docker and dependencies setup for AMD64 and ARM64.

tests/charts/make/chart_setup_env.sh

  • Unified Docker installation for AMD64 and ARM64.
  • Added installation steps for Docker Compose, kind, and Minikube for
    ARM64.
  • Updated dependencies and configurations for ARM64.
  • +129/-118
    generate_release_notes.sh
    Enhanced release notes generation with ARM64 support.       

    generate_release_notes.sh

  • Added support for ARM64 in release notes generation.
  • Included additional components and versions in the release notes.
  • +19/-10 
    tag_and_push_browser_images.sh
    Added tagging and pushing for Chromium images.                     

    tag_and_push_browser_images.sh

  • Added tagging and pushing logic for Chromium images.
  • Included versioning and tagging for Chromium and ChromeDriver.
  • +46/-0   
    bootstrap.sh
    Updated scripts to use Python 3.                                                 

    tests/charts/bootstrap.sh

    • Switched from python to python3 for script execution.
    +3/-3     
    chrome-cleanup.sh
    Added Chrome cleanup script.                                                         

    NodeChromium/chrome-cleanup.sh

  • Added a script to clean up stuck Chrome processes and temporary files.

  • +37/-0   
    bootstrap.sh
    Updated scripts to use Python 3.                                                 

    tests/build-backward-compatible/bootstrap.sh

    • Switched from python to python3 for script execution.
    +2/-2     
    bootstrap.sh
    Updated scripts to use Python 3.                                                 

    tests/bootstrap.sh

    • Switched from python to python3 for script execution.
    +2/-2     
    chart_build.sh
    Enhanced chart build script for CI environments.                 

    tests/charts/make/chart_build.sh

  • Added installation of yamale and yamllint for CI environments.
  • Switched from python to python3 for script execution.
  • +4/-1     
    chart_cluster_setup.sh
    Added Helm repository setup for KEDA core.                             

    tests/charts/make/chart_cluster_setup.sh

    • Added Helm repository setup for KEDA core installation.
    +1/-0     
    test.py
    Enhanced test script with platform detection and Docker network
    improvements.

    tests/test.py

  • Added platform detection and handling for Docker builds.
  • Enhanced network creation with duplicate check.
  • +31/-11 
    Makefile
    Enhanced Makefile for multi-platform builds and Chromium support.

    Makefile

  • Added support for building and tagging Chromium images.
  • Enhanced multi-platform build and test logic.
  • Added new targets for setting up containerd image store and multi-arch
    builds.
  • +235/-204
    Tests
    3 files
    chart_test.sh
    Enhanced chart tests for multi-platform support.                 

    tests/charts/make/chart_test.sh

  • Added conditional logic to disable EdgeNode for non-amd64 platforms.
  • Updated test configurations for different platforms.
  • +14/-2   
    __init__.py
    Enhanced Selenium tests for multi-platform support.           

    tests/SeleniumTests/init.py

  • Added support for different platforms in test configurations.
  • Increased timeouts for Android platform tests.
  • +21/-9   
    config.yml
    Enhanced CircleCI configuration for ARM64 and multi-platform support.

    .circleci/config.yml

  • Added new workflows for ARM64 platform builds and tests.
  • Updated job definitions for multi-platform support.
  • Simplified and consolidated CI configuration.
  • +161/-348

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

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    5, due to the extensive changes across multiple files and scripts, including modifications to Dockerfiles, shell scripts, and CI configuration files. The PR introduces support for ARM64 architecture, which involves complex changes in the build and deployment processes. Reviewing these changes requires a deep understanding of multi-architecture builds, Kubernetes configurations, and CI/CD workflows.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Compatibility Issues: The changes introduce ARM64 support, which might not be fully compatible with all dependencies and tools used in the project. Thorough testing on ARM64 environments is required to ensure compatibility.

    CI Configuration Complexity: The modifications to CI workflows are significant and might introduce errors in the build and test processes. The complexity of multi-architecture support could lead to maintenance challenges.

    Script Modifications: The changes in shell scripts involve critical deployment and build steps. Any errors here could lead to failed deployments or builds. Reviewing these for syntax and logical errors is crucial.

    🔒 Security concerns

    No specific security concerns identified in the PR changes. However, ensuring that no sensitive data is exposed in logs or error messages during the build and deployment processes is recommended.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Add the current user to the docker group instead of setting permissive permissions on the Docker socket

    Instead of using sudo chmod 666 /var/run/docker.sock, consider adding the current user to
    the docker group to avoid potential security risks associated with setting such permissive
    permissions.

    tests/charts/make/chart_setup_env.sh [28]

    -sudo chmod 666 /var/run/docker.sock
    +sudo usermod -aG docker $USER
    +newgrp docker
     
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly identifies a significant security risk with setting the Docker socket permissions to 666 and offers a safer alternative by adding the user to the Docker group.

    9
    Possible issue
    Ensure that required environment variables are defined before using them

    To avoid potential issues with uninitialized variables, ensure that NAMESPACE and
    TAG_VERSION are defined before using them in the script.

    tag_and_push_browser_images.sh [70-71]

    +if [ -z "${NAMESPACE}" ] || [ -z "${TAG_VERSION}" ]; then
    +  echo "NAMESPACE or TAG_VERSION is not set"
    +  exit 1
    +fi
     CHROMIUM_VERSION=$(docker run --rm ${NAMESPACE}/node-chromium:${TAG_VERSION} chromium --version | awk '{print $2}')
     echo "Chromium version -> "${CHROMIUM_VERSION}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a potential issue with uninitialized variables which could lead to runtime errors. It's a crucial fix for robustness.

    8
    Add a check for the skip-test input in the deploy job's if condition

    Consider adding a check for the skip-test input in the deploy job's if condition to ensure
    that tests are not skipped unintentionally during deployment.

    .github/workflows/deploy.yml [30]

    -if: contains(toJson(github.event.commits), '[deploy]') || (github.event_name == 'workflow_dispatch' && github.event.inputs.release == 'true' && !failure() && !cancelled())
    +if: contains(toJson(github.event.commits), '[deploy]') || (github.event_name == 'workflow_dispatch' && github.event.inputs.release == 'true' && github.event.inputs.skip-test == 'false' && !failure() && !cancelled())
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential issue where the deployment could proceed even if tests were intended to be skipped, which is crucial for maintaining deployment integrity.

    8
    Add a check to ensure Docker is installed before running Docker commands

    Add a check to ensure that the docker command is available before attempting to run Docker
    commands, to provide a clearer error message if Docker is not installed.

    generate_release_notes.sh [22]

    +if ! command -v docker &> /dev/null; then
    +    echo "Docker could not be found, please install Docker."
    +    exit 1
    +fi
     GECKODRIVER_VERSION=$(docker run --rm ${NAMESPACE}/node-firefox:${TAG_VERSION} geckodriver --version | awk 'NR==1{print $2}')
     
    Suggestion importance[1-10]: 7

    Why: Adding a check for Docker installation before executing Docker commands is a practical improvement that enhances the robustness of the script by preventing unclear errors when Docker is not installed.

    7
    Provide default values for environment variables to avoid potential issues

    To avoid potential issues with uninitialized environment variables, provide default values
    for BASE_VERSION and BASE_RELEASE.

    tests/test.py [25-26]

    -BASE_VERSION = os.environ.get('BASE_VERSION')
    -BASE_RELEASE = os.environ.get('BASE_RELEASE')
    +BASE_VERSION = os.environ.get('BASE_VERSION', 'default_version')
    +BASE_RELEASE = os.environ.get('BASE_RELEASE', 'default_release')
     
    Suggestion importance[1-10]: 7

    Why: Providing default values for environment variables is a good defensive programming practice, especially to ensure the script behaves predictably when environment variables are missing.

    7
    Readability
    Rephrase a sentence to improve readability

    The line "From image tag based 4.21.0 onwards, the architectures supported by this project
    as below." is missing a verb and could be clearer. Consider rephrasing it to improve
    readability.

    README.md [127]

    -From image tag based `4.21.0` onwards, the architectures supported by this project as below.
    +Starting from image tag `4.21.0`, the architectures supported by this project are listed below.
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a grammatical error and improves the clarity of the sentence, which is crucial for documentation readability.

    8
    Best practice
    Use mktemp to create a temporary directory for downloading and extracting files

    Use mktemp to create a temporary directory for downloading and extracting files to avoid
    potential conflicts and ensure a clean environment.

    tests/charts/make/chart_setup_env.sh [124-125]

    -mkdir -p helm
    -tar -xf helm.tar.gz --strip-components 1 -C helm
    +TEMP_DIR=$(mktemp -d)
    +tar -xf helm.tar.gz --strip-components 1 -C $TEMP_DIR
    +sudo cp -frp $TEMP_DIR/helm /usr/local/bin/helm
    +rm -rf $TEMP_DIR
     
    Suggestion importance[1-10]: 7

    Why: Using mktemp for creating a temporary directory is a best practice that avoids potential conflicts and ensures a clean environment. This is a valuable suggestion for improving the script's safety and reliability.

    7
    Use a case statement for platform-specific logic to improve readability

    To improve readability and maintainability, consider using a case statement instead of
    multiple if statements for platform-specific logic.

    tests/charts/make/chart_test.sh [168-173]

    -if [ "${PLATFORMS}" != "linux/amd64" ]; then
    -  HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} \
    -  --set edgeNode.enabled=false \
    -  --set chromeNode.imageName=node-chromium \
    -  "
    -fi
    +case "${PLATFORMS}" in
    +  "linux/amd64")
    +    ;;
    +  *)
    +    HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} \
    +    --set edgeNode.enabled=false \
    +    --set chromeNode.imageName=node-chromium \
    +    "
    +    ;;
    +esac
     
    Suggestion importance[1-10]: 5

    Why: Converting if statements to a case statement for handling platform-specific logic is a good practice for readability, but it's not a critical issue.

    5
    Rename the skip-test input to skip_tests for consistency with YAML naming conventions

    Consider renaming the skip-test input to skip_tests to maintain consistency with typical
    YAML naming conventions.

    .github/workflows/deploy.yml [11-15]

    -skip-test:
    +skip_tests:
       description: 'Skip the tests'
       required: false
       type: boolean
       default: false
     
    Suggestion importance[1-10]: 5

    Why: While the suggestion for renaming to maintain naming consistency is valid, it is a minor improvement and does not impact functionality.

    5
    Clarity
    Rename a step to provide a clearer description of its purpose

    The Set up containerd image store feature step should have a more descriptive name to
    clearly indicate its purpose.

    .github/workflows/helm-chart-test.yml [83]

    -- name: Set up containerd image store feature
    +- name: Enable containerd image store for multi-arch support
     
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances the descriptiveness of the step name, improving understanding of the workflow's purpose, which aids in maintainability and clarity.

    7
    Add a placeholder to indicate that the platform variable can be replaced

    The command to build images for a specific platform should include a placeholder for the
    platform variable to make it clear that it can be replaced with any desired platform.

    README.md [157]

    -PLATFORMS=linux/arm64 make build
    +PLATFORMS=<desired_platform> make build
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves clarity by indicating that the platform variable is customizable, which is helpful but not critical.

    6
    Maintainability
    Use a variable for the Docker GPG key URL to avoid repetition and simplify future updates

    Use a variable for the Docker GPG key URL to avoid repetition and make future updates
    easier.

    tests/charts/make/chart_setup_env.sh [20]

    -sudo curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc
    +DOCKER_GPG_URL="https://download.docker.com/linux/ubuntu/gpg"
    +sudo curl -fsSL $DOCKER_GPG_URL -o /etc/apt/keyrings/docker.asc
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to use a variable for the Docker GPG key URL is valid for maintainability and simplifies future updates. It's a good practice but not critical.

    6
    Enhancement
    Use pgrep to directly get process IDs of Chromium processes

    To avoid potential issues with ps command output, use pgrep to directly get the process
    IDs of Chromium processes.

    NodeChromium/chrome-cleanup.sh [8]

    -ps -e -o pid,etimes,command | grep -v grep | grep chromium/chromium | awk '{if($2>'${SE_BROWSER_LEFTOVERS_PROCESSES_SECS}') print $0}' | awk '{print $1}' | xargs -r kill -9
    +pgrep -f chromium/chromium | xargs -r kill -9
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves the reliability of the script by using pgrep instead of a complex pipeline, enhancing maintainability and reducing error risk.

    6

    @VietND96 VietND96 force-pushed the arm64 branch 2 times, most recently from eabf16c to 522714c Compare May 21, 2024 23:15
    @VietND96 VietND96 merged commit de9f2c5 into SeleniumHQ:trunk May 21, 2024
    1 of 15 checks passed
    @VietND96 VietND96 deleted the arm64 branch May 22, 2024 01:24
    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.

    1 participant