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: Multiple nodes browser in Helm configs #2475

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 26, 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

This pull request includes several changes to the Selenium Grid Helm chart to enhance its configuration and scalability. The key changes involve renaming deployment names for consistency, adding support for cross-browser nodes, and updating templates to accommodate these new configurations.

Example Usage

crossBrowsers:
  chromeNode:
    # Keep the first iteration with latest version of Chrome
    - nameOverride:
    - nameOverride: '{{ $.Release.Name }}-node-chrome-130'
      imageTag: '130.0'
      hpa:
        browserVersion: '130.0'
    - nameOverride: '{{ $.Release.Name }}-node-chrome-129'
      imageTag: '129.0'
      hpa:
        browserVersion: '129.0'
    - nameOverride: '{{ $.Release.Name }}-node-chrome-128'
      imageTag: '128.0'
      hpa:
        browserVersion: '128.0'
  firefoxNode:
    # Keep the first iteration with latest version of Firefox
    - nameOverride:
    - nameOverride: '{{ $.Release.Name }}-node-firefox-130'
      imageTag: '130.0'
      hpa:
        browserVersion: '130.0'
    - nameOverride: '{{ $.Release.Name }}-node-firefox-129'
      imageTag: '129.0'
      hpa:
        browserVersion: '129.0'
    - nameOverride: '{{ $.Release.Name }}-node-firefox-128'
      imageTag: '128.0'
      hpa:
        browserVersion: '128.0'
  edgeNode:
    # Keep the first iteration with latest version of Edge
    - nameOverride:
    - nameOverride: '{{ $.Release.Name }}-node-edge-130'
      imageTag: '130.0'
      hpa:
        browserVersion: '130.0'
    - nameOverride: '{{ $.Release.Name }}-node-edge-129'
      imageTag: '129.0'
      hpa:
        browserVersion: '129.0'
    - nameOverride: '{{ $.Release.Name }}-node-edge-128'
      imageTag: '128.0'
      hpa:
        browserVersion: '128.0'

