Skip to content

chart: Deployment scale metricType should be Value instead of AverageValue #2465

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 14, 2024
Merged

chart: Deployment scale metricType should be Value instead of AverageValue #2465

merged 1 commit into from
Nov 14, 2024

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 14, 2024 •
edited
Loading

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

Following KEDA docs - https://keda.sh/docs/2.16/reference/scaledobject-spec/#triggers, adjust configs in default chart values for ScaledObject

  • useCachedMetrics: false
  • metricType:"" (keep as empty, not set)

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

Bug fix, Enhancement

Description

  • Updated the autoscaling configuration to include useCachedMetrics and metricType options, enhancing the scaling logic.
  • Changed the Debian source in the NodeChromium Dockerfile from sid to stable for better stability.
  • Improved the smoke tests by setting default environment variables to None and adding HTTP basic authentication for requests.
  • Updated documentation to reflect new autoscaling options in the configuration.

Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Update environment variables and add authentication for requests

tests/SmokeTests/init.py

  • Changed default environment variable values to None.
  • Added CHART_CERT_PATH environment variable.
  • Updated requests.get to include HTTP basic authentication.
    		•  +6/-3     
    _helpers.tpl
    Enhance autoscaling triggers with additional configurations

    charts/selenium-grid/templates/_helpers.tpl

  • Added useCachedMetrics and metricType to autoscaling triggers.
  • Ensured nodeMaxSessions is set if not provided.
    		•  +7/-5     
    Dockerfile
    Update Debian source to stable in Dockerfile                         

    NodeChromium/Dockerfile

    • Changed Debian source from sid to stable.
    		 +1/-1     
    Documentation
    CONFIGURATION.md
    Add documentation for new autoscaling options                       

    charts/selenium-grid/CONFIGURATION.md

    • Documented useCachedMetrics and metricType options.
    		 +2/-0     
    Configuration changes
    values.yaml
    Add new autoscaling configuration options                               

    charts/selenium-grid/values.yaml

  • Added useCachedMetrics and metricType to autoscaling configuration.
    		•  +5/-0     

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

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    2464 - Partially compliant

    Compliant requirements:

    • Fix scaling issue by changing metricType to 'Value' instead of 'AverageValue' in KEDA configuration

    Non-compliant requirements:

    • No automated tests added to verify the scaling behavior
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Authentication Credentials Handling:
    The PR modifies how authentication credentials are handled in tests/SmokeTests/init.py, changing from empty string defaults to None. While this is generally safer, it should be verified that this doesn't break existing authentication flows or expose the system to unauthorized access.

    ⚡ Recommended focus areas for review

    Security Configuration
    The change from empty string defaults to None for authentication credentials might affect existing deployments that rely on empty string behavior

    Stability Risk
    Changing Debian source from 'sid' to 'stable' might affect chromium version compatibility

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle missing authentication credentials gracefully to prevent potential runtime errors

    Add error handling for the case when authentication credentials are required but not
    provided, as SELENIUM_GRID_USERNAME and SELENIUM_GRID_PASSWORD could be None.

    tests/SmokeTests/init.py [59]

    -response = requests.get(grid_url_status, verify=cert_path, auth=HTTPBasicAuth(SELENIUM_GRID_USERNAME, SELENIUM_GRID_PASSWORD))
+auth = HTTPBasicAuth(SELENIUM_GRID_USERNAME, SELENIUM_GRID_PASSWORD) if SELENIUM_GRID_USERNAME and SELENIUM_GRID_PASSWORD else None
+response = requests.get(grid_url_status, verify=cert_path, auth=auth)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime error by gracefully handling cases where authentication credentials are None, which is now the default value. This is particularly important as the PR changed the default values from empty strings to None.

    		8
    Validate certificate file existence before using it to prevent potential runtime errors

    Validate the certificate path before using it in requests.get() to avoid runtime
    errors if the certificate file is not accessible.

    tests/SmokeTests/init.py [20]

    -os.environ['REQUESTS_CA_BUNDLE'] = CHART_CERT_PATH
+if CHART_CERT_PATH and os.path.isfile(CHART_CERT_PATH):
+    os.environ['REQUESTS_CA_BUNDLE'] = CHART_CERT_PATH
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion prevents potential runtime errors by validating the certificate file's existence before setting it as an environment variable, which is important since CHART_CERT_PATH can now be None in the updated code.

    		7
    Best practice
    Specify architecture in package repository configuration for more precise dependency management

    Add version pinning for the Debian packages to ensure reproducible builds and
    prevent unexpected updates.

    NodeChromium/Dockerfile [12]

    -RUN echo "deb ${CHROMIUM_DEB_SITE}/ stable main" >> /etc/apt/sources.list \
+RUN echo "deb [arch=amd64] ${CHROMIUM_DEB_SITE}/ stable main" >> /etc/apt/sources.list \
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While adding architecture specification provides more explicit package management, it's a minor improvement since the default architecture would likely work correctly in most cases. The change from 'sid' to 'stable' is already handling the main stability concern.

    		4

    💡 Need additional feedback ? start a PR chat

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 14, 2024 •
    edited
    Loading

    CI Failure Feedback 🧐

    (Checks updated until commit 89c7f98)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed due to an error in the authentication process with GitHub CLI:

  • The token provided for authentication is missing the required scope read:org.
  • This missing scope caused the validation of the token to fail, resulting in an exit code 1.

    • 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:3b9b8c884f6b4bb4d5be2779c26374abadae0871)
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: 11828529104
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: 11828529104
127:  RERUN_FAILED_ONLY: true
...

165:  0 upgraded, 0 newly installed, 0 to remove and 55 not upgraded.
166:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
167:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
168:  shell: /usr/bin/bash -e {0}
169:  env:
170:  GH_CLI_TOKEN: ***
171:  GH_CLI_TOKEN_PR: ***
172:  RUN_ID: 11828529104
173:  RERUN_FAILED_ONLY: true
174:  RUN_ATTEMPT: 1
175:  ##[endgroup]
176:  error validating token: missing required scope 'read:org'
177:  ##[error]Process completed with exit code 1.
    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @VietND96 VietND96 merged commit 33471fe into trunk Nov 14, 2024
    51 of 53 checks passed
    @VietND96 VietND96 deleted the scale-deployment branch November 14, 2024 04:38
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @VietND96