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

SSH #359

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

SSH #359

wants to merge 7 commits into from

Conversation

lievenhey
Copy link
Contributor

@lievenhey lievenhey commented Jan 18, 2022

These patches allow hotspot to run perf record on remote devices via ssh. The recording data is streamed directly to hotspot without storing it on the device.

fixes #12

@GitMensch
Copy link
Contributor

Thank you for this nice addition, it seems that README.md may need an update (or is this actually a draft implementation so "too early for that")?

README.md Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/ssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/ssh.cpp Outdated Show resolved Hide resolved
src/ssh.cpp Outdated Show resolved Hide resolved
src/ssh.cpp Outdated Show resolved Hide resolved
@lievenhey
Copy link
Contributor Author

weird, somehow the ui on the mainwindow got changed, but I didn't touch that file.

@GitMensch
Copy link
Contributor

@milianw I'm wondering where this has gone, the requested changes seem to have been done.

src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/perfrecordssh.cpp Outdated Show resolved Hide resolved
src/ssh.h Outdated Show resolved Hide resolved
src/ssh.h Outdated Show resolved Hide resolved

class QFile;

class PerfRecordSSH : public PerfRecord
Copy link
Member

Choose a reason for hiding this comment

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

please add some basic documentation on how this is implemented. basically just something that says we overload a bunch of API to disable features and then use an external ssh process to communicate with another machine to remote control the perf session there

src/ssh.h Outdated
class QStringList;
class QProcessEnvironment;

QStringList assembleSSHArguments(const QString& deviceName, const QStringList& command);
Copy link
Member

Choose a reason for hiding this comment

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

please add basic api documentation to all of these

src/ssh.h Outdated Show resolved Hide resolved
@jeff-dagenais
Copy link

related to #12

@GitMensch
Copy link
Contributor

related to #12

Actually this "fixes #12" - maybe someone can edit the first post or better directly link it.

@GitMensch
Copy link
Contributor

Solving the conflicts would be nice; the reason I'm asking for it is that this would trigger CI, which now creates an AppImage which then can be used to test the feature before it is integrated.

@GitMensch
Copy link
Contributor

I've tested that with the generated appimage and when doing "test connection and save" I always just get an empty error message:

error

with stderr having

QProcess: Destroyed while process ("/usr/bin/ssh") is still running.

How to debug this issue? (note: /usr/bin/ssh -K ruser@rmach works like a charm)

Side note: the configuration dialog is named "Paths and Architecture Settings" which is getting even more wrong after applying this, I've created #436 to tackle that.

@lievenhey
Copy link
Contributor Author

lievenhey commented Nov 30, 2022

@GitMensch It should now work connecting does work, recording not (I messed up the order of arguments and ended up calling ssh host@user)

@GitMensch
Copy link
Contributor

Rechecked with hotspot-v1.3.0-374-g97c6a8e-x86_64.AppImage - still the same result (empty error + that message in stderr).
Can I suggest to add "command failed: $sshCommandHere" as start of the error message?

Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

only had time for the first patch for now

src/multiconfigwidget.cpp Outdated Show resolved Hide resolved
src/multiconfigwidget.cpp Outdated Show resolved Hide resolved
src/multiconfigwidget.cpp Outdated Show resolved Hide resolved
src/multiconfigwidget.cpp Show resolved Hide resolved
src/multiconfigwidget.h Outdated Show resolved Hide resolved
src/recordpage.cpp Outdated Show resolved Hide resolved
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

I think we also need some basic test coverage for this, maybe for starters a manual test binary that we can invoke manually to run some tests

then integrate that into the docker images somehow?

src/models/eventmodelproxy.cpp Outdated Show resolved Hide resolved
src/multiconfigwidget.cpp Show resolved Hide resolved
src/multiconfigwidget.cpp Show resolved Hide resolved
src/multiconfigwidget.h Show resolved Hide resolved
src/settingsdialog.cpp Outdated Show resolved Hide resolved
src/remotedevice.cpp Outdated Show resolved Hide resolved
src/remotedevice.cpp Outdated Show resolved Hide resolved
src/remotedevice.cpp Outdated Show resolved Hide resolved
src/remotedevice.cpp Show resolved Hide resolved
src/remotedevice.cpp Outdated Show resolved Hide resolved
This change makes it more usable by making the usage easier. Simply add
the edit widget as a child using addChild and be done. Now you only need
to call saveCurrentConfig and be done. Restoring and saving are handled
by the widget.
you can edit you devices, copy your ssh key and select you ssh binary on
this page
device are stored directly in a KConfigGroup, the deviceList property of
Settings is only there to pass the devices around to other widgets
Changes to existing logic:
RecordHost now saved the client app args
RecordHost no longer differentiate between local and remote when setting
the output file name, since the file is always saved on the host

New class RemoteDevice, this class handles communication between the
host and the remote device. It does this by using a multiplexed ssh
connection. This is faster since we only need to do authentification
once and can then reuse the existing connection.
There is some logic left from the time before RecordHost.
This patch removes these fragments
this patch finally runs perf via ssh on a remote device
perf is run with -o - to stream the recording to the host
in this case stderr contains the output of the program run
using coroutines allows use async io without having to deal with
multithreading
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.

allow launching perf on remote machines
4 participants