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

Autoscaling selenium grid on kubernetes with video recording #1689

Conversation

prashanth-volvocars
Copy link
Contributor

Description

Autoscale selenium browser nodes running in kubernetes based on the request pending in session queue using KEDA. It also includes ability to automatically record videos using ffmpeg and capture network, browser logs.
The recorded videos and logs are stored under <session_id> name and can be uploaded directly to S3.

Motivation and Context

Auto scaling selenium grid was a problem that was pending to be solved for long. So i took it up when there was a requirement at current my work place. KEDA seemed to the best candidate for the Job and i wrote a new scalar for Selenium Grid a year ago. I would like to have this enabled by default in our charts so everyone could use it.

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.

Autoscale selenium browser nodes running in kubernetes
based on the request pending in session queue using KEDA.
It also includes ability to automatically record videos
using ffmpeg and capture network, browser logs.
The recorded videos and logs can be uploaded to S3
which can be controlled using enviromental variables.
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2022

CLA assistant check
All committers have signed the CLA.

@jamesmortensen
Copy link
Member

Thank you for submitting the pull request regarding issue #1688 Please note it may take us some time to find someone who can review the changes.

@prashanth-volvocars
Copy link
Contributor Author

Sure thanks @jamesmortensen for the swift response. Hope to see this getting merged. Also there is a similar issue in the selenium project SeleniumHQ/selenium#9845 on the same topic

prashanth-volvocars and others added 2 commits October 4, 2022 07:18
If env variables used by video recorder script is not set, the script
error out which was fixed.

Enhanced prestop script to check if any video is currently being
uploaded to s3 before exiting.
@mhnaeem
Copy link
Contributor

mhnaeem commented Oct 14, 2022

@prashanth-volvocars Thank you for this work, my team is looking for an auto scaling solution and this PR would be great to have.

Personal opinion below - please wait for reply from contributors before making any changes
I do have one concern and perhaps @jamesmortensen or @diemol could chime in. Is it possible to make this video upload process cloud agnostic, since a lot of users won't be using AWS S3 buckets. Perhaps there is some way to let the user decide how the videos get processed or uploaded after being captured instead of the selenium repo handling the upload process which also comes at a cost of installing aws cli in every docker image.

Maybe we can have two PRs addressing the two separate parts - Autoscaling and Video Capture.

@jamesmortensen
Copy link
Member

@mhnaeem Thanks for jumping in. It does seems like we have two separate PR's here, and I think it would be easier to evaluate them if we could see them as separate PR's. @prashanth-volvocars what do you think?


Here's my feedback for the video recording portion of the PR:

Regarding the changes in NodeBase, I don't see any major issues with installing the AWS CLI. We do already install components that not everyone uses. For instance, for those who don't use VNC or noVNC, those components, including xvfb and fluxbox, are installed in the container images and end up doing nothing. Do we know how much extra space it takes to install the AWS CLI? If it's not trivial, then perhaps there are other ideas?

Regarding adding other cloud providers, I don't think we need to be concerned about that right now, I think we could merge with what we have. At the same time, we could make some space for it with a few abstract changes. For example, start-s3-uploader.sh could be encapsulated inside start-uploader.sh and then start-uploader.sh would either call start-s3-uploader.sh or start-gcp-uploader.sh depending on the environment variables. This would allow other providers to be easily added without disturbing other logic. If someone wanted to upload videos to Backblaze B2, then they'd add an implementation for start-b2-uploader.sh and open a pull request. We could potentially even script it in a way that the Dockerfile doesn't need to be modified for someone to add another provider.

What do you both think?

@prashanth-volvocars
Copy link
Contributor Author

Hello @jamesmortensen

Yeah sure we can have them as two separate PR's.

For the video upload to cloud provider. We can change it as per your suggestion to use start-uploader.sh and inside it call a scipt specific to a cloud provider. I will make these changes when i find some time and i will update the PR.

@jamesmortensen
Copy link
Member

Sounds great. :)

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the delay in a proper review. In general, yes, it'd be nice to have to separate PRs. Basically because this one is adding ffmpeg to the Node images, and that is a huge change how we could handle video recording.

I prefer to have a PR that only focuses on the Keda integration, and when we are there, we can discuss the video recording approach.

@prashanth-volvocars
Copy link
Contributor Author

@diemol @jamesmortensen

Closing this in favour of #1714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants