Skip to content

K8s: Distributor uses Greedy algo as slot selector strategy in autoscaling #2875

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

Merged
merged 2 commits into from
Jul 2, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 2, 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

Add the implementation in Grid core to Docker and Helm chart - SeleniumHQ/selenium#15897 ([grid] Add GreedySlotSelector as a built-in slot-selector option)

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


Description

  • Add slot selector strategy configuration for Selenium Grid

  • Enable GreedySlotSelector as default for autoscaling scenarios

  • Support slot selector in Hub, Distributor, and Standalone modes

  • Update Helm chart with new configuration options


Changes diagram

flowchart LR
  A["Shell Scripts"] --> B["SE_DISTRIBUTOR_SLOT_SELECTOR"]
  C["Helm Templates"] --> D["slotSelectorStrategy Config"]
  D --> E["Hub Deployment"]
  D --> F["Distributor Deployment"]
  G["values.yaml"] --> H["GreedySlotSelector Default"]
  H --> I["Autoscaling Optimization"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
6 files
start-selenium-grid-distributor.sh
Add slot selector environment variable support                     
+4/-0     
start-selenium-grid-hub.sh
Add slot selector environment variable support                     
+4/-0     
start-selenium-grid-standalone.sh
Add slot selector environment variable support                     
_helpers.tpl
Add slot selector strategy helper template                             
+10/-0   
distributor-deployment.yaml
Configure slot selector environment variable                         
+4/-0     
hub-deployment.yaml
Configure slot selector environment variable                         
+4/-0     
Documentation
1 files
CONFIGURATION.md
Document new slot selector configuration options                 
+3/-0     
Configuration changes
1 files
values.yaml
Add slot selector configuration with defaults                       
+10/-0   
Additional files
1 files
start-selenium-standalone.sh +4/-0     

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
    Contributor

    qodo-merge-pro bot commented Jul 2, 2025

    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

    Logic Complexity

    The template logic for determining slot selector strategy has complex conditional branching between KEDA and non-KEDA modes. The ternary operator usage should be validated to ensure correct behavior in all scenarios.

    {{- define "seleniumGrid.autoscaling.distributor.slotSelector" -}}
    {{- $slotSelector := "" -}}
    {{- if eq (include "seleniumGrid.useKEDA" $) "true" -}}
    {{- $slotSelector = $.Values.autoscaling.slotSelectorStrategy -}}
    {{- else -}}
    {{- $slotSelector = $.Values.isolateComponents | ternary $.Values.components.distributor.slotSelectorStrategy $.Values.hub.slotSelectorStrategy -}}
    {{- end -}}
    {{- $slotSelector -}}
    {{- end -}}
    Default Value

    The default slot selector strategy is set to GreedySlotSelector which changes existing behavior. This breaking change should be carefully validated for backward compatibility impact.

    slotSelectorStrategy: "org.openqa.selenium.grid.distributor.selector.GreedySlotSelector"
    # -- Specify an external KEDA TriggerAuthentication resource is used for scaler triggers config. Apply for all browser nodes

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 2, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add safe default for undefined variable

    The template logic assumes $.Values.isolateComponents exists but this variable
    is not defined in the values.yaml. This could cause template rendering failures
    when components are not isolated. Add a default value or safe check for this
    variable.

    charts/selenium-grid/templates/_helpers.tpl [205-213]

     {{- define "seleniumGrid.autoscaling.distributor.slotSelector" -}}
     {{- $slotSelector := "" -}}
     {{- if eq (include "seleniumGrid.useKEDA" $) "true" -}}
     {{- $slotSelector = $.Values.autoscaling.slotSelectorStrategy -}}
     {{- else -}}
    -{{- $slotSelector = $.Values.isolateComponents | ternary $.Values.components.distributor.slotSelectorStrategy $.Values.hub.slotSelectorStrategy -}}
    +{{- $slotSelector = (default false $.Values.isolateComponents) | ternary $.Values.components.distributor.slotSelectorStrategy $.Values.hub.slotSelectorStrategy -}}
     {{- end -}}
     {{- $slotSelector -}}
     {{- end -}}
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that using $.Values.isolateComponents without a default value can make the Helm template rendering fail if the value is not defined, and the proposed fix using default is a robust improvement.

    Medium
    • Update

    @VietND96 VietND96 changed the title K8s: Distributor uses Greedy as slot selector strategy in autoscaling K8s: Distributor uses Greedy algo as slot selector strategy in autoscaling Jul 2, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 2, 2025

    CI Feedback 🧐

    (Feedback updated until commit c146faf)

    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 during GitHub CLI authentication. The token provided via GH_CLI_TOKEN_PR is
    missing the required 'read:org' scope, which is necessary for the GitHub CLI to function properly.

    Relevant error logs:
    1:  ##[group]Runner Image Provisioner
    2:  Hosted Compute Agent
    ...
    
    24:  Issues: write
    25:  Metadata: read
    26:  Models: read
    27:  Packages: write
    28:  Pages: write
    29:  PullRequests: write
    30:  RepositoryProjects: write
    31:  SecurityEvents: write
    32:  Statuses: write
    33:  ##[endgroup]
    34:  Secret source: Actions
    35:  Prepare workflow directory
    36:  Prepare all required actions
    37:  Getting action download info
    38:  Download action repository 'actions/checkout@main' (SHA:09d2acae674a48949e3602304ab46fd20ae0c42f)
    39:  Complete job name: Rerun workflow when failure
    40:  ##[group]Run actions/checkout@main
    ...
    
    44:  ssh-strict: true
    45:  ssh-user: git
    46:  persist-credentials: true
    47:  clean: true
    48:  sparse-checkout-cone-mode: true
    49:  fetch-depth: 1
    50:  fetch-tags: false
    51:  show-progress: true
    52:  lfs: false
    53:  submodules: false
    54:  set-safe-directory: true
    55:  env:
    56:  GH_CLI_TOKEN: ***
    57:  GH_CLI_TOKEN_PR: ***
    58:  RUN_ID: 16036992847
    59:  RERUN_FAILED_ONLY: true
    60:  RUN_ATTEMPT: 1
    ...
    
    115:  Or undo this operation with:
    116:  git switch -
    117:  Turn off this advice by setting config variable advice.detachedHead to false
    118:  HEAD is now at 3661f33 Merge c146faf79bf52ccb3868d2e3f20b7f91000373c5 into 2641d8d9f59a90ce1a6230cb69bf433a9949b706
    119:  ##[endgroup]
    120:  [command]/usr/bin/git log -1 --format=%H
    121:  3661f3354ed3afbaeb3b9f404152a8457703cbf9
    122:  ##[group]Run sudo apt update
    123:  �[36;1msudo apt update�[0m
    124:  �[36;1msudo apt install gh�[0m
    125:  shell: /usr/bin/bash -e {0}
    126:  env:
    127:  GH_CLI_TOKEN: ***
    128:  GH_CLI_TOKEN_PR: ***
    129:  RUN_ID: 16036992847
    130:  RERUN_FAILED_ONLY: true
    131:  RUN_ATTEMPT: 1
    ...
    
    176:  Reading state information...
    177:  46 packages can be upgraded. Run 'apt list --upgradable' to see them.
    178:  WARNING: apt does not have a stable CLI interface. Use with caution in scripts.
    179:  Reading package lists...
    180:  Building dependency tree...
    181:  Reading state information...
    182:  gh is already the newest version (2.74.2).
    183:  0 upgraded, 0 newly installed, 0 to remove and 46 not upgraded.
    184:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    185:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    186:  shell: /usr/bin/bash -e {0}
    187:  env:
    188:  GH_CLI_TOKEN: ***
    189:  GH_CLI_TOKEN_PR: ***
    190:  RUN_ID: 16036992847
    191:  RERUN_FAILED_ONLY: true
    192:  RUN_ATTEMPT: 1
    193:  ##[endgroup]
    194:  error validating token: missing required scope 'read:org'
    195:  ##[error]Process completed with exit code 1.
    196:  Post job cleanup.
    

    @VietND96 VietND96 force-pushed the autoscaling-slot-selector branch 2 times, most recently from 311f2db to afe068d Compare July 2, 2025 17:45
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 force-pushed the autoscaling-slot-selector branch from afe068d to 88646b1 Compare July 2, 2025 19:05
    @VietND96 VietND96 merged commit 3c1453e into trunk Jul 2, 2025
    23 of 28 checks passed
    @VietND96 VietND96 deleted the autoscaling-slot-selector branch July 2, 2025 23:00
    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