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

Experimental: Selenium Grid scaler in K8s implementation preview #2400

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Sep 19, 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

Motivation and Context

Introduction

Selenium Grid Scaler is a built-in scaler is maintained in upstream KEDA repository. The scaler implementation could be found here. The official docs of the scaler could be seen here.

Now, SeleniumHQ/docker-selenium involves as the maintainer for the scaler.

In order to deliver and get feedback continuously on any new bug fixes, improvement, or features for the Selenium Grid scaler. We select the latest stable version of KEDA core, patch the scaler implementation then build and deploy KEDA container images following our image tag convention.

The stable implementation will be merged to the upstream KEDA repository frequently and will be available in the next KEDA core release.

Read more: https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.keda/README.md

Fixes #1925
Fixes #2160
Fixes #1904

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, Tests, Other


Description

  • Implemented Selenium Grid Scaler logic and integrated it with KEDA for scaling based on Selenium Grid metrics.
  • Added comprehensive test cases for Selenium Grid Scaler covering various scenarios.
  • Updated KEDA image configuration and Helm chart settings with Selenium-specific parameters.
  • Added documentation for Selenium Grid Scaler usage, configuration, and integration with KEDA.
  • Updated deployment workflow to include Selenium Grid scaler resources.
  • Introduced new environment variables and updated default values for KEDA configurations.

Changes walkthrough 📝

Relevant files
Configuration changes
5 files
chart_test.sh
Update KEDA Helm chart configuration and defaults.             

