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

Build static FFmpeg and copy binary to Node base #2468

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 19, 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

This pull request introduces several updates to the Dockerfiles and Makefile to improve build processes, add new features, and enhance the flexibility of the build configurations. The most important changes include the addition of support for building FFmpeg from source, modifications to the Makefile to handle new build arguments, and updates to various Dockerfiles to accommodate new environment variables and build configurations.

Dockerfile Enhancements:

  • Added a new .ffmpeg/Dockerfile to build FFmpeg from source, including dependencies and installation steps for x264 and rclone.
  • Updated Base/Dockerfile to include new environment variables and additional packages such as xz-utils. [1] [2]
  • Modified NodeBase/Dockerfile to add new environment variables and update package installation commands. [1] [2]

Makefile Updates:

  • Introduced new targets in the Makefile for building and tagging FFmpeg images, and updated existing targets to accommodate these changes. [1] [2] [3] [4]
  • Adjusted the node_base target to include the new video target as a dependency.

Browser Node Dockerfile Changes:

  • Updated browser-specific Dockerfiles (NodeChrome, NodeChromium, NodeEdge, NodeFirefox) to use a new BASE argument for more flexible base image selection. [1] [2] [3] [4] [5]
  • Enhanced NodeFirefox/Dockerfile to include a script for installing Firefox via APT and handling different Firefox versions.

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, tests


Description

  • Introduced a new recorder-base Docker image that includes FFmpeg and Rclone for video recording and uploading.
  • Added several scripts for video recording, endpoint validation, and GraphQL queries to manage video sessions.
  • Updated workflows to extend build timeouts and adjust build steps.
  • Modified the Makefile to include a new recorder_base target and updated dependencies.
  • Enhanced the NodeBase Dockerfile to use the new recorder-base as its base image.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
upload.sh
Simplify rclone configuration handling in upload script   

