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

Update _helpers.tpl to adress missing resource limits in pre-puller init containers for firefox,edge,... #2399

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

Koeppchen
Copy link
Contributor

@Koeppchen Koeppchen commented Sep 17, 2024

User description

Fixes to have pre-puller initContainer with resource limits.

Created to fix #2398

Description

Is already in the title. Resource Limit in the newly added pre-puller init container is missing resource limits.

Motivation and Context

If a kubernetes enforces resource limits it is not possible do rolle out changes with the init containers.

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


Description

  • Added resource limits to the pre-puller init containers in the Kubernetes configuration.
  • Utilized the .node.resources field to ensure that resource specifications are applied, addressing issues in environments enforcing resource limits.
  • This change fixes the inability to roll out changes when resource limits are enforced by Kubernetes.

Changes walkthrough 📝

Relevant files
Bug fix
_helpers.tpl
Add resource limits to pre-puller init containers               

charts/selenium-grid/templates/_helpers.tpl

  • Added resource limits to the pre-puller init containers.
  • Utilized .node.resources to define resource specifications.
  • +3/-0     

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

    Fixes to have pre-puller initContainer with resource limits.
    @CLAassistant
    Copy link

    CLAassistant commented Sep 17, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link

    PR Reviewer Guide 🔍

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

    Resource Specification
    The PR adds resource limits to pre-puller init containers, but it's worth verifying if the .node.resources field is properly defined and populated in all relevant configurations.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use a more informative command for the pre-puller init container

    Consider using a more specific command for the pre-puller init container instead of
    "true". A command like "echo 'Pre-pulling complete'" would provide more meaningful
    output and potentially aid in debugging.

    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: ["bash", "-c", "echo 'Pre-pulling {{ .name }} complete'"]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to replace "true" with a more descriptive command like "echo 'Pre-pulling {{ .name }} complete'" improves the clarity and debugging capability of the script, but it is not crucial for functionality.

    7

    @VietND96
    Copy link
    Member

    Thank you for the update. Could you please update the same for the video recorder in if statement also?

    Do the same for videoRecorder
    @Koeppchen
    Copy link
    Contributor Author

    Thank you for the update. Could you please update the same for the video recorder in if statement also?
    Thank you, makes sense.
    Done.

    @VietND96
    Copy link
    Member

    Thank you! Can you also take 1 minute to click on CLAsign in above comment and finish it?

    @VietND96 VietND96 merged commit 35fafaf into SeleniumHQ:trunk Sep 17, 2024
    31 of 34 checks passed
    @VietND96
    Copy link
    Member

    Thank you! @Koeppchen

    @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]: Resource Limits missing for pre-pull InitContainer
    3 participants