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: Node Relay to extend autoscaling Grid with test cloud resources #2703

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 11, 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, Tests, Configuration changes


Description

  • Introduced Relay Node for hybrid autoscaling with cloud providers.

    • Added support for test cloud credentials and relay configurations.
    • Updated Helm charts to include relay-specific configurations.
  • Enhanced Kubernetes test strategies with relay node testing.

  • Updated environment variables and configurations for relay node support.

  • Adjusted platform and browser configurations for compatibility.


Changes walkthrough 📝

Relevant files
Tests
1 files
chart_test.sh
Added relay node testing and secret creation.                       
+18/-2   
Enhancement
5 files
__init__.py
Added relay-specific environment variables and configurations.
+9/-1     
Dockerfile
Introduced `SE_NODE_RELAY_ONLY` environment variable.       
+1/-0     
generate_config
Adjusted stereotype generation for relay nodes.                   
+5/-2     
generate_relay_config
Enhanced relay configuration with dynamic URL and stereotype merging.
+13/-3   
generate_config
Updated stereotype generation for standalone relay nodes.
+4/-1     
Configuration changes
10 files
_helpers.tpl
Added relay node browser name configuration.                         
+4/-0     
helm-chart-test.yml
Added relay node test strategy in CI workflow.                     
+9/-0     
Makefile
Added relay node autoscaling job target.                                 
+9/-1     
multiple-nodes-platform-relay.yaml
Added relay node configurations for hybrid autoscaling.   
+82/-0   
multiple-nodes-platform.yaml
Updated platform names for compatibility.                               
+3/-3     
relay-node-scaledjobs.yaml
Adjusted relay node autoscaling template.                               
+1/-1     
values.yaml
Updated relay node default values.                                             
+2/-2     
DeploymentAutoscaling-values.yaml
Added relay node environment variable configuration.         
+6/-0     
JobAutoscaling-values.yaml
Added relay node environment variable configuration.         
+5/-0     
base-resources-values.yaml
Added resource limits for relay nodes.                                     
+9/-0     
Documentation
1 files
CONFIGURATION.md
Updated relay node configuration documentation.                   
+2/-2     

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: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The PR adds code that creates a Kubernetes secret containing cloud provider credentials (SAUCE_USERNAME, SAUCE_ACCESS_KEY) in the chart_test.sh file. While these are stored as Kubernetes secrets, the values are passed as environment variables and could potentially be exposed in logs or error messages. Additionally, the SE_NODE_RELAY_URL contains these credentials directly in the URL, which is a security risk if the URL is logged or exposed.

    ⚡ Recommended focus areas for review

    Logic Error

    The conditional logic for setting SE_NODE_STEREOTYPE has been changed, but the 'else' statement is now misplaced. This could cause unexpected behavior where SE_NODE_STEREOTYPE is not properly set in some scenarios.

    if [[ -z "${SE_NODE_STEREOTYPE}" ]] && [[ -n "${SE_NODE_BROWSER_NAME}" ]] && ([[ -z "${SE_NODE_RELAY_URL}" ]] || [[ "${SE_NODE_RELAY_ONLY}" = "false" ]]) ; then
    	SE_NODE_STEREOTYPE="{\"browserName\": \"${SE_NODE_BROWSER_NAME}\", \"browserVersion\": \"${SE_NODE_BROWSER_VERSION}\", \"platformName\": \"${SE_NODE_PLATFORM_NAME}\", \"se:containerName\": \"${SE_NODE_CONTAINER_NAME}\", \"container:hostname\": \"$(hostname)\"}"
    	if [[ -n "${SE_BROWSER_BINARY_LOCATION}" ]]; then
    	    SE_NODE_STEREOTYPE="$(python3 /opt/bin/json_merge.py "${SE_NODE_STEREOTYPE}" "${SE_BROWSER_BINARY_LOCATION}")"
    	fi
    else
    	SE_NODE_STEREOTYPE="${SE_NODE_STEREOTYPE}"
    fi
    Variable Reference

    The uploader variable reference was changed from '.Values.videoRecorder.uploader.name' to '$podScope.recorder.uploader.name', which might cause issues if the recorder structure doesn't contain the expected uploader name property.

    {{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}
    Environment Variable Expansion

    The implementation of environment variable substitution in the relay URL might not work as expected if the environment variables contain special characters or if the envsubst command is not available in the container.

    echo "url = \"$(envsubst < <(echo ${SE_NODE_RELAY_URL}))\"" >>"$FILENAME"

    Copy link

    qodo-merge-pro bot commented Mar 11, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix process substitution syntax

    The command using envsubst with process substitution has a syntax error. The
    current implementation pipes the output of echo into envsubst using process
    substitution, but this is not the correct way to use envsubst. This could cause
    the relay URL to be incorrectly processed.

    NodeBase/generate_relay_config [11]

     if [[ -n "${SE_NODE_RELAY_URL}" ]]; then
     	echo "[relay]" >>"$FILENAME"
    -	echo "url = \"$(envsubst < <(echo ${SE_NODE_RELAY_URL}))\"" >>"$FILENAME"
    +	echo "url = \"$(echo ${SE_NODE_RELAY_URL} | envsubst)\"" >>"$FILENAME"

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The current process substitution syntax is incorrect and could lead to the relay URL being improperly processed. The suggested fix uses a proper pipe to envsubst, which is the standard way to handle variable substitution in shell scripts.

    Medium
    • Update

    Copy link

    qodo-merge-pro bot commented Mar 11, 2025

    CI Feedback 🧐

    (Feedback updated until commit 465aa4e)

    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 token provided in GH_CLI_TOKEN_PR is missing the required
    permission scope 'read:org'. When attempting to authenticate with GitHub CLI using this token, the
    authentication failed with the error message "error validating token: missing required scope
    'read:org'".

    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: 13808724963
    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: 13808724963
    127:  RERUN_FAILED_ONLY: true
    ...
    
    162:  0 upgraded, 0 newly installed, 0 to remove and 49 not upgraded.
    163:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    164:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    165:  shell: /usr/bin/bash -e {0}
    166:  env:
    167:  GH_CLI_TOKEN: ***
    168:  GH_CLI_TOKEN_PR: ***
    169:  RUN_ID: 13808724963
    170:  RERUN_FAILED_ONLY: true
    171:  RUN_ATTEMPT: 2
    172:  ##[endgroup]
    173:  error validating token: missing required scope 'read:org'
    174:  ##[error]Process completed with exit code 1.
    

    @VietND96 VietND96 force-pushed the node-relay-cloud branch 6 times, most recently from 214da08 to a27124a Compare March 12, 2025 07:30
    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