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(fix): ensure images are pre-pulled and started together in Node #2387

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

VietND96
Copy link
Member

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

Fixes: #2386

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

  • Added pre-puller init containers to ensure node and video recorder images are pre-pulled before starting, improving startup reliability.
  • Updated image registry and tag handling for better clarity and maintainability.
  • Enabled basicAuth by default in both dummy and solution configurations to enhance security settings.

Changes walkthrough 📝

Relevant files
Enhancement
_helpers.tpl
Add pre-puller init containers for image pre-pulling         

charts/selenium-grid/templates/_helpers.tpl

  • Added pre-puller init containers for node and video recorder images.
  • Ensured images are pre-pulled before starting containers.
  • Updated image registry and tag variables for clarity.
  • +16/-8   
    Configuration changes
    dummy.yaml
    Enable basicAuth in dummy configuration                                   

    tests/charts/templates/render/dummy.yaml

    • Enabled basicAuth by default in the configuration.
    +1/-0     
    dummy_solution.yaml
    Enable basicAuth in dummy solution configuration                 

    tests/charts/templates/render/dummy_solution.yaml

    • Enabled basicAuth by default in the solution configuration.
    +1/-0     

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

    @VietND96 VietND96 marked this pull request as ready for review September 9, 2024 01:40
    Copy link

    qodo-merge-pro bot commented Sep 9, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Error
    The command in the pre-puller init containers uses single quotes within double quotes, which may cause issues. Consider removing the single quotes.

    Code Duplication
    The pre-puller init container logic is duplicated for node and video recorder. Consider refactoring to reduce duplication.

    Copy link

    qodo-merge-pro bot commented Sep 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Optimization
    Add a condition to check if image pre-pulling is necessary

    Consider adding a condition to check if the image pre-pulling is necessary before
    running the pre-puller init containers. This can help optimize the deployment
    process by avoiding unnecessary image pulls.

    charts/selenium-grid/templates/_helpers.tpl [305-313]

     initContainers:
    +{{- if .Values.prePuller.enabled }}
       - name: "pre-puller-{{ .name }}"
         image: {{ printf "%s/%s:%s" $nodeImageRegistry .node.imageName $nodeImageTag }}
         command: ["bash", "-c", "'true'"]
     {{- if $.Values.videoRecorder.enabled }}
       - name: "pre-puller-{{ $.Values.videoRecorder.name }}"
         image: {{ printf "%s/%s:%s" $videoImageRegistry $.Values.videoRecorder.imageName $videoImageTag }}
         command: ["bash", "-c", "'true'"]
     {{- end }}
    +{{- end }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion optimizes the deployment process by avoiding unnecessary operations, which can lead to more efficient resource usage and faster deployments.

    9
    Best practice
    Add resource limits and requests to pre-puller init containers

    Consider adding resource limits and requests for the pre-puller init containers to
    ensure they don't consume excessive resources during the pre-pulling process.

    charts/selenium-grid/templates/_helpers.tpl [306-308]

     - name: "pre-puller-{{ .name }}"
       image: {{ printf "%s/%s:%s" $nodeImageRegistry .node.imageName $nodeImageTag }}
       command: ["bash", "-c", "'true'"]
    +  resources:
    +    limits:
    +      cpu: "100m"
    +      memory: "128Mi"
    +    requests:
    +      cpu: "50m"
    +      memory: "64Mi"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding resource limits and requests is a best practice that helps prevent resource exhaustion and ensures stable performance, making this a valuable improvement.

    8
    Enhancement
    Use a more meaningful command in pre-puller init containers

    Consider using a more specific command in the pre-puller init containers instead of
    "'true'". For example, you could use a command that checks the image's presence or
    performs a quick health check.

    charts/selenium-grid/templates/_helpers.tpl [306-308]

     - name: "pre-puller-{{ .name }}"
       image: {{ printf "%s/%s:%s" $nodeImageRegistry .node.imageName $nodeImageTag }}
    -  command: ["bash", "-c", "'true'"]
    +  command: ["sh", "-c", "echo 'Image pre-pulled successfully'"]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the clarity and purpose of the pre-puller init containers by replacing a generic command with a more descriptive one, enhancing code readability and maintainability.

    7

    @VietND96 VietND96 merged commit 44d9224 into SeleniumHQ:trunk Sep 9, 2024
    22 of 26 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
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Worker node initiates session before video container is fully ready
    1 participant