Cross-Browser Support:

  • Added support for additional Chrome, Firefox, and Edge nodes in the Helm chart configuration. (charts/selenium-grid/CONFIGURATION.md: [1] charts/selenium-grid/cross-browsers-values.yaml: [2]

Template Updates:

  • Updated templates to handle the new cross-browser node configurations, including deployment, HPA, ScaledJob, and service templates. (charts/selenium-grid/templates/chrome-node-deployment.yaml: [1] charts/selenium-grid/templates/chrome-node-hpa.yaml: [2] charts/selenium-grid/templates/chrome-node-scaledjobs.yaml: [3] charts/selenium-grid/templates/chrome-node-service.yaml: [4] charts/selenium-grid/templates/edge-node-deployment.yaml: [5]

Name Helper Template:

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

  • Enhanced Selenium Grid Helm chart to support multiple nodes for Chrome, Firefox, Edge, and Relay.
  • Updated test scripts to reflect new naming conventions for browser nodes.
  • Added cross-browser configurations in values.yaml and cross-browsers-values.yaml.
  • Updated deployment, HPA, ScaledJob, and service templates to support cross-browser configurations.
  • Documented new configurations in CONFIGURATION.md.

Changes walkthrough 📝

Relevant files
Tests
2 files
bootstrap.sh
Add cross-browser values to Helm template commands             

tests/charts/bootstrap.sh

  • Added cross-browser values to Helm template commands.
  • Enhanced testing configurations for Selenium Grid.
  • +2/-0     
    test.py
    Update resource names in test cases                                           

    tests/charts/templates/test.py

  • Updated resource names for Chrome, Edge, and Firefox nodes.
  • Adjusted test cases to reflect new naming conventions.
  • +28/-28 
    Enhancement
    9 files
    _nameHelpers.tpl
    Rename node templates and add relay node support                 

    charts/selenium-grid/templates/_nameHelpers.tpl

  • Renamed node templates for consistency.
  • Added support for relay nodes.
  • +12/-4   
    chrome-node-deployment.yaml
    Support multiple Chrome node configurations                           

    charts/selenium-grid/templates/chrome-node-deployment.yaml

  • Added support for multiple Chrome node configurations.
  • Implemented templating for cross-browser support.
  • +23/-19 
    chrome-node-hpa.yaml
    Add HPA configuration for multiple Chrome nodes                   

    charts/selenium-grid/templates/chrome-node-hpa.yaml

  • Added HPA configuration for multiple Chrome nodes.
  • Enhanced templating for cross-browser support.
  • +16/-12 
    chrome-node-scaledjobs.yaml
    Add scaled job configuration for Chrome nodes                       

    charts/selenium-grid/templates/chrome-node-scaledjobs.yaml

  • Added scaled job configuration for multiple Chrome nodes.
  • Enhanced templating for cross-browser support.
  • +19/-15 
    chrome-node-service.yaml
    Add service configuration for Chrome nodes                             

    charts/selenium-grid/templates/chrome-node-service.yaml

  • Added service configuration for multiple Chrome nodes.
  • Enhanced templating for cross-browser support.
  • +18/-14 
    edge-node-deployment.yaml
    Support multiple Edge node configurations                               

    charts/selenium-grid/templates/edge-node-deployment.yaml

  • Added support for multiple Edge node configurations.
  • Implemented templating for cross-browser support.
  • +23/-19 
    edge-node-hpa.yaml
    Add HPA configuration for multiple Edge nodes                       

    charts/selenium-grid/templates/edge-node-hpa.yaml

  • Added HPA configuration for multiple Edge nodes.
  • Enhanced templating for cross-browser support.
  • +16/-12 
    edge-node-scaledjob.yaml
    Add scaled job configuration for Edge nodes                           

    charts/selenium-grid/templates/edge-node-scaledjob.yaml

  • Added scaled job configuration for multiple Edge nodes.
  • Enhanced templating for cross-browser support.
  • +19/-15 
    edge-node-service.yaml
    Add service configuration for Edge nodes                                 

    charts/selenium-grid/templates/edge-node-service.yaml

  • Added service configuration for multiple Edge nodes.
  • Enhanced templating for cross-browser support.
  • +21/-17 
    Configuration changes
    3 files
    selenium-grid-scaler.md
    Update deployment names in KEDA scaler configuration         

    .keda/scalers/selenium-grid-scaler.md

    • Updated deployment names for Chrome, Firefox, and Edge nodes.
    +14/-14 
    cross-browsers-values.yaml
    Add configurations for additional browser nodes                   

    charts/selenium-grid/cross-browsers-values.yaml

  • Added configurations for additional Chrome, Firefox, and Edge nodes.
  • Specified image tags and HPA settings for each browser version.
  • +46/-0   
    values.yaml
    Add cross-browser configurations to values.yaml                   

    charts/selenium-grid/values.yaml

  • Added cross-browser configurations for Chrome, Firefox, Edge, and
    Relay nodes.
  • +18/-0   
    Documentation
    1 files
    CONFIGURATION.md
    Document cross-browser node configurations                             

    charts/selenium-grid/CONFIGURATION.md

    • Documented additional cross-browser node configurations.
    +4/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Configuration Error
    The service port name is incorrectly set to 'tcp-chrome' for relay nodes, which could cause confusion and potential issues. Should be 'tcp-relay' instead.

    Version Compatibility
    Multiple browser versions (128-130) are configured but there's no validation to ensure version compatibility between browsers. This could lead to test inconsistencies.

    Resource Conflict
    Service configuration uses $.Values.firefoxNode.service.enabled but $nodeConfig.service.enabled for other checks, which could cause inconsistent service enablement.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect variable reference in template that could cause undefined value errors

    Fix the incorrect reference to .Values.videoRecorder.uploader.name which should be
    $podScope.recorder.uploader.name to maintain consistency with other node templates
    and avoid potential undefined values.

    charts/selenium-grid/templates/relay-node-deployment.yaml [36]

    -{{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder (.Values.videoRecorder.uploader.name | toString)) -}}
    +{{- $_ =  set $podScope "uploader" (get $.Values.videoRecorder ($podScope.recorder.uploader.name | toString)) -}}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion fixes a critical bug where using .Values.videoRecorder.uploader.name instead of $podScope.recorder.uploader.name could lead to undefined values and template rendering failures.

    8
    Fix template scope reference to ensure proper access to root context within range loops

    Replace the incorrect reference to '.' with '$' in the commonLabels include
    statement to ensure proper access to the root scope within the range loop.

    charts/selenium-grid/templates/firefox-node-service.yaml [11]

    -{{- include "seleniumGrid.commonLabels" . | nindent 4 }}
    +{{- include "seleniumGrid.commonLabels" $ | nindent 4 }}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The incorrect scope reference could cause template rendering errors or unexpected behavior. Using '$' instead of '.' is critical for accessing the root scope within range loops.

    8
    Fix boolean comparison in template condition to prevent potential rendering errors

    Fix the condition check by wrapping (include "seleniumGrid.useKEDA" $) in quotes to
    ensure proper string comparison, as the template function returns a string.

    charts/selenium-grid/templates/chrome-node-scaledjobs.yaml [3]

    -{{- if and $nodeConfig.enabled (include "seleniumGrid.useKEDA" $) (eq $.Values.autoscaling.scalingType "job") }}
    +{{- if and $nodeConfig.enabled (eq (include "seleniumGrid.useKEDA" $) "true") (eq $.Values.autoscaling.scalingType "job") }}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential bug by properly comparing the template function output with a string value, preventing incorrect condition evaluation.

    7
    General
    Ensure browser version consistency by deriving it from the image tag

    Add validation for browser version numbers to ensure they are valid semantic
    versions and match the corresponding imageTag.

    charts/selenium-grid/cross-browsers-values.yaml [8]

     hpa:
    -  browserVersion: '130.0'
    +  browserVersion: '{{ .imageTag }}'
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Automatically deriving the browser version from imageTag reduces the risk of version mismatches and maintenance overhead, improving configuration reliability.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit bea0769 into trunk Nov 26, 2024
    51 of 52 checks passed
    @VietND96 VietND96 deleted the chart-cross-browsers branch November 26, 2024 05:15
    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.

    [🚀 Feature]: Allow multiple nodes of the same type in Helm configuration
    1 participant