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

Docker: Video uploader process is spawned by env var flag SE_VIDEO_UPLOAD_ENABLED #2565

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

VietND96
Copy link
Member

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

In Node container could see logs as below

Trapped SIGTERM/SIGINT/x so shutting down supervisord...
2025-01-06 19:57:50,220 WARN received SIGTERM indicating exit request
2025-01-06 19:57:50,221 INFO waiting for xvfb, video-upload, vnc, novnc, selenium-standalone to die
2025-01-06 19:57:51,655 WARN stopped: selenium-standalone (terminated by SIGTERM)
2025-01-06 19:57:53,662 WARN stopped: novnc (terminated by SIGTERM)
2025-01-06 19:57:53,663 INFO waiting for xvfb, video-upload, vnc to die
2025-01-06 19:57:54,667 WARN stopped: vnc (terminated by SIGTERM)
2025-01-06 19:57:54,828 [video.uploader] - Trapped SIGTERM/SIGINT/x so shutting down uploader
2025-01-06 19:57:54,833 [video.uploader] - Start consuming pipe file to upload
2025-01-06 19:57:54,834 [video.uploader] - Stopped consuming pipe file. Upload process is done
2025-01-06 19:57:54,835 [video.uploader] - Uploader consumed all files in the pipe
2025-01-06 19:57:54,836 [video.uploader] - Uploader is ready to shutdown
2025-01-06 19:57:54,837 [video.uploader] - Trapped SIGTERM/SIGINT/x so shutting down uploader
2025-01-06 19:57:54,839 [video.uploader] - Start consuming pipe file to upload
2025-01-06 19:57:54,840 [video.uploader] - Received exit signal. Aborting upload process
2025-01-06 19:57:54,841 [video.uploader] - Uploader consumed all files in the pipe
2025-01-06 19:57:54,843 [video.uploader] - Uploader is ready to shutdown
2025-01-06 19:57:54,843 INFO stopped: video-upload (exit status 0)
2025-01-06 19:57:55,846 WARN stopped: xvfb (terminated by SIGTERM)
Shutdown complete

It is due to process is spawned by env var flag SE_VIDEO_INTERNAL_UPLOAD, which is always true

However, video uploader should be listen on flag SE_VIDEO_UPLOAD_ENABLED (which is false by default). So, expect that the process of uploader should not be present in container logs.

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


Description

  • Updated SE_VIDEO_UPLOAD_ENABLED to default to false in documentation.

  • Clarified default behavior of SE_VIDEO_INTERNAL_UPLOAD in documentation.

  • Changed video-upload process configuration to depend on SE_VIDEO_UPLOAD_ENABLED.

  • Adjusted video-upload process restart behavior based on SE_VIDEO_UPLOAD_ENABLED.


Changes walkthrough 📝

Relevant files
Documentation
README.md
Update documentation for video upload configuration           

README.md

  • Updated SE_VIDEO_UPLOAD_ENABLED to default to false.
  • Clarified default value of SE_VIDEO_INTERNAL_UPLOAD.
  • Improved documentation for video upload feature.
  • +2/-2     
    Enhancement
    uploader.conf
    Update video-upload process configuration                               

    Video/uploader.conf

  • Changed autostart to depend on SE_VIDEO_UPLOAD_ENABLED.
  • Changed autorestart to depend on SE_VIDEO_UPLOAD_ENABLED.
  • Adjusted process behavior for video upload feature.
  • +2/-2     

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

    Copy link

    qodo-merge-pro bot commented Jan 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Validation

    The video uploader process now depends on SE_VIDEO_UPLOAD_ENABLED instead of SE_VIDEO_INTERNAL_UPLOAD. Verify this change doesn't break existing video upload functionality, especially when SE_VIDEO_INTERNAL_UPLOAD is true but SE_VIDEO_UPLOAD_ENABLED is false.

    autostart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s
    startsecs=0
    autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s

    Copy link

    qodo-merge-pro bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add default values for environment variables in configuration files to ensure system stability

    Add a default value for SE_VIDEO_UPLOAD_ENABLED in the configuration to prevent
    startup issues if the environment variable is not set.

    Video/uploader.conf [5-7]

    -autostart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s
    +autostart=%(ENV_SE_VIDEO_UPLOAD_ENABLED:false)s
     startsecs=0
    -autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED)s
    +autorestart=%(ENV_SE_VIDEO_UPLOAD_ENABLED:false)s
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding default values for environment variables is crucial for system stability, especially since the PR changes make SE_VIDEO_UPLOAD_ENABLED default to false. This prevents potential startup issues if the environment variable is not set.

    8

    @VietND96 VietND96 merged commit d35048e into trunk Jan 6, 2025
    36 checks passed
    @VietND96 VietND96 deleted the recorder branch January 6, 2025 21:28
    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