-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: add support for VNC using x11vnc and novnc #15
Conversation
Thank you for the PR @synacktraa! I'll review the PR soon. Can you provide steps to test the VNC support? |
desktop = Desktop()
desktop.vnc_url That's it! |
@mlejva A suggestion: Build the template and publish it from your account as currently the template is under my account. |
This is beautiful. I just tried it and it works without issues. Nice PR! I'll dedicate some time to review it in the near future. Thank you again! |
Alright, In the meantime I will learn some ts and update the js-sdk |
Hi @mlejva, I have added browser only templates. They are lightweight (~1.34gb) unlike the desktop (~3.3gb) and works just fine with the SDK. It also supports VNC. |
Hey, thanks for the PR, looks very cool! Did you also test connecting via a VNC client or does it only work through the web browser currently? |
Yes I didn't knew about novnc initially. I tried exposing port 5900 but wasn't able to connect to it through VNC client (I used TightVNC viewer). I think novnc is already good enough, we can add this as an iframe (If we build something like openai's operator). Is there any particular reason why you want to use VNC client? If you're testing it rn, you can try once - |
Hey @synacktraa please keep this in a separate PR/discussion and leave this one only for the VNC support |
Okay yeah, I am sorry about this. I will remove this and create a PR from another branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa can you leave this PR for purely VNC support? I see you made several unrelated changes in this PR like removing pyautogui
and using xdotool
instead.
I'd like to keep things strictly separate in their own PRs. This PR should only add VNC support and we can move from there.
I get it, but the |
I see. Using The game plan:
Side note - we won't be able to merge this without updates to the JS SDK as well. |
Sounds good. We can discuss about 3rd and 4th point after I am done with the first two. |
templates/ubuntu-desktop/start-up.sh
Outdated
start_x11vnc() { | ||
(x11vnc -display :0 \ | ||
-forever \ | ||
$VNC_SHARED_FLAG \ | ||
-wait 50 \ | ||
-rfbport 5900 \ | ||
$VNC_PW_FLAG \ | ||
2>/tmp/x11vnc_stderr.log) & | ||
|
||
x11vnc_pid=$! | ||
|
||
# Wait for x11vnc to start | ||
timeout=10 | ||
while [ $timeout -gt 0 ]; do | ||
if netstat -tuln | grep ":5900 "; then | ||
break | ||
fi | ||
sleep 1 | ||
((timeout--)) | ||
done | ||
|
||
( | ||
while true; do | ||
if ! kill -0 $x11vnc_pid 2>/dev/null; then | ||
echo "x11vnc process crashed, restarting..." >&2 | ||
if [ -f /tmp/x11vnc_stderr.log ]; then | ||
echo "x11vnc stderr output:" >&2 | ||
cat /tmp/x11vnc_stderr.log >&2 | ||
rm /tmp/x11vnc_stderr.log | ||
fi | ||
start_x11vnc | ||
return | ||
fi | ||
sleep 5 | ||
done | ||
) & | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how this function works and why it's being called recursively?
I understand that the point is to check if vnc crashed and then try to restart it but would like to have it explained in the codebase as well.
Also, is this even required? If the vnc is failing shouldn't we just fail completely? What are the use cases when vnc can fail to start and we should keep trying starting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa @mlejva Another way to approach this would be to not start the VNC server automatically on startup but make it a method in the SDK.
This would:
- Prevent unnecessary overhead when VNC isn't being used.
- Allow developers to handle the VNC server failing in their preferred manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how this function works and why it's being called recursively?
I understand that the point is to check if vnc crashed and then try to restart it but would like to have it explained in the codebase as well.
Also, is this even required? If the vnc is failing shouldn't we just fail completely? What are the use cases when vnc can fail to start and we should keep trying starting it?
So first we run the x11vnc to start the vnc server, then we just give it some time to start. After that we use "kill -0" to check if the server is running without actually killing it - This happens in background, so if the server crashes or stops for some reason we just try to restart it.
I have not yet faced any fails when starting it. This was just to ensure If anything goes wrong it should start the service again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa @mlejva Another way to approach this would be to not start the VNC server automatically on startup but make it a method in the SDK.
This would:
Prevent unnecessary overhead when VNC isn't being used.
Allow developers to handle the VNC server failing in their preferred manner.
Makes sense. If you want this to be implemented just give a thumbs up to this reply. @jamesmurdza @mlejva
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa @mlejva Another way to approach this would be to not start the VNC server automatically on startup but make it a method in the SDK.
This would:
- Prevent unnecessary overhead when VNC isn't being used.
- Allow developers to handle the VNC server failing in their preferred manner.
@synacktraa can you test how long a user would wait during runtime to start the VNC server after calling something like sandbox.start_vnc_server()
? I like this approach more than having it enabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay sure.
templates/ubuntu-desktop/start-up.sh
Outdated
-forever \ | ||
$VNC_SHARED_FLAG \ | ||
-wait 50 \ | ||
-rfbport 5900 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put things like the 5900
VNC port number into a variable so it can be reused in subsequent calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa please expand on what packages you added and why they're needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting following error when trying to install xfce4-goodies
. Did it work for you? Is that package required?
ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get -y upgrade && apt-get -y install xfce4 xfce4-panel xfce4-goodies xfce4-session xvfb xterm xdotool scrot imagemagick sudo mutter x11vnc build-essential libssl-dev zlib1g-dev libbz2-dev libreadline-dev libsqlite3-dev curl wget git libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev net-tools netcat software-properties-common && sudo add-apt-repository ppa:mozillateam/ppa && sudo apt-get install -y --no-install-recommends libreoffice firefox-esr x11-apps xpdf gedit xpaint tint2 galculator pcmanfm unzip && apt-get clean" did not complete successfully: exit code: 100
Error: Command failed: docker build . -f e2b.dev.Dockerfile --pull --platform linux/amd64 -t docker.e2b.dev/e2b/custom-envs/xrlk1xkhq3q708rhhqjg:70cb8d1b-b738-4711-90c5-2e20d9b7b85d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same is happening when trying to install xfce4-terminal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it did install without any issue or else It wouldn't be possible to build the image.
It contains some misc software and artwork. https://packages.debian.org/buster/xfce/xfce4-goodies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same is happening when trying to install
xfce4-terminal
.
Okay, this is unexpected. Are there any more packages that is throwing the same error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting following error when trying to install
xfce4-goodies
. Did it work for you? Is that package required?ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get -y upgrade && apt-get -y install xfce4 xfce4-panel xfce4-goodies xfce4-session xvfb xterm xdotool scrot imagemagick sudo mutter x11vnc build-essential libssl-dev zlib1g-dev libbz2-dev libreadline-dev libsqlite3-dev curl wget git libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev net-tools netcat software-properties-common && sudo add-apt-repository ppa:mozillateam/ppa && sudo apt-get install -y --no-install-recommends libreoffice firefox-esr x11-apps xpdf gedit xpaint tint2 galculator pcmanfm unzip && apt-get clean" did not complete successfully: exit code: 100 Error: Command failed: docker build . -f e2b.dev.Dockerfile --pull --platform linux/amd64 -t docker.e2b.dev/e2b/custom-envs/xrlk1xkhq3q708rhhqjg:70cb8d1b-b738-4711-90c5-2e20d9b7b85d
I don't any see particular error message stating that goodies package has failed. Can you provide me with the whole error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa please expand on what packages you added and why they're needed
I am going to separate the installs in different RUN blocks so it easier to see and faster to rebuild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors look like it was my issue. You can ignore it. Sorry.
Can you still do this please?
please expand on what packages you added and why they're needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, give me some time. I will reply when I get to my laptop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synacktraa please expand on what packages you added and why they're needed
I am going to separate the installs in different RUN blocks so it easier to see and faster to rebuild.
I just did this in my latest commit
@synacktraa I created a separate template for development called |
Sure, I will restore the original template directory. |
Besides my comments above, there are two additional things I'd like to resolve before getting merged:
I also added a multi-client support in my previous commit |
|
@mlejva What about password authentication? Are we including it? |
And If we're starting up vnc through code, why don't we do the same with xvfb? That way we can decide the screen resolution when creating sandbox instance. This can also bypass the issue with providing env vars to start up script. |
By the way, I also added a new branch called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DX is like 80% there. Same comments apply to Python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the examples up-to-date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Usage: #15 (comment)
*/ | ||
async moveMouse(x: number, y: number) { | ||
return this.runPyautoguiCode(`pyautogui.moveTo(${x}, ${y})`) | ||
private async setVncCommand(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setVncCommand(novncAuthEnabled)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this later.
packages/js-sdk/src/sandbox.ts
Outdated
return this.runPyautoguiCode(`pyautogui.moveTo(${x}, ${y})`) | ||
private async setVncCommand(): Promise<void> { | ||
let pwdFlag = "-nopw"; | ||
if (this.desktop.novncAuthEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (novncAuthEnabled) {}
return out | ||
} | ||
if (this.vncCommand === "") { | ||
await this.setVncCommand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.setVncCommand({ novncAuthEnabled })
I've made the eight changes (separate commits above) that @mishushakov and I talked about, and also deleted the extra README file for the Python SDK. All of our tests and our example files are working. Ideally would test a bit more, but I think it is ready to ship. |
@mlejva we're ready for release here, but before you should change "desktop" template alias to point to template |
We can't do that because that's not E2B's (team owner) template. I'll update the desktop template based on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebuilt the desktop
template and it works with this branch.
- ✅
Please move back the Python readme (and make sure it's updated to reflect the changes). Without readmepoetry install
won't work and the published package won't have a readme. - ✅
Add changeset so the new SDKs get released.
README.md
Outdated
|
||
# Start the stream | ||
desktop.stream.start( | ||
port=6080, # Custom HTTPS port for streaming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is setting this useful?
The comments should explain when a user would want this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed an example of how to change the port from the readmes.
@jamesmurdza @mishushakov correct me if I'm wrong but I don't think changing the port is something a user would do without going deep into customizing the sandbox template and changing the port of the noVNC server itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's okay to leave out
…by invalidating Docker cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up implementing the changes I requested. This is good to get merged and released.
This don't really do anything now because environment variables passed to the sandbox are not available to start-up.sh script. Please look into this issue.It doesn't work right now because of the above mentioned issue.Todo: Update js-sdk