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

K8s: Default Node register period is longer with a short cycle #2662

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Feb 18, 2025

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

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, Documentation


Description

  • Introduced SE_NODE_REGISTER_PERIOD and SE_NODE_REGISTER_CYCLE for node registration control.

  • Updated Helm chart configurations and documentation for new environment variables.

  • Added default values for SE_EVENT_BUS_PUBLISH_PORT and SE_EVENT_BUS_SUBSCRIBE_PORT.

  • Enhanced environment variable handling in Dockerfile and configuration scripts.


Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Add node registration variables to pod template                   

charts/selenium-grid/templates/_helpers.tpl

  • Added nodeRegisterPeriod and nodeRegisterCycle variables to pod
    template.
  • Introduced new environment variables for node registration in
    Kubernetes.
  • +8/-6     
    Dockerfile
    Add new environment variables to Dockerfile                           

    NodeBase/Dockerfile

  • Added SE_NODE_REGISTER_PERIOD and SE_NODE_REGISTER_CYCLE with default
    values.
  • Set default values for event bus publish and subscribe ports.
  • +4/-0     
    generate_config
    Enhance configuration script for event bus                             

    NodeBase/generate_config

  • Conditional configuration for event bus based on SE_EVENT_BUS_HOST.
  • Improved configuration script for dynamic environment variables.
  • +6/-4     
    values.yaml
    Add node registration configurations to Helm values           

    charts/selenium-grid/values.yaml

  • Added global and node-specific configurations for nodeRegisterPeriod
    and nodeRegisterCycle.
  • Enhanced node configurations for custom capabilities.
  • +28/-0   
    Documentation
    ENV_VARIABLES.md
    Update documentation for new environment variables             

    ENV_VARIABLES.md

  • Documented default values for SE_NODE_REGISTER_PERIOD and
    SE_NODE_REGISTER_CYCLE.
  • Updated default values for event bus ports.
  • +4/-4     
    CONFIGURATION.md
    Document new node registration configurations                       

    charts/selenium-grid/CONFIGURATION.md

  • Documented nodeRegisterPeriod and nodeRegisterCycle configurations.
  • Added details for custom node capabilities.
  • +14/-0   
    Configuration changes
    value.yaml
    Update default environment variable values                             

    scripts/generate_list_env_vars/value.yaml

  • Set default values for SE_NODE_REGISTER_PERIOD and
    SE_NODE_REGISTER_CYCLE.
  • Updated default values for event bus ports.
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Logic

    The event bus configuration is now conditional on SE_EVENT_BUS_HOST being set. Verify this change doesn't break existing setups that rely on event bus configuration.

    if [[ -n "${SE_EVENT_BUS_HOST}" ]]; then
        echo "[events]
        publish = \"tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_PUBLISH_PORT}\"
        subscribe = \"tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_SUBSCRIBE_PORT}\"
        " >"$FILENAME"
    fi

    Copy link

    qodo-merge-pro bot commented Feb 18, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate required event bus configuration

    Add validation to ensure SE_EVENT_BUS_HOST is not empty when using event bus
    configuration, as the current conditional might silently fail if the host is
    undefined.

    NodeBase/generate_config [17-22]

    -if [[ -n "${SE_EVENT_BUS_HOST}" ]]; then
    +if [[ -z "${SE_EVENT_BUS_HOST}" ]]; then
    +    echo "Error: SE_EVENT_BUS_HOST is required for event bus configuration" >&2
    +    exit 1
    +else
         echo "[events]
         publish = \"tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_PUBLISH_PORT}\"
         subscribe = \"tcp://${SE_EVENT_BUS_HOST}:${SE_EVENT_BUS_SUBSCRIBE_PORT}\"
         " >"$FILENAME"
     fi
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds important error handling to prevent silent failures when SE_EVENT_BUS_HOST is undefined, which could lead to connectivity issues. This validation improves the robustness of the configuration process.

    Medium
    • Update

    Copy link

    qodo-merge-pro bot commented Feb 18, 2025

    CI Feedback 🧐

    (Feedback updated until commit ef5adf7)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub authentication token lacks the required 'read:org' permission
    scope. Specifically:

  • The workflow attempted to authenticate using gh auth login --with-token
  • The provided token (GH_CLI_TOKEN_PR) was invalid due to missing the 'read:org' scope
  • This resulted in an authentication error and the workflow terminated with exit code 1

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    28:  SecurityEvents: write
    29:  Statuses: write
    30:  ##[endgroup]
    31:  Secret source: Actions
    32:  Prepare workflow directory
    33:  Prepare all required actions
    34:  Getting action download info
    35:  Download action repository 'actions/checkout@main' (SHA:85e6279cec87321a52edac9c87bce653a07cf6c2)
    36:  Complete job name: Rerun workflow when failure
    ...
    
    48:  show-progress: true
    49:  lfs: false
    50:  submodules: false
    51:  set-safe-directory: true
    52:  env:
    53:  GH_CLI_TOKEN: ***
    54:  GH_CLI_TOKEN_PR: ***
    55:  RUN_ID: 13409513133
    56:  RERUN_FAILED_ONLY: true
    ...
    
    119:  ##[group]Run sudo apt update
    120:  �[36;1msudo apt update�[0m
    121:  �[36;1msudo apt install gh�[0m
    122:  shell: /usr/bin/bash -e {0}
    123:  env:
    124:  GH_CLI_TOKEN: ***
    125:  GH_CLI_TOKEN_PR: ***
    126:  RUN_ID: 13409513133
    127:  RERUN_FAILED_ONLY: true
    ...
    
    172:  0 upgraded, 0 newly installed, 0 to remove and 26 not upgraded.
    173:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    174:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    175:  shell: /usr/bin/bash -e {0}
    176:  env:
    177:  GH_CLI_TOKEN: ***
    178:  GH_CLI_TOKEN_PR: ***
    179:  RUN_ID: 13409513133
    180:  RERUN_FAILED_ONLY: true
    181:  RUN_ATTEMPT: 1
    182:  ##[endgroup]
    183:  error validating token: missing required scope 'read:org'
    184:  ##[error]Process completed with exit code 1.
    

    @VietND96 VietND96 force-pushed the node-register-period branch from 4c7ac27 to 5ad07ec Compare February 18, 2025 12:00
    @VietND96 VietND96 force-pushed the node-register-period branch from 5ad07ec to ef5adf7 Compare February 19, 2025 09:37
    @VietND96 VietND96 merged commit 111ba26 into trunk Feb 19, 2025
    25 of 27 checks passed
    @VietND96 VietND96 deleted the node-register-period branch February 19, 2025 13:20
    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