Skip to content

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Aug 5, 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

Share the preStop script in Helm chart to drain the Node before stopping the container can use when deployed via docker, docker compose.
Env var to enable SE_NODE_GRACEFUL_SHUTDOWN is true

Motivation and Context

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

  • Added graceful shutdown handling for Selenium Node in entry_point.sh.
  • Updated various scripts to use SE_OPT_BIN for configuration directory.
  • Enhanced node grid URL script with authentication and default settings.
  • Added TEST_PARALLEL_COUNT and updated test parallel autoscaling logic.
  • Updated Dockerfiles and configurations to include new scripts and environment variables.
  • Set TEST_DELAY_AFTER_TEST default value to 0 in test scripts.
  • Commented out tests service in docker-compose-v3-test-parallel.yml.

Changes walkthrough 📝

Relevant files
Enhancement
12 files
entry_point.sh
Add graceful shutdown handling in entry point script         

Base/entry_point.sh

  • Added graceful shutdown handling for Selenium Node.
  • Introduced NODE_CONFIG_DIRECTORY variable.
  • Added conditional execution of nodePreStop.sh.
  • +5/-0     
    distributorProbe.sh
    Update distributor probe script configuration directory   

    charts/selenium-grid/configs/distributor/distributorProbe.sh

    • Changed ROUTER_CONFIG_DIRECTORY default value to SE_OPT_BIN.
    +1/-1     
    nodeGridUrl.sh
    Enhance node grid URL script with auth and defaults           

    charts/selenium-grid/configs/node/nodeGridUrl.sh

  • Added support for basic authentication and sub-path handling.
  • Set default grid URL for standalone mode.
  • +8/-7     
    nodePreStop.sh
    Update node pre-stop script configuration                               

    charts/selenium-grid/configs/node/nodePreStop.sh

  • Added SE_NODE_PORT variable.
  • Changed NODE_CONFIG_DIRECTORY default value to SE_OPT_BIN.
  • +2/-1     
    nodeProbe.sh
    Update node probe script configuration                                     

    charts/selenium-grid/configs/node/nodeProbe.sh

  • Added SE_NODE_PORT variable.
  • Changed NODE_CONFIG_DIRECTORY default value to SE_OPT_BIN.
  • +2/-1     
    routerProbe.sh
    Update router probe script configuration directory             

    charts/selenium-grid/configs/router/routerProbe.sh

    • Changed ROUTER_CONFIG_DIRECTORY default value to SE_OPT_BIN.
    +1/-1     
    Dockerfile
    Update Dockerfile to include new scripts and environment variable

    Base/Dockerfile

  • Added nodeGridUrl.sh and nodePreStop.sh to /opt/bin/.
  • Set SE_OPT_BIN environment variable.
  • +3/-2     
    Makefile
    Update Makefile for nightly builds and test configurations

    Makefile

  • Added commands to handle nightly base build.
  • Updated test configurations for parallel testing.
  • +13/-8   
    selenium.conf
    Update Selenium Node configuration for group kill               

    NodeBase/selenium.conf

    • Added killasgroup=true to Selenium Node configuration.
    +1/-0     
    Dockerfile
    Update Chromium installation source                                           

    NodeChromium/Dockerfile

    • Updated Chromium installation source to Debian sid.
    +3/-3     
    supervisord.conf
    Update video recording configuration for group kill           

    Video/supervisord.conf

  • Added stopasgroup and killasgroup to video recording configuration.
  • +3/-3     
    values.yaml
    Update values.yaml for new script directory                           

    charts/selenium-grid/values.yaml

  • Changed extra scripts directory to /opt/bin.
  • Updated preStop command path.
  • +4/-4     
    Tests
    3 files
    chart_test.sh
    Update test delay configuration in chart test script         

    tests/charts/make/chart_test.sh

    • Set TEST_DELAY_AFTER_TEST default value to 0.
    +1/-1     
    __init__.py
    Add parallel count configuration for tests                             

    tests/SeleniumTests/init.py

  • Added TEST_PARALLEL_COUNT environment variable.
  • Updated test parallel autoscaling logic.
  • +3/-2     
    docker-compose-v3-test-parallel.yml
    Update docker-compose for graceful shutdown and session drain

    tests/docker-compose-v3-test-parallel.yml

  • Added graceful shutdown and session drain configurations for nodes.
  • Commented out tests service.
  • +23/-14 

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 5, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Script Execution
    The script nodePreStop.sh is executed without checking if it exists or is executable, which could lead to errors if the script is missing or not executable.

    Security Concern
    The use of basic authentication credentials in URLs can expose sensitive information in logs or other outputs. Consider using a more secure method of authentication.

    Test Configuration
    The introduction of TEST_PARALLEL_COUNT environment variable affects the parallel test execution but lacks validation for non-integer values, which could cause runtime errors.

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure the nodePreStop.sh script exists and is executable before running it

    Ensure that the script nodePreStop.sh exists and is executable before attempting to
    call it. This can prevent runtime errors if the script is missing or lacks execution
    permissions.

    Base/entry_point.sh [23]

    -bash ${NODE_CONFIG_DIRECTORY}/nodePreStop.sh
    +if [ -x "${NODE_CONFIG_DIRECTORY}/nodePreStop.sh" ]; then
    +    bash ${NODE_CONFIG_DIRECTORY}/nodePreStop.sh
    +else
    +    echo "Error: nodePreStop.sh does not exist or is not executable."
    +    exit 1
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion adds a crucial check to prevent runtime errors, ensuring that the script exists and is executable before attempting to run it. This is a significant improvement for robustness.

    9
    Check if routerGraphQLUrl.sh is executable and exists before setting GRID_GRAPHQL_URL

    Use a more robust method for setting the GRID_GRAPHQL_URL by checking if the script
    routerGraphQLUrl.sh is executable and exists.

    charts/selenium-grid/configs/router/routerProbe.sh [9]

    -GRID_GRAPHQL_URL=$(bash ${ROUTER_CONFIG_DIRECTORY}/routerGraphQLUrl.sh)
    +if [ -x "${ROUTER_CONFIG_DIRECTORY}/routerGraphQLUrl.sh" ]; then
    +    GRID_GRAPHQL_URL=$(bash ${ROUTER_CONFIG_DIRECTORY}/routerGraphQLUrl.sh)
    +else
    +    echo "Error: routerGraphQLUrl.sh does not exist or is not executable."
    +    GRID_GRAPHQL_URL=""
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion adds a necessary check to ensure that the script exists and is executable before using it, preventing potential runtime errors. This is a significant improvement for robustness.

    9
    Ensure BASIC_AUTH is not empty before using it in the URL construction

    Add a check to ensure that BASIC_AUTH is not empty before constructing the grid_url
    with it. This prevents constructing a URL with an unexpected format if
    authentication credentials are not provided.

    charts/selenium-grid/configs/node/nodeGridUrl.sh [15]

    -grid_url=${SE_SERVER_PROTOCOL}://${BASIC_AUTH}${SE_HUB_HOST:-$SE_ROUTER_HOST}:${SE_HUB_PORT:-$SE_ROUTER_PORT}${SE_SUB_PATH}
    +if [ -n "${BASIC_AUTH}" ]; then
    +    grid_url=${SE_SERVER_PROTOCOL}://${BASIC_AUTH}${SE_HUB_HOST:-$SE_ROUTER_HOST}:${SE_HUB_PORT:-$SE_ROUTER_PORT}${SE_SUB_PATH}
    +else
    +    grid_url=${SE_SERVER_PROTOCOL}://${SE_HUB_HOST:-$SE_ROUTER_HOST}:${SE_HUB_PORT:-$SE_ROUTER_PORT}${SE_SUB_PATH}
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures that the URL is constructed correctly by checking if BASIC_AUTH is not empty, preventing potential issues with malformed URLs. This is a valuable improvement for reliability.

    8
    Best practice
    Validate SE_NODE_PORT to ensure it is a valid port number

    Validate the SE_NODE_PORT environment variable to ensure it contains a valid port
    number. This prevents potential issues with network configurations.

    charts/selenium-grid/configs/node/nodePreStop.sh [4]

    -SE_NODE_PORT=${SE_NODE_PORT:-"5555"}
    +if [[ "${SE_NODE_PORT}" =~ ^[0-9]+$ ]] && [ "${SE_NODE_PORT}" -ge 1 ] && [ "${SE_NODE_PORT}" -le 65535 ]; then
    +    SE_NODE_PORT=${SE_NODE_PORT}
    +else
    +    echo "Invalid SE_NODE_PORT: ${SE_NODE_PORT}. Using default 5555."
    +    SE_NODE_PORT="5555"
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Validating the SE_NODE_PORT ensures that it is within the valid range for port numbers, which is a good practice to prevent network configuration errors. This is a useful enhancement for stability.

    7

    @VietND96 VietND96 merged commit 8bada80 into SeleniumHQ:trunk Aug 5, 2024
    17 checks passed
    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 9, 2024
    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