Fix/web ui terminal hypervisor #612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdknives
approved these changes
Nov 21, 2020
|
@i-hate-nicknames regarding potential downsides of using
therefore the only downside which I can see is the inability of the visor to manage itself over dmsgpty when |
|
@i-hate-nicknames @jdknives the only problem is that build keeps failing |
i-hate-nicknames
force-pushed
the
fix/web-ui-terminal-hypervisor
branch
from
9762a8c to
9fe7e97
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Did you run
make format && make check?yes
Fixes #605
Changes:
How to test this PR:
Go to hypervisor web api, select hypervisor in the list of visors, click actions button at the top right and select Terminal/Full Terminal
The issue with terminal in hypervisor had most likely appeared after merging visor and hypervisors under the same entity. Previously, hypervisor would connect to visors using dmesg. There is a package for that, called dmesgpty that offers exposing pty (pseudo terminal) over dmsg transport.
I played around for a while trying to create a pty locally and just serve it over websockets to the client, but it proved to be more architecturally difficult than I originally anticipated.
In this implementation I simply allowed a visor to connect to itself and created a dmsg pty at hypervisor start. This way it minimizes changes to the code (and therefore the risk introducing bugs), as well as no increase in the overall complexity.
On the other hand, we make from a node to itself over dmsg, which obviously is redundant. What are the shortcomings of this approach? Terminal interaction seems to me lightweight (infrequent request-response of small amount text). Could there be use-cases in which this would cause performance issues?
Additionally, I bumped creack/pty to 1.1.11, as the older version had a bug that didn't allow using the terminal.