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

chart(update): Node deployment replicas use minReplicaCount in autoscaling #2430

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 14, 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

When enabling autoscaling, type: Deployment. In chart template, field replicas is not defined in deployment spec, K8s will assume it as 1 by default.
When autoscaling.scaledOptions.minReplicaCount set as 0, we can see 1 Node pod is up then it's Terminating to reach replicas 0/0
Or when autoscaling.scaledOptions.minReplicaCount set > 1, also there is 1 Node pod up from the beginning before it is scaled up to reach the minReplicaCount.
So, when enabling autoscaling && type = Deployment, minReplicaCount is used to set replicas in deployment spec

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 test scripts to provide more debugging information by retrieving all resources in all namespaces.
  • Updated Selenium test to remove an unused variable.
  • Modified CI configuration to include parameters for Firefox language package installation and managed downloads.
  • Updated Makefile to accommodate new environment variables for Firefox and download configurations.
  • Changed default update strategy to RollingUpdate in chart configurations.
  • Set node deployment replicas based on minReplicaCount when autoscaling is enabled for Chrome, Edge, and Firefox nodes.
  • Bumped chart version and Prometheus dependency version.

Changes walkthrough 📝

Relevant files
Enhancement
4 files
chart_test.sh
Enhance debugging information in test script                         

tests/charts/make/chart_test.sh

  • Added commands to get all resources in all namespaces.
  • Removed redundant commands for getting pod status.
  • +3/-6     
    chrome-node-deployment.yaml
    Use minReplicaCount for Chrome node replicas                         

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

  • Set replicas based on minReplicaCount when autoscaling is enabled.
  • +4/-2     
    edge-node-deployment.yaml
    Use minReplicaCount for Edge node replicas                             

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

  • Set replicas based on minReplicaCount when autoscaling is enabled.
  • +4/-2     
    firefox-node-deployment.yaml
    Use minReplicaCount for Firefox node replicas                       

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

  • Set replicas based on minReplicaCount when autoscaling is enabled.
  • +4/-2     
    Miscellaneous
    1 files
    __init__.py
    Clean up unused variable in Selenium test                               

    tests/SeleniumTests/init.py

    • Removed unused variable is_continue.
    +0/-1     
    Tests
    1 files
    test.py
    Update strategy assertion change in test                                 

    tests/charts/templates/test.py

  • Changed update strategy assertion from Recreate to RollingUpdate.
  • +1/-1     
    Configuration changes
    3 files
    config.yml
    Add configuration for Firefox language and downloads in CI

    .circleci/config.yml

  • Added parameters for Firefox language package installation and managed
    downloads.
  • Updated Docker test jobs to include new parameters.
  • +27/-1   
    Makefile
    Update Makefile for Firefox and download configurations   

    Makefile

  • Modified test commands to include new environment variables for
    Firefox and downloads.
  • +3/-3     
    values.yaml
    Default update strategy set to RollingUpdate                         

    charts/selenium-grid/values.yaml

    • Changed default update strategy to RollingUpdate.
    +1/-1     
    Documentation
    1 files
    CONFIGURATION.md
    Update chart version and default strategy                               

    charts/selenium-grid/CONFIGURATION.md

  • Updated chart version and Prometheus chart dependency version.
  • Changed default update strategy to RollingUpdate.
  • +3/-3     
    Dependencies
    1 files
    Chart.yaml
    Bump chart and dependency versions                                             

    charts/selenium-grid/Chart.yaml

    • Updated chart version and Prometheus dependency version.
    +2/-2     

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

    Sorry, something went wrong.

    …aling
    
    Signed-off-by: Viet Nguyen Duc <[email protected]>
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Bug
    The new logic for setting replicas based on autoscaling may not handle all cases correctly. Verify that the replicas are set correctly for all scenarios, including when autoscaling is disabled.

    Configuration Change
    The default update strategy has been changed from 'Recreate' to 'RollingUpdate'. Ensure this change doesn't cause issues with existing deployments or during updates.

    Test Configuration
    New environment variables have been added to the test commands. Verify that these changes are consistent across all relevant test scenarios and don't break existing tests.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Improve CircleCI configuration maintainability by using YAML anchors and aliases for common job parameters

    Consider using a YAML anchor and alias to define common job parameters, reducing
    repetition in the CircleCI configuration.

    .circleci/config.yml [76-84]

    +docker-test-base: &docker-test-base
    +  test-strategy: test
    +  platforms: linux/arm64
    +  machine-type: ubuntu2204arm64
    +  firefox-install-lang-package: false
    +  enable-managed-downloads: false
    +
     - docker-test:
    +    <<: *docker-test-base
         name: "Docker test - Use random user (true)"
    -    test-strategy: test
         use-random-user: true
    -    platforms: linux/arm64
    -    machine-type: ubuntu2204arm64
    -    firefox-install-lang-package: false
    -    enable-managed-downloads: false
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion significantly enhances maintainability by reducing repetition in the CircleCI configuration, making it easier to manage and update common parameters across multiple jobs.

    8
    Reduce code duplication in deployment files by using a helper template for replica count

    Consider using a helper template for setting the replica count to reduce code
    duplication across node deployment files.

    charts/selenium-grid/templates/chrome-node-deployment.yaml [20-24]

    -{{- if not (eq (include "seleniumGrid.useKEDA" $) "true") }}
    -replicas: {{ .Values.chromeNode.replicas }}
    -{{- else }}
    -replicas: {{ default $.Values.autoscaling.scaledOptions.minReplicaCount ($.Values.chromeNode.scaledOptions).minReplicaCount }}
    -{{ end }}
    +replicas: {{ include "seleniumGrid.getReplicaCount" (dict "context" $ "component" "chromeNode") }}
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to use a helper template reduces code duplication, improving maintainability across multiple deployment files. However, it requires additional implementation of the helper template.

    6
    Enhancement
    Improve flexibility in autoscaling tests by using a variable for minimum replica count

    Update the test command to use a variable for setting the minimum replica count,
    allowing for more flexible testing of autoscaling behavior.

    Makefile [925]

    -SELENIUM_GRID_AUTOSCALING_MIN_REPLICA=0 SET_MAX_REPLICAS=3 TEST_DELAY_AFTER_TEST=2 SELENIUM_GRID_MONITORING=false  \
    +SELENIUM_GRID_AUTOSCALING_MIN_REPLICA=$(MIN_REPLICA_COUNT) SET_MAX_REPLICAS=3 TEST_DELAY_AFTER_TEST=2 SELENIUM_GRID_MONITORING=false  \
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a variable for the minimum replica count enhances flexibility in testing different autoscaling scenarios, making the tests more adaptable to changes.

    7
    Ensure all deployment resources use the RollingUpdate strategy

    Update the assertion for the 'recreate' strategy to match the new 'RollingUpdate'
    strategy. This ensures consistency with the changes made in the deployment
    configuration.

    tests/charts/templates/test.py [281-282]

     self.assertTrue(doc['spec']['strategy']['type'] == 'RollingUpdate', f"Resource {doc['metadata']['name']} doesn't have strategy RollingUpdate")
     count_recreate += 1
    +self.assertEqual(count_recreate, 0, "Found deployment resources with strategy Recreate, but all should be RollingUpdate")
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion ensures consistency by checking that no resources use the Recreate strategy, aligning with the PR's change to RollingUpdate. However, it may not be necessary if the test already covers this implicitly.

    5

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit e7ca1dc into trunk Oct 14, 2024
    51 of 52 checks passed
    @VietND96 VietND96 deleted the update-test branch October 14, 2024 01:22
    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.

    None yet

    1 participant