tests/charts/make/chart_test.sh

  • Added new environment variables KEDA_BASED_NAME and KEDA_BASED_TAG.
  • Changed default value of TEST_EXISTING_KEDA to "false".
  • Updated Helm commands to use new KEDA image settings.
  • +15/-3   
    CONFIGURATION.md
    Update KEDA image configuration in Selenium Grid chart.   

    charts/selenium-grid/CONFIGURATION.md

    • Updated KEDA image configuration with Selenium-specific settings.
    +4/-1     
    values.yaml
    Configure KEDA components and enable webhooks                       

    charts/selenium-grid/values.yaml

  • Added configuration for KEDA image specifications.
  • Enabled KEDA admission webhooks component.
  • +17/-1   
    deploy.yml
    Update deployment workflow to include grid scaler resources

    .github/workflows/deploy.yml

  • Added step to fetch Selenium Grid scaler resources in the deployment
    workflow.
  • +3/-1     
    base-auth-ingress-values.yaml
    Enable KEDA webhooks in base auth ingress configuration   

    tests/charts/ci/base-auth-ingress-values.yaml

    • Enabled KEDA webhooks in base authentication ingress values.
    +4/-0     
    Enhancement
    3 files
    update_tag_in_docs_and_files.sh
    Add KEDA tag version update logic.                                             

    update_tag_in_docs_and_files.sh

    • Added logic to update KEDA tag versions in files.
    +5/-0     
    selenium_grid_scaler.go
    Implement Selenium Grid Scaler for KEDA integration.         

    .keda/scalers/selenium_grid_scaler.go

  • Implemented Selenium Grid Scaler logic.
  • Added functions for parsing metadata and counting sessions.
  • Integrated with KEDA for scaling based on Selenium Grid metrics.
  • +379/-0 
    Makefile
    Add Selenium Grid scaler integration and release processes

    Makefile

  • Added variables for KEDA versioning and naming.
  • Introduced new targets for fetching and releasing Selenium Grid scaler
    resources and images.
  • Integrated grid scaler release steps into existing release processes.
  • Updated chart test configurations to include KEDA parameters.
  • +41/-10 
    Documentation
    4 files
    generate_chart_changelog.sh
    Add experimental section for Selenium Grid Scaler.             

    generate_chart_changelog.sh

  • Added a new "Experimental" section to the changelog for Selenium Grid
    Scaler.
  • +4/-0     
    README.md
    Document Selenium Grid Scaler usage and configuration.     

    .keda/README.md

  • Added documentation for Selenium Grid Scaler usage and configuration.
  • Included instructions for using patched KEDA images.
  • +62/-0   
    selenium-grid-scaler.md
    Document Selenium Grid Scaler configuration and usage       

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

  • Added documentation for Selenium Grid Scaler.
  • Included trigger specification and example configurations.
  • Provided authentication parameters and examples.
  • +207/-0 
    README.md
    Update README with Selenium Grid scaler preview information

    charts/selenium-grid/README.md

  • Added note about previewing new changes in Selenium Grid scaler
    implementation.
  • +2/-0     
    Tests
    2 files
    selenium_grid_scaler_test.go
    Add test cases for Selenium Grid Scaler functionality.     

    .keda/scalers/selenium_grid_scaler_test.go

  • Added comprehensive test cases for Selenium Grid Scaler.
  • Tests cover various scenarios for session queue requests and node
    capabilities.
  • +1808/-0
    DeploymentAutoscaling-values.yaml
    Adjust max replica count in deployment autoscaling test   

    tests/charts/ci/DeploymentAutoscaling-values.yaml

    • Increased max replica count for deployment autoscaling test.
    +1/-1     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Test Coverage
    Extensive new test cases have been added for the Selenium Grid scaler. These tests cover various scenarios and edge cases. The reviewer should carefully examine these tests to ensure they adequately cover the scaler's functionality and potential failure modes.

    Documentation Update
    New documentation has been added explaining the Selenium Grid Scaler, its maintenance, and how to use the patched version. The reviewer should ensure this documentation is clear, accurate, and provides sufficient guidance for users.

    Configuration Changes
    The KEDA configuration in the Helm chart values has been updated to use Selenium-specific images. The reviewer should verify these changes are correct and consistent with the new scaler implementation.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Use maps instead of slices for faster node lookup operations

    Consider using a more efficient data structure, such as a map, for reservedNodes and
    newRequestNodes to improve lookup performance when checking for existing nodes.

    .keda/scalers/selenium_grid_scaler.go [310-312]

    -var reservedNodes []ReservedNodes
    -var newRequestNodes []ReservedNodes
    +reservedNodes := make(map[string]ReservedNodes)
    +newRequestNodes := make(map[string]ReservedNodes)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using maps instead of slices for node lookups is a significant performance improvement, especially when dealing with large datasets, making this a valuable suggestion.

    9
    Maintainability
    Use a function to extract version numbers from the Makefile to reduce code duplication

    Consider using a function to extract version numbers from the Makefile to reduce
    code duplication and improve maintainability.

    update_tag_in_docs_and_files.sh [8-12]

    -FFMPEG_TAG_PREV_VERSION=$(grep FFMPEG_TAG_PREV_VERSION Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1)
    -FFMPEG_TAG_VERSION=$(grep FFMPEG_TAG_VERSION Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1)
    -RCLONE_TAG_VERSION=$(grep RCLONE_TAG_VERSION Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1)
    -KEDA_TAG_PREV_VERSION=$(grep KEDA_TAG_PREV_VERSION Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1)
    -KEDA_TAG_VERSION=$(grep KEDA_TAG_VERSION Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1)
    +get_version() {
    +  grep "$1" Makefile | sed 's/.*,\([^)]*\))/\1/p' | head -n 1
    +}
     
    +FFMPEG_TAG_PREV_VERSION=$(get_version FFMPEG_TAG_PREV_VERSION)
    +FFMPEG_TAG_VERSION=$(get_version FFMPEG_TAG_VERSION)
    +RCLONE_TAG_VERSION=$(get_version RCLONE_TAG_VERSION)
    +KEDA_TAG_PREV_VERSION=$(get_version KEDA_TAG_PREV_VERSION)
    +KEDA_TAG_VERSION=$(get_version KEDA_TAG_VERSION)
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Introducing a function to extract version numbers reduces code duplication and enhances maintainability. This change simplifies the code and makes it easier to manage, which is beneficial for long-term maintenance.

    8
    Use a template function to reduce repetition in grid scaler release targets

    Consider using a loop or a function to reduce repetition in the
    'release_grid_scaler', 'release_grid_scaler_latest', and
    'release_grid_scaler_nightly' targets.

    Makefile [261-274]

    -release_grid_scaler: fetch_grid_scaler_images
    -	docker buildx imagetools create -t $(NAME)/keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
    +define release_grid_scaler_template
    +release_grid_scaler$(1): fetch_grid_scaler_images
    +	docker buildx imagetools create -t $(NAME)/keda$(2) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    +	docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver$(2) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    +	docker buildx imagetools create -t $(NAME)/keda-admission-webhooks$(2) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
    +endef
     
    -release_grid_scaler_latest: fetch_grid_scaler_images
    -	docker buildx imagetools create -t $(NAME)/keda:latest $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:latest $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:latest $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
    +$(eval $(call release_grid_scaler_template,,:$(KEDA_TAG_VERSION)-$(BUILD_DATE)))
    +$(eval $(call release_grid_scaler_template,_latest,:latest))
    +$(eval $(call release_grid_scaler_template,_nightly,:nightly))
     
    -release_grid_scaler_nightly: fetch_grid_scaler_images
    -	docker buildx imagetools create -t $(NAME)/keda:nightly $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:nightly $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    -	docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:nightly $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion effectively reduces code repetition and enhances maintainability by using a template function, which is a significant improvement for code clarity and future updates.

    8
    Use an array to store KEDA image settings for improved maintainability and reduced repetition

    Consider using an array to store the KEDA image settings and then join them in the
    Helm command. This approach would make the code more maintainable and reduce
    repetition.

    tests/charts/make/chart_test.sh [340-344]

    -HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} \
    ---set keda.image.keda.registry=${KEDA_BASED_NAME} --set keda.image.keda.repository=keda --set keda.image.keda.tag=${KEDA_BASED_TAG} \
    ---set keda.image.metricsApiServer.registry=${KEDA_BASED_NAME} --set keda.image.metricsApiServer.repository=keda-metrics-apiserver --set keda.image.metricsApiServer.tag=${KEDA_BASED_TAG} \
    ---set keda.image.webhooks.registry=${KEDA_BASED_NAME} --set keda.image.webhooks.repository=keda-admission-webhooks --set keda.image.webhooks.tag=${KEDA_BASED_TAG} \
    -"
    +keda_images=(
    +  "keda.image.keda.registry=${KEDA_BASED_NAME}"
    +  "keda.image.keda.repository=keda"
    +  "keda.image.keda.tag=${KEDA_BASED_TAG}"
    +  "keda.image.metricsApiServer.registry=${KEDA_BASED_NAME}"
    +  "keda.image.metricsApiServer.repository=keda-metrics-apiserver"
    +  "keda.image.metricsApiServer.tag=${KEDA_BASED_TAG}"
    +  "keda.image.webhooks.registry=${KEDA_BASED_NAME}"
    +  "keda.image.webhooks.repository=keda-admission-webhooks"
    +  "keda.image.webhooks.tag=${KEDA_BASED_TAG}"
    +)
    +HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} $(printf -- '--set %s ' "${keda_images[@]}")"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using an array to store the KEDA image settings improves maintainability by reducing repetition and making the code easier to update in the future. This is a good practice for cleaner code, though it doesn't address a critical issue.

    7
    Encapsulate parameters in a struct to improve function signature readability

    Consider using a struct to encapsulate the parameters passed to the
    checkCapabilitiesMatch function to improve readability and maintainability.

    .keda/scalers/selenium_grid_scaler.go [250]

    -func checkCapabilitiesMatch(capability Capability, requestCapability Capability, browserName string, browserVersion string, sessionBrowserName string, platformName string) bool {
    +type CapabilityMatchParams struct {
    +    capability         Capability
    +    requestCapability  Capability
    +    browserName        string
    +    browserVersion     string
    +    sessionBrowserName string
    +    platformName       string
    +}
     
    +func checkCapabilitiesMatch(params CapabilityMatchParams) bool {
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Encapsulating parameters in a struct improves readability and maintainability of the function signature, but it does not address any critical issues.

    7
    Use a heredoc for the multi-line Helm command to improve readability

    Consider using a heredoc for the multi-line Helm command to improve readability and
    maintainability.

    tests/charts/make/chart_test.sh [334-338]

    -helm upgrade -i ${KEDA_NAMESPACE} -n ${KEDA_NAMESPACE} --create-namespace --set webhooks.enabled=true \
    ---set image.keda.registry=${KEDA_BASED_NAME} --set image.keda.repository=keda --set image.keda.tag=${KEDA_BASED_TAG} \
    ---set image.metricsApiServer.registry=${KEDA_BASED_NAME} --set image.metricsApiServer.repository=keda-metrics-apiserver --set image.metricsApiServer.tag=${KEDA_BASED_TAG} \
    ---set image.webhooks.registry=${KEDA_BASED_NAME} --set image.webhooks.repository=keda-admission-webhooks --set image.webhooks.tag=${KEDA_BASED_TAG} \
    -kedacore/keda
    +helm upgrade -i ${KEDA_NAMESPACE} -n ${KEDA_NAMESPACE} --create-namespace kedacore/keda <<EOF
    +  --set webhooks.enabled=true
    +  --set image.keda.registry=${KEDA_BASED_NAME}
    +  --set image.keda.repository=keda
    +  --set image.keda.tag=${KEDA_BASED_TAG}
    +  --set image.metricsApiServer.registry=${KEDA_BASED_NAME}
    +  --set image.metricsApiServer.repository=keda-metrics-apiserver
    +  --set image.metricsApiServer.tag=${KEDA_BASED_TAG}
    +  --set image.webhooks.registry=${KEDA_BASED_NAME}
    +  --set image.webhooks.repository=keda-admission-webhooks
    +  --set image.webhooks.tag=${KEDA_BASED_TAG}
    +EOF
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a heredoc for multi-line commands can enhance readability and maintainability, making the code easier to understand and modify. However, this suggestion is more about style and readability than addressing a functional issue.

    6
    Enhancement
    Add more comprehensive test cases to cover edge cases and diverse scenarios

    Consider adding more specific test cases that cover edge cases and boundary
    conditions, such as testing with maximum allowed sessions, or with a mix of
    different browser versions and platform names.

    .keda/scalers/selenium_grid_scaler_test.go [21-42]

     tests := []struct {
         name    string
         args    args
         want    int64
         wantErr bool
     }{
         // ... existing test cases ...
    +    {
    +        name: "max allowed sessions reached",
    +        args: args{
    +            b:                  []byte(`{"data":{"grid":{"maxSession":10},"sessionsInfo":{"sessionQueueRequests":["","","","","","","","","","",""]}}}`),
    +            browserName:        "chrome",
    +            sessionBrowserName: "chrome",
    +            browserVersion:     "latest",
    +            platformName:       "linux",
    +            nodeMaxSessions:    10,
    +        },
    +        want:    1,
    +        wantErr: false,
    +    },
    +    {
    +        name: "mix of different browser versions and platforms",
    +        args: args{
    +            b:                  []byte(`{"data":{"grid":{},"sessionsInfo":{"sessionQueueRequests":["{\"browserName\":\"chrome\",\"browserVersion\":\"91.0\",\"platformName\":\"windows\"}","{\"browserName\":\"chrome\",\"browserVersion\":\"92.0\",\"platformName\":\"linux\"}"]}}}`),
    +            browserName:        "chrome",
    +            sessionBrowserName: "chrome",
    +            browserVersion:     "latest",
    +            platformName:       "any",
    +            nodeMaxSessions:    5,
    +        },
    +        want:    2,
    +        wantErr: false,
    +    },
     }
     
    Suggestion importance[1-10]: 8

    Why: Adding more specific test cases for edge cases and boundary conditions is important for ensuring robustness and reliability of the code, making this a valuable enhancement.

    8
    Refactor test cases to use a more concise and maintainable table-driven test structure

    Consider using table-driven tests to reduce code duplication and improve test
    maintainability. This approach allows for easier addition of new test cases and
    better organization of test data.

    .keda/scalers/selenium_grid_scaler_test.go [21-42]

     tests := []struct {
    -    name    string
    -    args    args
    -    want    int64
    -    wantErr bool
    +    name             string
    +    responseBody     []byte
    +    browserName      string
    +    expectedCount    int64
    +    expectedError    bool
     }{
         {
    -        name: "nil response body should throw error",
    -        args: args{
    -            b:           []byte(nil),
    -            browserName: "",
    -        },
    -        wantErr: true,
    +        name:          "nil response body should throw error",
    +        responseBody:  nil,
    +        browserName:   "",
    +        expectedCount: 0,
    +        expectedError: true,
         },
         {
    -        name: "empty response body should throw error",
    -        args: args{
    -            b:           []byte(""),
    -            browserName: "",
    -        },
    -        wantErr: true,
    +        name:          "empty response body should throw error",
    +        responseBody:  []byte(""),
    +        browserName:   "",
    +        expectedCount: 0,
    +        expectedError: true,
         },
         // ... more test cases ...
     }
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to refactor the test cases into a more concise table-driven format improves readability and maintainability, making it easier to add new test cases. However, it is not crucial for functionality.

    7
    Introduce a variable for the common Docker image name prefix to improve maintainability and reduce repetition

    Consider using a variable for the common Docker image name prefix 'selenium/' to
    improve maintainability and reduce repetition.

    Makefile [262-264]

    -docker buildx imagetools create -t $(NAME)/keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    -docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    -docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
    +DOCKER_IMAGE_PREFIX := $(NAME)/
    +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG)
    +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG)
    +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a variable for the Docker image name prefix is valid as it reduces repetition and improves maintainability, but it is not a critical change.

    7
    Best practice
    Introduce a constant for the default node max sessions value to improve flexibility and readability

    Consider using a constant or a variable for the nodeMaxSessions value in the args
    struct, as it's currently hardcoded and might need to be adjusted for different test
    cases.

    .keda/scalers/selenium_grid_scaler_test.go [13-20]

    +const defaultNodeMaxSessions = 5
    +
     type args struct {
         b                  []byte
         browserName        string
         sessionBrowserName string
         browserVersion     string
         platformName       string
         nodeMaxSessions    int
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a constant for nodeMaxSessions enhances code readability and flexibility, allowing for easier adjustments in the future. It is a good practice but not critical for the current implementation.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link
    Member Author

    @VietND96 VietND96 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Merged

    @VietND96 VietND96 merged commit 2af6166 into SeleniumHQ:trunk Sep 19, 2024
    29 of 33 checks passed
    @VietND96 VietND96 added this to the 4.25.0 milestone Sep 22, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment