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

[grid][java]: dynamic grid re-fetch browser images if they were pruned during runtime #13821

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 14, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Fix: SeleniumHQ/docker-selenium#1644

Steps to reproduce:

  • launch Selenium Grid in dynamic grid mode (in our case, grid was deployed in docker stack, but this should not make a difference)
  • spin up a browser node in dynamic grid and terminate it (image is persisted afterwards)
  • perform docker image prune -a
  • try to spin up the same browser node again

Expected behavior:

  • browser node image is fetched again as it is missing locally
  • browser node is launched successfully after image was fetched

Actual behavior:
Grid fails fatally trying to spin up a browser with the now missing image.

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.

Type

enhancement, bug_fix


Description

  • Added a new method getImage() in ContainerConfig to facilitate image retrieval.
  • Enhanced the container creation process to automatically re-fetch browser images if they were pruned during runtime, addressing a fatal grid failure issue.
  • Implemented a retry mechanism in the container creation process to handle cases where the Docker image is not found locally, by attempting to pull the image again before retrying.

Changes walkthrough

Relevant files
Enhancement
ContainerConfig.java
Add Method to Retrieve Image from ContainerConfig               

java/src/org/openqa/selenium/docker/ContainerConfig.java

  • Added a method getImage() to retrieve the image from a ContainerConfig
    object.
  • +4/-0     
    Bug_fix
    CreateContainer.java
    Implement Image Re-fetch Logic for Docker Containers         

    java/src/org/openqa/selenium/docker/v1_41/CreateContainer.java

  • Implemented a retry mechanism in apply method to handle "No such
    image" DockerException.
  • Added a createContainer method to encapsulate container creation
    logic.
  • Utilized PullImage to fetch the image if it's not found locally before
    retrying container creation.
  • +15/-0   

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

    Copy link
    Contributor

    PR Description updated to latest commit (079cc93)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are localized to specific methods within two classes, and the logic added is straightforward. The addition of a method to retrieve the image from a container configuration and the handling of a specific Docker exception to re-pull an image if it's not found are clear and concise modifications.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Error Handling: The catch block in CreateContainer.apply only checks for the presence of "No such image" in the exception message. This could potentially lead to false positives if the DockerException message changes or includes "No such image" in a different context.

    Retry Logic: The retry mechanism after pulling the image does not have a limit. If the pull image operation fails silently (i.e., does not throw an exception but does not result in the image being available), this could lead to an infinite loop.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Best practice
    Limit the visibility of the getImage method if it's for internal use.

    Consider making the getImage method package-private or protected if it's intended for
    internal use only, to adhere to the principle of least privilege.

    java/src/org/openqa/selenium/docker/ContainerConfig.java [87-89]

    -public Image getImage() {
    +Image getImage() {
       return this.image;
     }
     
    Use error codes for DockerException handling.

    Consider handling specific DockerException subtypes or error codes for more granular error
    handling instead of relying on message content.

    java/src/org/openqa/selenium/docker/v1_41/CreateContainer.java [59-62]

    -if (e.getMessage().contains("No such image")) {
    +if (e.getStatusCode() == HttpStatus.NOT_FOUND) { // Assuming DockerException includes a method to get HTTP status
       PullImage pullImage = new PullImage(client);
       pullImage.apply(Reference.parse(info.getImage().getName()));
       return createContainer(info);
     }
     
    Maintainability
    Refactor image pulling logic into a separate method.

    Extract the logic for pulling an image into a separate method to improve readability and
    maintainability.

    java/src/org/openqa/selenium/docker/v1_41/CreateContainer.java [56-66]

     try {
       return createContainer(info);
     } catch (DockerException e) {
       if (e.getMessage().contains("No such image")) {
    -    PullImage pullImage = new PullImage(client);
    -    pullImage.apply(Reference.parse(info.getImage().getName()));
    -    return createContainer(info);
    +    pullAndRetryCreateContainer(info);
       } else {
         throw e;
       }
     }
     
    +private Container pullAndRetryCreateContainer(ContainerConfig info) {
    +  PullImage pullImage = new PullImage(client);
    +  pullImage.apply(Reference.parse(info.getImage().getName()));
    +  return createContainer(info);
    +}
    +
    Enhancement
    Add logging before image pull operations.

    Add logging before pulling an image to aid in debugging and monitoring the pull process.

    java/src/org/openqa/selenium/docker/v1_41/CreateContainer.java [60-61]

    +logger.info("Pulling image: " + info.getImage().getName());
     PullImage pullImage = new PullImage(client);
     pullImage.apply(Reference.parse(info.getImage().getName()));
     
    Possible issue
    Prevent infinite loops in image pull retry logic.

    Ensure that the retry logic after pulling an image does not lead to an infinite loop in
    case of repeated failures.

    java/src/org/openqa/selenium/docker/v1_41/CreateContainer.java [59-62]

     if (e.getMessage().contains("No such image")) {
       PullImage pullImage = new PullImage(client);
       pullImage.apply(Reference.parse(info.getImage().getName()));
    -  return createContainer(info);
    +  try {
    +    return createContainer(info);
    +  } catch (DockerException retryException) {
    +    logger.error("Failed to create container even after pulling image", retryException);
    +    throw retryException;
    +  }
     }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96
    Copy link
    Member Author

    @diemol, could you please review this?

    @codecov-commenter
    Copy link

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.72%. Comparing base (8d77159) to head (2608802).
    Report is 6 commits behind head on trunk.

    ❗ Current head 2608802 differs from pull request most recent head c5c54e7. Consider uploading reports for the commit c5c54e7 to get more accurate results

    ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #13821      +/-   ##
    ==========================================
    - Coverage   58.82%   58.72%   -0.10%     
    ==========================================
      Files          86       86              
      Lines        5272     5298      +26     
      Branches      219      226       +7     
    ==========================================
    + Hits         3101     3111      +10     
    - Misses       1952     1961       +9     
    - Partials      219      226       +7     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    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.

    [🚀 Feature]: Dynamic Grid does not re-fetch browser images, when they were pruned during runtime
    3 participants