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: Update default component ConfigMap and resources limits #2596

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 18, 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

Support #2528

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, Configuration changes


Description

  • Added new ConfigMap definitions for sessionQueue and updated existing ConfigMaps to support dynamic data injection.

  • Updated resource limits and requests for various Kubernetes components to optimize performance.

  • Enhanced values.yaml with default Java options and resource configurations for better memory and CPU management.

  • Improved Helm templates to dynamically handle ConfigMap and Secret data using range loops.


Changes walkthrough 📝

Relevant files
Enhancement
10 files
_nameHelpers.tpl
Added ConfigMap fullname helper for sessionQueue                 
+7/-0     
distributor-configmap.yaml
Updated distributor ConfigMap to support dynamic data injection
+3/-0     
event-bus-configmap.yaml
Enhanced event-bus ConfigMap with dynamic data handling   
+2/-2     
logging-configmap.yaml
Added dynamic data injection for logging ConfigMap             
+3/-0     
node-configmap.yaml
Updated node ConfigMap to support dynamic data injection 
+3/-3     
router-configmap.yaml
Enhanced router ConfigMap with dynamic data handling         
+3/-0     
secrets.yaml
Updated secrets template to handle dynamic data injection
+7/-7     
server-configmap.yaml
Enhanced server ConfigMap with dynamic data handling         
+3/-3     
session-map-configmap.yaml
Added dynamic data injection for session-map ConfigMap     
+3/-0     
session-queue-configmap.yaml
Added new ConfigMap template for sessionQueue                       
+19/-0   
Documentation
1 files
CONFIGURATION.md
Documented new ConfigMap data and resource limits               
+20/-13 
Configuration changes
1 files
values.yaml
Updated default values for ConfigMaps and resource limits
+72/-14 

Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The VNC password is set to a default value of 'secret' in values.yaml. This could lead to unauthorized access if not changed by users. Consider removing the default value and requiring users to explicitly set a secure password.

    ⚡ Recommended focus areas for review

    Memory Configuration

    The Java memory settings in ConfigMaps use MaxRAMPercentage=100 for most components, which could lead to container OOM issues. Consider setting lower percentages or using absolute memory limits.

    data:
      SE_JAVA_OPTS: "-XX:+UseG1GC -XX:MaxGCPauseMillis=1000 -XX:MaxRAMPercentage=100"
    Default Password

    The default VNC password is set to 'secret' which is insecure. Consider requiring users to explicitly set this value rather than providing an insecure default.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect annotation reference that could cause missing server ConfigMap annotations

    Fix incorrect annotation reference in server ConfigMap template. Currently using
    busConfigMap.annotations instead of serverConfigMap.annotations, which could lead to
    missing or incorrect annotations.

    charts/selenium-grid/templates/server-configmap.yaml [7-9]

    -{{- with .Values.busConfigMap.annotations }}
    +{{- with .Values.serverConfigMap.annotations }}
       annotations: {{- toYaml . | nindent 4 }}
     {{- end }}
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Critical fix for incorrect annotation reference that would cause server ConfigMap annotations to be missing, as it's currently using busConfigMap.annotations instead of serverConfigMap.annotations.

    9
    General
    Add missing component label for better resource identification and management

    Add missing label section in session queue ConfigMap template to ensure proper
    Kubernetes resource identification and management.

    charts/selenium-grid/templates/session-queue-configmap.yaml [10-14]

     labels:
    +  app.kubernetes.io/component: session-queue
       {{- include "seleniumGrid.commonLabels" . | nindent 4 }}
       {{- with .Values.customLabels }}
         {{- toYaml . | nindent 4 }}
       {{- end }}
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While adding the component label would improve resource identification, it's not critical as the common labels already provide basic identification capabilities.

    4

    Copy link

    qodo-merge-pro bot commented Jan 18, 2025

    CI Failure Feedback 🧐

    (Checks updated until commit e31d575)

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub authentication token provided in GH_CLI_TOKEN_PR lacks the
    required 'read:org' permission scope. The GitHub CLI (gh) command attempted to authenticate using
    the token but failed due to insufficient permissions.

    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:85e6279cec87321a52edac9c87bce653a07cf6c2)
    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: 12843892379
    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: 12843892379
    127:  RERUN_FAILED_ONLY: true
    ...
    
    163:  0 upgraded, 0 newly installed, 0 to remove and 73 not upgraded.
    164:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    165:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    166:  shell: /usr/bin/bash -e {0}
    167:  env:
    168:  GH_CLI_TOKEN: ***
    169:  GH_CLI_TOKEN_PR: ***
    170:  RUN_ID: 12843892379
    171:  RERUN_FAILED_ONLY: true
    172:  RUN_ATTEMPT: 1
    173:  ##[endgroup]
    174:  error validating token: missing required scope 'read:org'
    175:  ##[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 2130c34 into trunk Jan 18, 2025
    17 of 19 checks passed
    @VietND96 VietND96 deleted the resource-limits branch January 18, 2025 13:58
    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