RecorderBase/upload.sh

  • Replaced UPLOAD_CONFIG_DIRECTORY and UPLOAD_CONFIG_FILE_NAME with
    RCLONE_CONFIG.
  • Updated rclone command to use RCLONE_CONFIG.
  • +2/-3     
    validate_endpoint.sh
    Add endpoint validation script for HTTP checks                     

    RecorderBase/validate_endpoint.sh

  • Added a script to validate HTTP endpoints.
  • Supports basic and GraphQL endpoints.
  • +28/-1   
    video.sh
    Implement video recording and management script                   

    RecorderBase/video.sh

  • Added a script for video recording using FFmpeg.
  • Handles video upload and session management.
  • +241/-1 
    video_graphQLQuery.sh
    Add GraphQL query script for session details                         

    RecorderBase/video_graphQLQuery.sh

  • Added a script to query GraphQL for session details.
  • Extracts video recording capabilities and session information.
  • +83/-1   
    video_gridUrl.sh
    Add script to determine grid URL for video recording         

    RecorderBase/video_gridUrl.sh

    • Added a script to determine the grid URL for video recording.
    +21/-1   
    video_ready.py
    Add video readiness check script with HTTP server               

    RecorderBase/video_ready.py

  • Added a Python script to check if video recording is ready.
  • Uses HTTP server to report readiness status.
  • +33/-1   
    Dockerfile
    Update base Dockerfile with video folder and dependencies

    Base/Dockerfile

  • Added VIDEO_FOLDER environment variable.
  • Included python3-psutil in the installation.
  • +14/-12 
    Makefile
    Add recorder base target and update video build                   

    Makefile

  • Added recorder_base target.
  • Updated video build process to depend on recorder_base.
  • +35/-20 
    Dockerfile
    Update NodeBase Dockerfile to use recorder base                   

    NodeBase/Dockerfile

  • Changed base image to recorder-base.
  • Removed secret mount for password.
  • +6/-5     
    Dockerfile
    Add Dockerfile for recorder base with FFmpeg and Rclone   

    RecorderBase/Dockerfile

  • Added Dockerfile for building recorder-base with FFmpeg and Rclone.
  • +106/-0 
    Tests
    1 files
    test.py
    Update volume mount path in test configuration                     

    tests/charts/templates/test.py

    • Updated volume mount path for upload configuration.
    +1/-1     
    Configuration changes
    4 files
    deploy.yml
    Extend build timeout in deploy workflow                                   

    .github/workflows/deploy.yml

    • Increased timeout for build images step from 90 to 120 minutes.
    +1/-1     
    docker-test.yml
    Adjust Docker test workflow timeout and steps                       

    .github/workflows/docker-test.yml

  • Increased timeout for Docker image build from 20 to 60 minutes.
  • Removed pre-build step to reduce logs.
  • +2/-14   
    helm-chart-test.yml
    Extend build timeout in Helm chart test workflow                 

    .github/workflows/helm-chart-test.yml

    • Increased timeout for Docker image build from 12 to 60 minutes.
    +1/-1     
    nightly.yml
    Extend build timeout in nightly workflow                                 

    .github/workflows/nightly.yml

    • Increased timeout for build images step from 90 to 120 minutes.
    +1/-1     

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Basic auth exposure:
    The validate_endpoint.sh script uses base64 encoded credentials (BASIC_AUTH) which are stored in environment variables and could potentially be exposed through logs or error messages. Consider using more secure credential management.

    ⚡ Recommended focus areas for review

    Error Handling
    The script lacks proper error handling for FFmpeg failures. Video recording could silently fail without proper notification or recovery.

    Security Issue
    The script uses environment variables for authentication credentials which could be exposed in logs or error messages.

    Race Condition
    The script has a tight timeout (max_time=1) which could cause false negatives on slower networks or under load.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add timeout and force kill for graceful process termination

    The script uses kill -SIGTERM to stop ffmpeg but doesn't verify if the process was
    actually terminated. Add a timeout and force kill if needed.

    RecorderBase/video.sh [128-139]

    +timeout=10
     while true; do
       FFMPEG_PID=$(pgrep -f ffmpeg | tr '\n' ' ')
       if [ -n "$FFMPEG_PID" ]; then
         kill -SIGTERM $FFMPEG_PID
    -    wait $FFMPEG_PID
    +    for ((i=0; i<timeout; i++)); do
    +      if ! pgrep -f ffmpeg >/dev/null; then
    +        break 2
    +      fi
    +      sleep 1
    +    done
    +    kill -9 $FFMPEG_PID 2>/dev/null
       fi
       if ! pgrep -f ffmpeg >/dev/null; then
         break
       fi
       sleep ${poll_interval}
     done
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling for process termination, preventing potential hanging issues by implementing a timeout and force kill mechanism if graceful termination fails.

    8
    Add error handling for named pipe creation operations

    The script creates a named pipe without checking write permissions, which could fail
    silently. Add error handling for pipe creation.

    RecorderBase/video.sh [55-62]

     if [ ! -p "${UPLOAD_PIPE_FILE}" ]; then
       if [ -e "${UPLOAD_PIPE_FILE}" ]; then
    -    rm -f "${UPLOAD_PIPE_FILE}"
    +    rm -f "${UPLOAD_PIPE_FILE}" || { echo "Failed to remove existing file ${UPLOAD_PIPE_FILE}"; exit 1; }
       fi
    -  mkfifo "${UPLOAD_PIPE_FILE}"
    +  if ! mkfifo "${UPLOAD_PIPE_FILE}" 2>/dev/null; then
    +    echo "$(date -u +"${ts_format}") [${process_name}] - Failed to create named pipe ${UPLOAD_PIPE_FILE}"
    +    exit 1
    +  fi
       echo "$(date -u +"${ts_format}") [${process_name}] - Created named pipe ${UPLOAD_PIPE_FILE}"
     fi
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by checking for failures in pipe creation and file removal operations, preventing silent failures that could cause issues with video upload functionality.

    7
    General
    Minimize installed packages by preventing installation of recommended packages

    Add --no-install-recommends flag to the apt-get install command to minimize
    installed packages.

    RecorderBase/Dockerfile [71-77]

    -&& apt-get -qqy install \
    +&& apt-get -qqy --no-install-recommends install \
     libx11-6 \
     libxcb1 \
     libxcb-shm0 \
     x11-xserver-utils x11-utils \
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding --no-install-recommends flag is a good practice in Dockerfiles as it reduces the image size by preventing installation of unnecessary recommended packages, improving build efficiency and reducing potential security vulnerabilities.

    7

    💡 Need additional feedback ? start a PR chat

    @amardeep2006
    Copy link
    Contributor

    amardeep2006 commented Nov 19, 2024

    Want to share my past experience and concerns with the merging video image into browser images:

    1. rclone makes use of golang and it includes golang runtime into binary. If there is any vulnerability found in golang runtime then vulnerability scanners starts flagging the rclone as vulnerable packages. Rclone has to bumpup the golang version for such vulnerabilities. Unlike java the go runtime cannot be upgraded separately. It can cause issues in enterprise environments for those who scan images and do not use video feature. We have to wait for next rclone release (which at times takes months). I have faced this issue with rclone/golang few times .
    2. What will be final image size for browser images if we bundle python and rclone into them? The image size matters specially in autoscaling environments.

    I have nothing but praise for rclone but looking at the release cycles I am pretty sure it will block many enterprise users.
    image

    @VietND96
    Copy link
    Member Author

    I saw that Rclone would be similar to images in the KEDA core project. Sometimes we can see Go CVE that need to upgrade the Go version and rebuild the app on top of that. If the release cycle is not frequent, I think we can patch it ourselves, something like the KEDA images we recently did.
    Regarding the image size, I think it would be increased, but exactly how much I think I need a Nightly image published to the Docker hub to see. But there is a trade-off between image size and the number of images in a Pod

    @diemol
    Copy link
    Member

    diemol commented Nov 19, 2024

    The initial version of Docker Selenium, even before it was part of the Selenium org, had FFMPEG in it. It actually had all browsers in a single image. Which made the image quite heavy but made the usage very simple.

    Along time we changed because many users manifested themselves, they did not need images with everything included but rather the option to choose if they needed a given part. That is why we have tried to build images that focus on a single concern.

    Is deployment simplification the only motivation to do this?

    @VietND96
    Copy link
    Member Author

    VietND96 commented Nov 19, 2024

    Ok, then I will optimize a video container to record multiple Nodes using another approach.
    In this PR, we will keep the video container built from our Base and our Dockerfile instruction.

    Copy link

    qodo-merge-pro bot commented Nov 19, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 4d8047f)

    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:

  • The GitHub CLI (gh) attempted to authenticate using a token.
  • The token provided was missing the required scope read:org.
  • This resulted in a validation error, causing the process to exit with 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:cbb722410c2e876e24abbe8de2cc27693e501dcb)
    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: 12289201385
    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: 12289201385
    127:  RERUN_FAILED_ONLY: true
    ...
    
    165:  0 upgraded, 0 newly installed, 0 to remove and 50 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: 12289201385
    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.

    @amardeep2006
    Copy link
    Contributor

    Based on my experience, side car pattern is not a bad thing. It is very common to spot now a days. I have seen wide range from envoy-opa side cars, Dapr , vault side cars to service mesh . Good for separation of concern.

    @VietND96 VietND96 force-pushed the optimize-recorder branch 5 times, most recently from 22a50fb to 2423b66 Compare November 22, 2024 06:02
    @VietND96 VietND96 changed the title Build FFmpeg and optimize video container Use static FFmpeg and optimize video container Dec 7, 2024
    @VietND96 VietND96 force-pushed the optimize-recorder branch 9 times, most recently from d2255b8 to 73ed298 Compare December 11, 2024 17:15
    @VietND96 VietND96 changed the title Use static FFmpeg and optimize video container Build static FFmpeg and copy binary to Node base Dec 12, 2024
    @VietND96 VietND96 merged commit d66ac52 into trunk Dec 12, 2024
    55 of 60 checks passed
    @VietND96 VietND96 deleted the optimize-recorder branch December 12, 2024 05:14
    @VietND96 VietND96 linked an issue Dec 12, 2024 that may be closed by this pull request
    @VietND96
    Copy link
    Member Author

    In part of this change, I am adding capability to record/update video into Node itself with persisting the size of images. The result as below (note that the size is the sum of both amd64 and arm64)

    Before
    
    docker images | grep selenium
    selenium/video                 ffmpeg-7.1-20241211        708e91421376   11 minutes ago   1.81GB
    selenium/standalone-docker     4.28.0-SNAPSHOT-20241211   7da8c1adf880   12 minutes ago   1.09GB
    selenium/standalone-firefox    4.28.0-SNAPSHOT-20241211   5bb4fde3e095   12 minutes ago   3.25GB
    selenium/standalone-edge       4.28.0-SNAPSHOT-20241211   aeb520b2ab69   12 minutes ago   2.67GB
    selenium/standalone-chromium   4.28.0-SNAPSHOT-20241211   5e34248b647f   13 minutes ago   3.45GB
    selenium/standalone-chrome     4.28.0-SNAPSHOT-20241211   551296ef09ce   13 minutes ago   2.34GB
    selenium/node-docker           4.28.0-SNAPSHOT-20241211   0ec0754fc210   14 minutes ago   1.09GB
    selenium/node-firefox          4.28.0-SNAPSHOT-20241211   4ad25118ee74   18 minutes ago   3.25GB
    selenium/node-edge             4.28.0-SNAPSHOT-20241211   8e64d7c57f19   19 minutes ago   2.67GB
    selenium/node-chromium         4.28.0-SNAPSHOT-20241211   27fd909b2b13   23 minutes ago   3.45GB
    selenium/node-chrome           4.28.0-SNAPSHOT-20241211   620134402481   23 minutes ago   2.34GB
    selenium/node-base             4.28.0-SNAPSHOT-20241211   8dfea3d5ff40   33 minutes ago   2.2GB
    selenium/event-bus             4.28.0-SNAPSHOT-20241211   3505373519b7   34 minutes ago   1.08GB
    selenium/session-queue         4.28.0-SNAPSHOT-20241211   1738dbf2b5fa   34 minutes ago   1.08GB
    selenium/sessions              4.28.0-SNAPSHOT-20241211   048b95072800   34 minutes ago   1.08GB
    selenium/router                4.28.0-SNAPSHOT-20241211   4ab96475b6ce   34 minutes ago   1.08GB
    selenium/distributor           4.28.0-SNAPSHOT-20241211   aaae460f95c7   35 minutes ago   1.08GB
    selenium/hub                   4.28.0-SNAPSHOT-20241211   82bf81d37714   35 minutes ago   1.08GB
    selenium/base                  4.28.0-SNAPSHOT-20241211   f1ced4583b1b   47 minutes ago   1.08GB
    
    ---
    
    After
    
    docker images | grep selenium
    selenium/video                 ffmpeg-7.1-20241212        85726023b9a7   29 minutes ago   1.7GB
    selenium/standalone-docker     4.28.0-SNAPSHOT-20241212   701e4915f814   4 seconds ago    1.05GB
    selenium/standalone-firefox    4.28.0-SNAPSHOT-20241212   3681f1c25388   19 seconds ago   3.29GB
    selenium/standalone-edge       4.28.0-SNAPSHOT-20241212   9a34d53217d9   30 seconds ago   2.89GB
    selenium/standalone-chromium   4.28.0-SNAPSHOT-20241212   b5ca67841712   41 seconds ago   3.73GB
    selenium/standalone-chrome     4.28.0-SNAPSHOT-20241212   d20f1825ee78   52 seconds ago   2.57GB
    selenium/node-docker           4.28.0-SNAPSHOT-20241212   5d9f463a4723   2 minutes ago    1.05GB
    selenium/node-firefox          4.28.0-SNAPSHOT-20241212   2aad5e94e869   6 minutes ago    3.29GB
    selenium/node-edge             4.28.0-SNAPSHOT-20241212   a2cda2c4b9d0   7 minutes ago    2.89GB
    selenium/node-chromium         4.28.0-SNAPSHOT-20241212   45d3c96b401c   11 minutes ago   3.73GB
    selenium/node-chrome           4.28.0-SNAPSHOT-20241212   5a947c9a87e1   11 minutes ago   2.57GB
    selenium/node-base             4.28.0-SNAPSHOT-20241212   c5eab1490f88   23 minutes ago   2.48GB
    selenium/event-bus             4.28.0-SNAPSHOT-20241212   b955283ed516   29 minutes ago   1.05GB
    selenium/session-queue         4.28.0-SNAPSHOT-20241212   7ccc418cbf2f   29 minutes ago   1.05GB
    selenium/sessions              4.28.0-SNAPSHOT-20241212   36cc4fd1ff4a   29 minutes ago   1.05GB
    selenium/router                4.28.0-SNAPSHOT-20241212   996ee88b8c55   30 minutes ago   1.05GB
    selenium/distributor           4.28.0-SNAPSHOT-20241212   222e698ed352   30 minutes ago   1.05GB
    selenium/hub                   4.28.0-SNAPSHOT-20241212   63290012a53d   30 minutes ago   1.05GB
    selenium/base                  4.28.0-SNAPSHOT-20241212   088b58a0c4c0   40 minutes ago   1.05GB
    

    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]: Optimize video recorder containers (Phase 1)
    3 participants